Re: [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root

[Eric Blake]

On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> Writethrough mode is going to become a BlockBackend feature rather than
> a BDS one, so forbid it in places where we won't be able to support it
> when the code finally matches the envisioned design.
> 
> We only allowed setting the cache mode of non-root nodes after the 2.5
> release, so we're still free to make this change.
> 
> The target of block jobs is now always opened in a writeback mode
> because it doesn't have a BlockBackend attached. This makes more sense
> anyway because block jobs know when to flush. If the graph is modified
> on job completion, the original cache mode moves to the new root, so
> for the guest device writethough always stays enabled if it was
> configured this way.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c|  7 
>  blockdev.c | 19 -
>  tests/qemu-iotests/142 | 50 +++
>  tests/qemu-iotests/142.out | 98 
> +-
>  4 files changed, 68 insertions(+), 106 deletions(-)
> 

Reviewed-by: Eric Blake 

> +++ b/tests/qemu-iotests/142
> @@ -96,36 +96,36 @@ function check_cache_all()
>  # bs->backing
>  
>  echo -e "cache.direct=on on none0"

Pre-existing, but 'echo -e' is non-portable (even in bash, you can set
options such that it changes behavior).  printf is better, if we're
worried about it.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root

[Kevin Wolf]

Writethrough mode is going to become a BlockBackend feature rather than
a BDS one, so forbid it in places where we won't be able to support it
when the code finally matches the envisioned design.

We only allowed setting the cache mode of non-root nodes after the 2.5
release, so we're still free to make this change.

The target of block jobs is now always opened in a writeback mode
because it doesn't have a BlockBackend attached. This makes more sense
anyway because block jobs know when to flush. If the graph is modified
on job completion, the original cache mode moves to the new root, so
for the guest device writethough always stays enabled if it was
configured this way.

Signed-off-by: Kevin Wolf 
---
 block.c|  7 
 blockdev.c | 19 -
 tests/qemu-iotests/142 | 50 +++
 tests/qemu-iotests/142.out | 98 +-
 4 files changed, 68 insertions(+), 106 deletions(-)

diff --git a/block.c b/block.c
index 666fe61..94b8894 100644
--- a/block.c
+++ b/block.c
@@ -999,6 +999,13 @@ static int bdrv_open_common(BlockDriverState *bs, 
BdrvChild *file,
 
 /* Apply cache mode options */
 update_flags_from_options(>open_flags, opts);
+
+if (!bs->blk && (bs->open_flags & BDRV_O_CACHE_WB) == 0) {
+error_setg(errp, "Can't set writethrough mode except for the root");
+ret = -EINVAL;
+goto free_and_fail;
+}
+
 bdrv_set_enable_write_cache(bs, bs->open_flags & BDRV_O_CACHE_WB);
 
 /* Open the image, either directly or using a protocol */
diff --git a/blockdev.c b/blockdev.c
index a22b444..9f36087 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1758,6 +1758,12 @@ static void external_snapshot_prepare(BlkActionState 
*common,
 flags |= BDRV_O_NO_BACKING;
 }
 
+/* There is no BB attached during bdrv_open(), so we can't set a
+ * writethrough mode. bdrv_append() will swap the WCE setting so that the
+ * backing file becomes unconditionally writeback (which is what backing
+ * files should always be) and the new overlay gets the original setting. 
*/
+flags |= BDRV_O_CACHE_WB;
+
 assert(state->new_bs == NULL);
 ret = bdrv_open(>new_bs, new_image_file, snapshot_ref, options,
 flags, errp);
@@ -2509,6 +2515,7 @@ void qmp_blockdev_change_medium(const char *device, const 
char *filename,
 BlockBackend *blk;
 BlockDriverState *medium_bs = NULL;
 int bdrv_flags, ret;
+bool writethrough;
 QDict *options = NULL;
 Error *err = NULL;
 
@@ -2527,6 +2534,12 @@ void qmp_blockdev_change_medium(const char *device, 
const char *filename,
 bdrv_flags &= ~(BDRV_O_TEMPORARY | BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING |
 BDRV_O_PROTOCOL);
 
+/* Must open the image in writeback mode as long as no BlockBackend is
+ * attached. The right mode can be set as the final step after changing the
+ * medium. */
+writethrough = !(bdrv_flags & BDRV_O_CACHE_WB);
+bdrv_flags |= BDRV_O_CACHE_WB;
+
 if (!has_read_only) {
 read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
 }
@@ -2584,6 +2597,8 @@ void qmp_blockdev_change_medium(const char *device, const 
char *filename,
 goto fail;
 }
 
+bdrv_set_enable_write_cache(medium_bs, !writethrough);
+
 qmp_blockdev_close_tray(device, errp);
 
 fail:
@@ -3209,7 +3224,7 @@ static void do_drive_backup(const char *device, const 
char *target,
 goto out;
 }
 
-flags = bs->open_flags | BDRV_O_RDWR;
+flags = bs->open_flags | BDRV_O_CACHE_WB | BDRV_O_RDWR;
 
 /* See if we have a backing HD we can use to create our new image
  * on top of. */
@@ -3504,7 +3519,7 @@ void qmp_drive_mirror(const char *device, const char 
*target,
 format = mode == NEW_IMAGE_MODE_EXISTING ? NULL : bs->drv->format_name;
 }
 
-flags = bs->open_flags | BDRV_O_RDWR;
+flags = bs->open_flags | BDRV_O_CACHE_WB | BDRV_O_RDWR;
 source = backing_bs(bs);
 if (!source && sync == MIRROR_SYNC_MODE_TOP) {
 sync = MIRROR_SYNC_MODE_FULL;
diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
index 8aa50f8..80834b5 100755
--- a/tests/qemu-iotests/142
+++ b/tests/qemu-iotests/142
@@ -96,36 +96,36 @@ function check_cache_all()
 # bs->backing
 
 echo -e "cache.direct=on on none0"
-echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.direct=on | grep 
"Cache"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.direct=on | grep 
-e "Cache" -e "[Cc]annot|[Cc]ould not|[Cc]an't"
 echo -e "\ncache.direct=on on file"
-echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.direct=on | 
grep "Cache"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.direct=on | 
grep -e "Cache" -e "[Cc]annot|[Cc]ould not|[Cc]an't"
 echo -e "\ncache.direct=on on backing"
-echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.direct=on 
| grep