| Message ID | 1341321642-24598-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 03.07.2012 15:20, schrieb Paolo Bonzini: > If the guest does not support flushes, we should run in writethrough mode. > The setting is temporary until the next reset, so that for example the > BIOS will run in writethrough mode while Linux will run with a writeback > cache. > > VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and > was backported to RHEL/CentOS 5.6 (in 2010). The Windows drivers have > two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and > 837324. With these patches they will suffer a performance hit but > gain correctness. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> I generally like the idea for a default, but doesn't this override even an explicit cache=writeback? Are we sure that we want this? Kevin
Il 03/07/2012 15:49, Kevin Wolf ha scritto: >> If the guest does not support flushes, we should run in writethrough mode. >> > The setting is temporary until the next reset, so that for example the >> > BIOS will run in writethrough mode while Linux will run with a writeback >> > cache. >> > >> > VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and >> > was backported to RHEL/CentOS 5.6 (in 2010). The Windows drivers have >> > two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and >> > 837324. With these patches they will suffer a performance hit but >> > gain correctness. >> > >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > I generally like the idea for a default, but doesn't this override even > an explicit cache=writeback? Yes. It doesn't override cache=unsafe though. > Are we sure that we want this? The idea is that this change will overcome Anthony's objections to switching the default to writeback... Paolo
Am 03.07.2012 15:51, schrieb Paolo Bonzini: > Il 03/07/2012 15:49, Kevin Wolf ha scritto: >>> If the guest does not support flushes, we should run in writethrough mode. >>>> The setting is temporary until the next reset, so that for example the >>>> BIOS will run in writethrough mode while Linux will run with a writeback >>>> cache. >>>> >>>> VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and >>>> was backported to RHEL/CentOS 5.6 (in 2010). The Windows drivers have >>>> two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and >>>> 837324. With these patches they will suffer a performance hit but >>>> gain correctness. >>>> >>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> I generally like the idea for a default, but doesn't this override even >> an explicit cache=writeback? > > Yes. It doesn't override cache=unsafe though. When the guest doesn't support flushes, cache=writeback is equivalent to cache=unsafe, so if you want the old behaviour back you can switch to cache=unsafe without additional risks. We don't have a cache=directunsafe, though, so if you want to get the old behaviour of cache=none back, you're out of luck. Not sure how acceptable this is. Irrespective of this concern I've come to the conclusion that I agree and we actually must enforce this for non-unsafe mode, and not doing it is a bug. Kevin
Il 04/07/2012 12:16, Kevin Wolf ha scritto: >> > Yes. It doesn't override cache=unsafe though. > When the guest doesn't support flushes, cache=writeback is equivalent to > cache=unsafe, so if you want the old behaviour back you can switch to > cache=unsafe without additional risks. > > We don't have a cache=directunsafe, though, so if you want to get the > old behaviour of cache=none back, you're out of luck. Not sure how > acceptable this is. If we want to fix this, let's take the occasion to split the parameters into cache=on/off (well, we have that already), flush=on/off, and a device-side wce=on/off. > Irrespective of this concern I've come to the conclusion that I agree > and we actually must enforce this for non-unsafe mode, and not doing it > is a bug. Thanks! Is that an Acked-by/Reviewed-by? :) Paolo
Am 04.07.2012 14:21, schrieb Paolo Bonzini: > Il 04/07/2012 12:16, Kevin Wolf ha scritto: >>>> Yes. It doesn't override cache=unsafe though. >> When the guest doesn't support flushes, cache=writeback is equivalent to >> cache=unsafe, so if you want the old behaviour back you can switch to >> cache=unsafe without additional risks. >> >> We don't have a cache=directunsafe, though, so if you want to get the >> old behaviour of cache=none back, you're out of luck. Not sure how >> acceptable this is. > > If we want to fix this, let's take the occasion to split the parameters > into cache=on/off (well, we have that already), flush=on/off, and a > device-side wce=on/off. You're volunteering? Great! ;-) >> Irrespective of this concern I've come to the conclusion that I agree >> and we actually must enforce this for non-unsafe mode, and not doing it >> is a bug. > > Thanks! Is that an Acked-by/Reviewed-by? :) Before merging the patches (or actually this patch, I think patch 1 is fairly independent), I'd like to hear more opinions on whether we need the cache parameter split first. But as far as the hardware is concerned, sure, take it as an Acked-by and go forward with the spec and kernel side of things. Kevin
Il 04/07/2012 14:50, Kevin Wolf ha scritto: > Before merging the patches (or actually this patch, I think patch 1 is > fairly independent), Yes, it is. > I'd like to hear more opinions on whether we need > the cache parameter split first Ok, let's discuss it next week on the KVM/QEMU call. Getting the cache parameter split before 1.2 is going to be hard, but who knows. > But as far as the hardware is > concerned, sure, take it as an Acked-by and go forward with the spec and > kernel side of things. Rusty already committed everything, so it's too late! :) (Sorry for the tabs vs. spaces BTW). Paolo
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 280f96d..500e026 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -543,6 +543,19 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev, uint32_t features) return features; } +static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) +{ + VirtIOBlock *s = to_virtio_blk(vdev); + uint32_t features; + + if (!(status & VIRTIO_CONFIG_S_DRIVER_OK)) { + return; + } + + features = vdev->guest_features; + bdrv_set_enable_write_cache(s->bs, !!(features & (1 << VIRTIO_BLK_F_WCE))); +} + static void virtio_blk_save(QEMUFile *f, void *opaque) { VirtIOBlock *s = opaque; @@ -628,6 +641,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, VirtIOBlkConf *blk) s->vdev.get_config = virtio_blk_update_config; s->vdev.set_config = virtio_blk_set_config; s->vdev.get_features = virtio_blk_get_features; + s->vdev.set_status = virtio_blk_set_status; s->vdev.reset = virtio_blk_reset; s->bs = blk->conf.bs; s->conf = &blk->conf;
If the guest does not support flushes, we should run in writethrough mode. The setting is temporary until the next reset, so that for example the BIOS will run in writethrough mode while Linux will run with a writeback cache. VIRTIO_BLK_F_FLUSH has been introduced in Linux 2.6.32 (in 2009) and was backported to RHEL/CentOS 5.6 (in 2010). The Windows drivers have two bugs, which I reported on the Red Hat Bugzilla as bugs 837321 and 837324. With these patches they will suffer a performance hit but gain correctness. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/virtio-blk.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)