allow guest control of the volatile write cache

Change BDRV_O_NOCACHE to only imply bypassing the host OS file cache, but no writeback semantics.  All existing callers are changed to also specify BDRV_O_CACHE_WB to give them writeback semantics.

Signed-off-by Christoph Hellwig <[email protected]>

Quoted 5 lines

Change BDRV_O_NOCACHE to only imply bypassing the host OS file cache, but no writeback semantics. All existing callers are changed to also specify BDRV_O_CACHE_WB to give them writeback semantics.

Signed-off-by Christoph Hellwig <[email protected]>

I think there is one hunk missing:

block/qcow2.c +0 -0

index 75b8bec..db1931b 100644

Quoted 2 lines · Stefan Hajnoczi

-    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
+    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);

or rather

       writethrough = ((flags & (BDRV_O_CACHE_WB) != );

but yes, this code had sneaked in since my initial version.

Quoted 6 lines · Stefan Hajnoczi

block/qcow2.c static int qcow2_open(BlockDriverState *bs, int flags) · line 232

Quoted 2 lines

block/qcow2.c +1 -1

-    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
+    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);

or rather

Quoted 3 lines

       writethrough = ((flags & (BDRV_O_CACHE_WB) != );

but yes, this code had sneaked in since my initial version.

My intention was that if we don't care about honoring flushes then we might as well use Qcow2Cache. But yes, just checking for cache mode is the clearest.

Am 16.03.2011 18:00, schrieb Stefan Hajnoczi:

Quoted 9 lines · Christoph Hellwig

Quoted 8 lines · Stefan Hajnoczi

Quoted 2 lines

-    writethrough = ((flags & BDRV_O_CACHE_MASK) == 0);
+    writethrough = ((flags & (BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)) == 0);

or rather

       writethrough = ((flags & (BDRV_O_CACHE_WB) != );

Quoted 5 lines

Quoted 1 line

but yes, this code had sneaked in since my initial version.

My intention was that if we don't care about honoring flushes then we might as well use Qcow2Cache.  But yes, just checking for cache mode is the clearest.

You mean for a possible writethrough mode with BDRV_O_NO_FLUSH set? Such a mode doesn't make a lot of sense to me.

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

Index: qemu/block.c
===================================================================

block.c        2011-03-15 14:57:03.680633093 +0100 renamed from block.c   2011-03-15 11:47:31.285634626 +0100 +7 -0

@@ -441,6 +441,8 @@ static int bdrv_open_common(BlockDriverS
 
     if (flags & BDRV_O_CACHE_WB)
         bs->enable_write_cache = 1;
+    else
+        bs->enable_write_cache = 0;
 
     /*
      * Clear flags that are internal to the block layer before opening the

@@ -651,6 +653,44 @@ unlink_and_fail:
     return ret;
  }
 
+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+

Quoted 30 lines

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

Index: qemu/block.c
===================================================================

block.c 2011-03-15 14:57:03.680633093 +0100 renamed from block.c 2011-03-15 11:47:31.285634626 +0100 +7 -0

@@ -441,6 +441,8 @@ static int bdrv_open_common(BlockDriverS
 
    if (flags & BDRV_O_CACHE_WB)
        bs->enable_write_cache = 1;
+    else
+        bs->enable_write_cache = 0;
 
    /*
     * Clear flags that are internal to the block layer before opening the

@@ -651,6 +653,44 @@ unlink_and_fail:
    return ret;
 }
 
+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+

block.c 2011-03-15 14:57:03.680633093 +0100 unlink_and_fail: · line 670

Quoted 10 lines

block.c 2011-03-15 14:57:03.680633093 +0100 renamed from block.c 2011-03-15 11:47:31.285634626 +0100 +10 -0

+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);

This will fail for -snapshot disks since the on-disk file is deleted.

In general it would be more robust to keep the original file descriptor around in case the file cannot be opened with the new flags.

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

block.c +27 -0

+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+     */
+    if (ret != 0) {
+        abort();
+    }
+
+    return ret;
+}
 
 
 
+
+int bdrv_change_cache(BlockDriverState *bs, bool enable)

Quoted 39 lines

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

block.c +27 -0

+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+     */
+    if (ret != 0) {
+        abort();
+    }
+
+    return ret;
+}
 
 
 
+
+int bdrv_change_cache(BlockDriverState *bs, bool enable)

block.c unlink_and_fail: · line 692

Quoted 10 lines

block.c +10 -0

+{
+    int bdrv_flags = 0;
+
+    bdrv_flags = bs->open_flags & ~BDRV_O_CACHE_WB;
+    if (enable) {
+        bdrv_flags |= BDRV_O_CACHE_WB;
+    }
+
+    return bdrv_reopen(bs, bdrv_flags);
+}

Is there any way we can manage todo this *without* closing and re-opening the file descriptor ?

One of the things we're considering for the future is to enable QEMU to be passed open FD(s) for its drives, from the management system, instead of having QEMU open the files itself.

If QEMU expects to close+reopen any of its disks at any time the guest requests, this will complicate life somewhat

Am 17.03.2011 15:44, schrieb Daniel P. Berrange:

Quoted 54 lines · Christoph Hellwig

Quoted 39 lines

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

block.c +27 -0

+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+     */
+    if (ret != 0) {
+        abort();
+    }
+
+    return ret;
+}
 
 
 
+
+int bdrv_change_cache(BlockDriverState *bs, bool enable)

block.c unlink_and_fail: · line 692

Quoted 10 lines

block.c +10 -0

+{
+    int bdrv_flags = 0;
+
+    bdrv_flags = bs->open_flags & ~BDRV_O_CACHE_WB;
+    if (enable) {
+        bdrv_flags |= BDRV_O_CACHE_WB;
+    }
+
+    return bdrv_reopen(bs, bdrv_flags);
+}

Is there any way we can manage todo this *without* closing and re-opening the file descriptor ?

Quoted 3 lines

One of the things we're considering for the future is to enable QEMU to be passed open FD(s) for its drives, from the management system, instead of having QEMU open the files itself.

What about cache mode, read-only flags etc. that are set with the right flags during the open? Must qemu just assume that the management has done the right thing?

And what about things like backing files or other information that depends on the content of the image file?

Quoted 2 lines

If QEMU expects to close+reopen any of its disks at any time the guest requests, this will complicate life somewhat

It does expect this. For example for making backing files temporarily read-write during a 'commit' monitor command, we already reopen the image. (Let's hope nobody uses -snapshot, live snapshots and commit, he would lose his disk...)

Quoted 66 lines · Kevin Wolf

Am 17.03.2011 15:44, schrieb Daniel P. Berrange:

Quoted 54 lines · Christoph Hellwig

Quoted 39 lines

Add a new bdrv_change_cache that can set/clear the writeback flag at runtime by stopping all I/O and closing/reopening the image file.

All code is based on a patch from Prerna Saxena <[email protected]> with minimal refactoring.

Signed-off-by Christoph Hellwig <[email protected]>

block.c +27 -0

+static int bdrv_reopen(BlockDriverState *bs, int bdrv_flags)
+{
+    BlockDriver *drv = bs->drv;
+    int ret;
+
+    if (bdrv_flags == bs->open_flags) {
+        return 0;
+    }
+
+    /* Quiesce IO for the given block device */
+    qemu_aio_flush();
+    bdrv_flush(bs);
+
+    bdrv_close(bs);
+    ret = bdrv_open(bs, bs->filename, bdrv_flags, drv);
+
+    /*
+     * A failed attempt to reopen the image file must lead to 'abort()'
+     */
+    if (ret != 0) {
+        abort();
+    }
+
+    return ret;
+}
 
 
 
+
+int bdrv_change_cache(BlockDriverState *bs, bool enable)

block.c unlink_and_fail: · line 692

Quoted 10 lines

block.c +10 -0

+{
+    int bdrv_flags = 0;
+
+    bdrv_flags = bs->open_flags & ~BDRV_O_CACHE_WB;
+    if (enable) {
+        bdrv_flags |= BDRV_O_CACHE_WB;
+    }
+
+    return bdrv_reopen(bs, bdrv_flags);
+}

Is there any way we can manage todo this *without* closing and re-opening the file descriptor ?

Quoted 3 lines

One of the things we're considering for the future is to enable QEMU to be passed open FD(s) for its drives, from the management system, instead of having QEMU open the files itself.

What about cache mode, read-only flags etc. that are set with the right flags during the open? Must qemu just assume that the management has done the right thing?

And what about things like backing files or other information that depends on the content of the image file?

Quoted 7 lines

Quoted 2 lines

If QEMU expects to close+reopen any of its disks at any time the guest requests, this will complicate life somewhat

It does expect this. For example for making backing files temporarily read-write during a 'commit' monitor command, we already reopen the image. (Let's hope nobody uses -snapshot, live snapshots and commit, he would lose his disk...)

I think we need to use the most robust solution possible.  We don't want to get into a situation that can be avoided.  Especially in cases where human errors can (==will) be made.

I suggested using /proc/$pid/fd to reopen an existing file descriptor. That interface is only available on Linux and possibly some of {Solaris, *BSD, OSX}.  So there needs to be a fallback, but in this situation it feels right to take advantage of whatever means are provided by the host OS to make safe transitions between image files.

Add support for the new dynamic features config space field to allow en/disabling the write cache at runtime. The userspace interface is a SCSI-compatible sysfs attribute.

Signed-off-by Christoph Hellwig <[email protected]>

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 337

Quoted 3 lines

drivers/block/virtio_blk.c +3 -0

+	if (strncmp(buf, "write through", sizeof("write through") - 1) == 0) {
+		;
+	} else if (strncmp(buf, "write back", sizeof("write back") - 1) == 0) {

Is there a reason we're not letting gcc and/or strcmp do the
optimization work here?

Quoted 4 lines

drivers/block/virtio_blk.c +4 -0

+	vdev->config->set(vdev, offsetof(struct virtio_blk_config, features),
+	                  &features, sizeof(features));
+
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, features),

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 352

Quoted 5 lines

drivers/block/virtio_blk.c +5 -0

+			  &features2, sizeof(features2));
+
+	if ((features & VIRTIO_BLK_RT_WCE) !=
+	    (features2 & VIRTIO_BLK_RT_WCE))
+		return -EIO;

This seems like a debugging check you left in. Or do you suspect
some issues?

Quoted 6 lines · Rusty Russell

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 337

Quoted 3 lines

drivers/block/virtio_blk.c +3 -0

+	if (strncmp(buf, "write through", sizeof("write through") - 1) == 0) {
+		;
+	} else if (strncmp(buf, "write back", sizeof("write back") - 1) == 0) {

Is there a reason we're not letting gcc and/or strcmp do the
optimization work here?

I'm happ to switch strcmp.

Quoted 4 lines

Quoted 4 lines

drivers/block/virtio_blk.c +4 -0

+	vdev->config->set(vdev, offsetof(struct virtio_blk_config, features),
+	                  &features, sizeof(features));
+
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, features),

Quoted 8 lines

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 352

Quoted 5 lines

drivers/block/virtio_blk.c +5 -0

+			  &features2, sizeof(features2));
+
+	if ((features & VIRTIO_BLK_RT_WCE) !=
+	    (features2 & VIRTIO_BLK_RT_WCE))
+		return -EIO;

This seems like a debugging check you left in. Or do you suspect
some issues?

No, it's intentional. config space writes can't return errors, so we need to check that the value has really changed. I'll add a comment explaining it.

Quoted 1 line

I'm happ to switch strcmp.

Of course, that's assuming buf is nul terminated.

Quoted 2 lines

No, it's intentional. config space writes can't return errors, so we need to check that the value has really changed. I'll add a comment explaining it.

OK, under what circumstances could it fail?

If you're using this mechanism to indicate that the host doesn't support the feature, that's making an assumption about the nature of config space writes which isn't true for non-PCI virtio.

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should? But we should be explicit about needing it...

Quoted 6 lines · Rusty Russell

OK, under what circumstances could it fail?

If you're using this mechanism to indicate that the host doesn't support the feature, that's making an assumption about the nature of config space writes which isn't true for non-PCI virtio.

Quoted 3 lines

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should? But we should be explicit about needing it...

We have the features flag to indicate if updating the caching mode is supported, but we we could still fail it for other reasons - e.g. we could fail to reopen the file with/without O_SYNC.

But if lguest or S/390 don't support
trapping config space write this feature won't work for them at all. As do other features that make use of config space write, e.g. updating the MAC address for virtio-net.

Quoted 2 lines · Rusty Russell

Quoted 1 line

I'm happ to switch strcmp.

Of course, that's assuming buf is nul terminated.

It's the string the user writes into it, which normally should be nul-terminated.

Quoted 10 lines

Quoted 2 lines

No, it's intentional. config space writes can't return errors, so we need to check that the value has really changed. I'll add a comment explaining it.

OK, under what circumstances could it fail?

If you're using this mechanism to indicate that the host doesn't support the feature, that's making an assumption about the nature of config space writes which isn't true for non-PCI virtio.

ie. lguest and S/390 don't trap writes to config space.

Quoted 1 line

Or perhaps they should? But we should be explicit about needing it...

We have the features flag to indicate if updating the caching mode is supported, but we we could still fail it for other reasons - e.g. we could fail to reopen the file with/without O_SYNC. But if lguest or S/390 don't support trapping config space write this feature won't work for them at all. As do other features that make use of config space write, e.g. updating the MAC address for virtio-net.

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Rusty Russell

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 337

Quoted 3 lines

drivers/block/virtio_blk.c +3 -0

+	if (strncmp(buf, "write through", sizeof("write through") - 1) == 0) {
+		;
+	} else if (strncmp(buf, "write back", sizeof("write back") - 1) == 0) {

Is there a reason we're not letting gcc and/or strcmp do the
optimization work here?

I'm happ to switch strcmp.

Of course, that's assuming buf is nul terminated.

Quoted 11 lines

Quoted 11 lines

drivers/block/virtio_blk.c static ssize_t virtblk_serial_show(struc · line 352

Quoted 9 lines

drivers/block/virtio_blk.c +9 -0

+	vdev->config->set(vdev, offsetof(struct virtio_blk_config, features),
+	&features, sizeof(features));
+
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, features),
+			&features2, sizeof(features2));
+
+	if ((features&  VIRTIO_BLK_RT_WCE) !=
+	    (features2&  VIRTIO_BLK_RT_WCE))
+		return -EIO;

This seems like a debugging check you left in. Or do you suspect
some issues?

Quoted 2 lines

No, it's intentional. config space writes can't return errors, so we need to check that the value has really changed. I'll add a comment explaining it.

OK, under what circumstances could it fail?

If you're using this mechanism to indicate that the host doesn't support the feature, that's making an assumption about the nature of config space writes which isn't true for non-PCI virtio.

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should? But we should be explicit about needing it...

Thanks,
Rusty.

Quoted 3 lines

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should? But we should be explicit about needing it...

I don't think we ever operated on the assumption that config space writes would trap.

I don't think adding it is the right thing either because you can do byte access to the config space which makes atomicity difficult.

Any reason not to use a control queue to negotiate dynamic features?

Am 24.03.2011 04:05, schrieb Anthony Liguori:

Quoted 5 lines

Quoted 3 lines

ie. lguest and S/390 don't trap writes to config space.

Or perhaps they should? But we should be explicit about needing it...

I don't think we ever operated on the assumption that config space writes would trap.

Quoted 1 line

I don't think adding it is the right thing either because you can do byte access to the config space which makes atomicity difficult.

There is the additional problem, that s390 has no MMIO and,therefore, there is no real HW support for trapping writes to an area. You can use page faults, or read-only faults on newer systems, but this is expensive. In addition, page faults only deliver the page frame, but not the offset within a page.