All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/5] block: API changes for 2.6
@ 2016-03-14 15:44 Kevin Wolf
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields() Kevin Wolf
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

This series contains those part of my current work that I want to be in
the 2.6 release because they change the semantics of some QMP
interfaces.

This is especially true for the WCE changes, which concern things that
weren't even accessible in 2.5 yet, so we're still completely free to
change them until 2.6. The other two parts have existed for a bit
longer, but the changes can reasonably be considered bug fixes.

Kevin Wolf (5):
  block: Remove copy-on-read from bdrv_move_feature_fields()
  block: Remove dirty bitmaps from bdrv_move_feature_fields()
  block: Remove cache.writeback from blockdev-add
  block: Make backing files always writeback
  block: Reject writethrough mode except at the root

 block.c                    |  17 ++++---
 blockdev.c                 |  26 +++++++++--
 qapi/block-core.json       |   4 +-
 tests/qemu-iotests/142     |  50 ++++++++++-----------
 tests/qemu-iotests/142.out | 108 ++++++++++-----------------------------------
 5 files changed, 82 insertions(+), 123 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields()
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
@ 2016-03-14 15:44 ` Kevin Wolf
  2016-03-14 15:51   ` Eric Blake
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps " Kevin Wolf
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

Ever since we first introduced bdrv_append() in commit 8802d1fd ('qapi:
Introduce blockdev-group-snapshot-sync command'), the copy-on-read flag
was moved to the new top layer when taking a snapshot. The only problem
is that it doesn't make a whole lot of sense.

The use case for manually enabled CoR is to avoid reading data twice
from a slow remote image, so we want to save it to a local overlay, say
an ISO image accessed via HTTP to a local qcow2 overlay. When taking a
snapshot, we end up with a backing chain like this:

    http <- local.qcow2 <- snap_overlay.qcow2

There is no point in doing CoR from local.qcow2 into snap_overlay.qcow2,
we just want to keep copying data from the remote source into
local.qcow2.

The other use case of CoR is in the context of streaming, which isn't
very interesting for bdrv_move_feature_fields() because op blockers
prevent this combination.

This patch makes the copy-on-read flag stay on the image for which it
was originally set and prevents it from being propagated to the new
overlay. It is no longer intended to move CoR to the BlockBackend level.
In order for this to make sense, we also need to keep the respective
image read-write.

As a side effect of these changes, creating a live snapshot image (as
opposed to using an existing externally created one) on top of a COR
block device works now. It used to fail because it tried to open its
backing file both read-only and with COR.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c    | 2 --
 blockdev.c | 7 +++++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index cf5eb34..a75c4b3 100644
--- a/block.c
+++ b/block.c
@@ -2283,8 +2283,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
     /* move some fields that need to stay attached to the device */
 
     /* dev info */
-    bs_dest->copy_on_read       = bs_src->copy_on_read;
-
     bs_dest->enable_write_cache = bs_src->enable_write_cache;
 
     /* dirty bitmap */
diff --git a/blockdev.c b/blockdev.c
index 322ca03..a22b444 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1728,6 +1728,7 @@ static void external_snapshot_prepare(BlkActionState *common,
         }
 
         flags = state->old_bs->open_flags;
+        flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
 
         /* create new image w/backing file */
         mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
@@ -1798,8 +1799,10 @@ static void external_snapshot_commit(BlkActionState *common)
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
-    bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
-                NULL);
+    if (!state->old_bs->copy_on_read) {
+        bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR,
+                    NULL);
+    }
 }
 
 static void external_snapshot_abort(BlkActionState *common)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps from bdrv_move_feature_fields()
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields() Kevin Wolf
@ 2016-03-14 15:44 ` Kevin Wolf
  2016-03-14 15:55   ` Eric Blake
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add Kevin Wolf
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

This patch changes dirty bitmaps from following a BlockBackend in graph
changes to sticking with the node they were created at. For the full
discussion, read the following mailing list thread:

  [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields()
  https://lists.nongnu.org/archive/html/qemu-block/2016-02/msg00745.html

In summary, the justification for this change is:

* When moving the dirty bitmap to the top of the tree was introduced in
  bdrv_append() in commit a9fc4408, it didn't actually have any effect
  because there could never be a bitmap in use when bdrv_append() was
  called (op blockers would prevent this). This is still true today for
  all internal uses of dirty bitmaps.

* Support for user-defined dirty bitmaps was introduced in 2.4, but we
  discouraged users from using it because we didn't consider it ready
  yet.

  Moreover, in 2.5, the bdrv_swap() removal introduced a bug that left
  dangling pointers if a dirty bitmap was present (the anchors of the
  dirty bitmap were swapped, but the back link in the first element
  wasn't updated), so it didn't even work correctly.

* block-dirty-bitmap-add takes an arbitrary node name, even if no
  BlockBackend is attached. This suggests that it is a node level
  operation and not a BlockBackend one. Consequently, there is no reason
  for dirty bitmaps to stay with a BlockBackend that was attached to the
  node they were created for.

* It was suggested that block-dirty-bitmap-add could track the node if a
  node name was specified, and track the BlockBackend if the device name
  was specified. This would however be inconsistent with other QMP
  commands. Commands that accept both device and node names currently
  interpret the device name just as an alias for the current root node
  of that BlockBackend.

* Dirty bitmaps have a name that is only unique amongst the bitmaps in a
  specific node. Moving bitmaps could lead to name clashes. Automatic
  renaming would involve too much magic.

* Persistent bitmaps are stored in a specific node. Moving them around
  automatically might be at least surprising, but it would probably also
  become a real problem because that would have to happen atomically
  without the management tool knowing of the operation.

At the end of the day it seems to be very clear that it was a mistake to
include dirty bitmaps in bdrv_move_feature_fields(). The functionality
of moving bitmaps and/or attaching them to a BlockBackend instead will
probably be needed, but it should be done with a new explicit QMP
command or option.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block.c b/block.c
index a75c4b3..698e2c7 100644
--- a/block.c
+++ b/block.c
@@ -2284,9 +2284,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
 
     /* dev info */
     bs_dest->enable_write_cache = bs_src->enable_write_cache;
-
-    /* dirty bitmap */
-    bs_dest->dirty_bitmaps      = bs_src->dirty_bitmaps;
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields() Kevin Wolf
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps " Kevin Wolf
@ 2016-03-14 15:44 ` Kevin Wolf
  2016-03-14 16:10   ` Eric Blake
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback Kevin Wolf
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

The WCE bit is a frontend property and should not be part of the backend
configuration. This is especially important because the same BDS can be
used by different users with different WCE requirements.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9bf1b22..e3617e2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1614,7 +1614,6 @@
 #
 # Includes cache-related options for block devices
 #
-# @writeback:   #optional enables writeback mode for any caches (default: true)
 # @direct:      #optional enables use of O_DIRECT (bypass the host page cache;
 #               default: false)
 # @no-flush:    #optional ignore any flush requests for the device (default:
@@ -1623,8 +1622,7 @@
 # Since: 1.7
 ##
 { 'struct': 'BlockdevCacheOptions',
-  'data': { '*writeback': 'bool',
-            '*direct': 'bool',
+  'data': { '*direct': 'bool',
             '*no-flush': 'bool' } }
 
 ##
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
                   ` (2 preceding siblings ...)
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add Kevin Wolf
@ 2016-03-14 15:44 ` Kevin Wolf
  2016-03-14 16:46   ` Eric Blake
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root Kevin Wolf
  2016-03-18 17:10 ` [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
  5 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

First of all, we're generally not writing to backing files, but when we
do, it's in the context of block jobs which know very well when to flush
the image.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                    |  5 +++--
 tests/qemu-iotests/142.out | 10 +++++-----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 698e2c7..666fe61 100644
--- a/block.c
+++ b/block.c
@@ -762,8 +762,9 @@ static void bdrv_backing_options(int *child_flags, QDict *child_options,
 {
     int flags = parent_flags;
 
-    /* The cache mode is inherited unmodified for backing files */
-    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_WB);
+    /* The cache mode is inherited unmodified for backing files; except WCE,
+     * which is only applied on the top level (BlockBackend) */
+    qdict_set_default_str(child_options, BDRV_OPT_CACHE_WB, "on");
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
     qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
 
diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
index b555d5a..abe94c3 100644
--- a/tests/qemu-iotests/142.out
+++ b/tests/qemu-iotests/142.out
@@ -62,7 +62,7 @@ cache.direct=on on backing-file
 cache.writeback=off on none0
     Cache mode:       writethrough
     Cache mode:       writeback
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
 
 cache.writeback=off on file
@@ -143,7 +143,7 @@ cache.writeback=off on none0
     Cache mode:       writethrough
     Cache mode:       writethrough
     Cache mode:       writeback
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
 
 cache.writeback=off on file
@@ -302,7 +302,7 @@ cache.direct=on on backing-file
 cache.writeback=off on none0
     Cache mode:       writethrough, direct
     Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
+    Cache mode:       writeback, direct
     Cache mode:       writeback, direct
 
 cache.writeback=off on file
@@ -383,7 +383,7 @@ cache.writeback=off on none0
     Cache mode:       writeback, direct
     Cache mode:       writethrough
     Cache mode:       writeback
-    Cache mode:       writethrough
+    Cache mode:       writeback
     Cache mode:       writeback
 
 cache.writeback=off on file
@@ -718,7 +718,7 @@ cache.direct=on on backing-file
 cache.writeback=off on none0
     Cache mode:       writethrough
     Cache mode:       writeback
-    Cache mode:       writethrough, direct
+    Cache mode:       writeback, direct
     Cache mode:       writeback, direct
 
 cache.writeback=off on file
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
                   ` (3 preceding siblings ...)
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback Kevin Wolf
@ 2016-03-14 15:44 ` Kevin Wolf
  2016-03-14 16:50   ` Eric Blake
  2016-03-18 17:10 ` [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
  5 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 15:44 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, qemu-devel, armbru, mreitz, jsnow

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 <kwolf@redhat.com>
---
 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(&bs->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(&state->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 "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.direct=on | grep -e "Cache" -e "[Cc]annot|[Cc]ould not|[Cc]an't"
     echo -e "\ncache.direct=on on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.direct=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.direct=on | grep -e "Cache" -e "[Cc]annot|[Cc]ould not|[Cc]an't"
 
     # cache.writeback is supposed to be inherited by bs->backing; bs->file
     # always gets cache.writeback=on
 
     echo -e "\n\ncache.writeback=off on none0"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.writeback=off | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.writeback=off | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on file"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.writeback=off | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.writeback=off | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on backing"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.writeback=off | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.writeback=off | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.writeback=off | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.writeback=off | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
 
     # cache.no-flush is supposed to be inherited by both bs->file and bs->backing
 
     echo -e "\n\ncache.no-flush=on on none0"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.no-flush=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.no-flush=on | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on file"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.no-flush=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.no-flush=on | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on backing"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.no-flush=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.no-flush=on | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.no-flush=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.file.cache.no-flush=on | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
 }
 
 echo
@@ -218,7 +218,7 @@ info block image
 info block blkdebug
 info block file"
 
-echo "$hmp_cmds" | run_qemu -drive if=none,file="blkdebug::json:{\"filename\":\"$TEST_IMG\",,\"cache\":{\"writeback\":false,,\"direct\":false}}",node-name=image,file.node-name=blkdebug,file.image.node-name=file | grep "Cache"
+echo "$hmp_cmds" | run_qemu -drive if=none,file="blkdebug::json:{\"filename\":\"$TEST_IMG\",,\"cache\":{\"direct\":false}}",node-name=image,file.node-name=blkdebug,file.image.node-name=file | grep "Cache"
 
 echo
 echo "=== Check that referenced BDSes don't inherit ==="
@@ -234,35 +234,35 @@ function check_cache_all_separate()
     # Check cache.direct
 
     echo -e "cache.direct=on on blk"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",cache.direct=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",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 "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.direct=on -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.direct=on -drive "$drv_img" | 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 "$drv_bkfile" -drive "$drv_bk",cache.direct=on -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk",cache.direct=on -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.direct=on on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.direct=on -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.direct=on -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
 
     # Check cache.writeback
 
     echo -e "\n\ncache.writeback=off on blk"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",cache.writeback=off | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",cache.writeback=off | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on file"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.writeback=off -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.writeback=off -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on backing"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk",cache.writeback=off -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk",cache.writeback=off -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.writeback=off on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.writeback=off -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.writeback=off -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
 
     # Check cache.no-flush
 
     echo -e "\n\ncache.no-flush=on on blk"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",cache.no-flush=on | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img",cache.no-flush=on | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on file"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.no-flush=on -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk" -drive "$drv_file",cache.no-flush=on -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on backing"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk",cache.no-flush=on -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile" -drive "$drv_bk",cache.no-flush=on -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
     echo -e "\ncache.no-flush=on on backing-file"
-    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.no-flush=on -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep "Cache"
+    echo "$hmp_cmds" | run_qemu -drive "$drv_bkfile",cache.no-flush=on -drive "$drv_bk" -drive "$drv_file" -drive "$drv_img" | grep -e "Cache" -e "[Cc]annot\|[Cc]ould not\|[Cc]an't"
 }
 
 echo
diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out
index abe94c3..5dd5bd0 100644
--- a/tests/qemu-iotests/142.out
+++ b/tests/qemu-iotests/142.out
@@ -66,22 +66,13 @@ cache.writeback=off on none0
     Cache mode:       writeback
 
 cache.writeback=off on file
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
@@ -147,25 +138,13 @@ cache.writeback=off on none0
     Cache mode:       writeback
 
 cache.writeback=off on file
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
@@ -230,22 +209,13 @@ cache.writeback=off on none0
     Cache mode:       writeback, direct
 
 cache.writeback=off on file
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
@@ -306,22 +276,13 @@ cache.writeback=off on none0
     Cache mode:       writeback, direct
 
 cache.writeback=off on file
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
@@ -387,25 +348,13 @@ cache.writeback=off on none0
     Cache mode:       writeback
 
 cache.writeback=off on file
-    Cache mode:       writeback, direct
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback, direct
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback, direct
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
@@ -440,7 +389,7 @@ cache.no-flush=on on backing-file
 
     Cache mode:       writethrough, direct, ignore flushes
     Cache mode:       writeback, direct, ignore flushes
-    Cache mode:       writethrough, ignore flushes
+    Cache mode:       writeback, ignore flushes
 
 === Check that referenced BDSes don't inherit ===
 
@@ -722,22 +671,13 @@ cache.writeback=off on none0
     Cache mode:       writeback, direct
 
 cache.writeback=off on file
-    Cache mode:       writeback
-    Cache mode:       writethrough
-    Cache mode:       writeback, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,file.cache.writeback=off: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writethrough, direct
-    Cache mode:       writeback, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 cache.writeback=off on backing-file
-    Cache mode:       writeback
-    Cache mode:       writeback
-    Cache mode:       writeback, direct
-    Cache mode:       writethrough, direct
+QEMU_PROG: -drive if=none,file=TEST_DIR/t.qcow2,backing.file.filename=TEST_DIR/t.qcow2.base,node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file,backing.file.cache.writeback=off: Could not open backing file: Can't set writethrough mode except for the root
 
 
 cache.no-flush=on on none0
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields()
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields() Kevin Wolf
@ 2016-03-14 15:51   ` Eric Blake
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2016-03-14 15:51 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

[-- Attachment #1: Type: text/plain, Size: 1924 bytes --]

On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> Ever since we first introduced bdrv_append() in commit 8802d1fd ('qapi:
> Introduce blockdev-group-snapshot-sync command'), the copy-on-read flag
> was moved to the new top layer when taking a snapshot. The only problem
> is that it doesn't make a whole lot of sense.
> 
> The use case for manually enabled CoR is to avoid reading data twice
> from a slow remote image, so we want to save it to a local overlay, say
> an ISO image accessed via HTTP to a local qcow2 overlay. When taking a
> snapshot, we end up with a backing chain like this:
> 
>     http <- local.qcow2 <- snap_overlay.qcow2
> 
> There is no point in doing CoR from local.qcow2 into snap_overlay.qcow2,
> we just want to keep copying data from the remote source into
> local.qcow2.
> 
> The other use case of CoR is in the context of streaming, which isn't
> very interesting for bdrv_move_feature_fields() because op blockers
> prevent this combination.

Your arguments make sense.

> 
> This patch makes the copy-on-read flag stay on the image for which it
> was originally set and prevents it from being propagated to the new
> overlay. It is no longer intended to move CoR to the BlockBackend level.
> In order for this to make sense, we also need to keep the respective
> image read-write.
> 
> As a side effect of these changes, creating a live snapshot image (as
> opposed to using an existing externally created one) on top of a COR
> block device works now. It used to fail because it tried to open its
> backing file both read-only and with COR.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c    | 2 --
>  blockdev.c | 7 +++++--
>  2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps from bdrv_move_feature_fields()
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps " Kevin Wolf
@ 2016-03-14 15:55   ` Eric Blake
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2016-03-14 15:55 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> This patch changes dirty bitmaps from following a BlockBackend in graph
> changes to sticking with the node they were created at. For the full
> discussion, read the following mailing list thread:
> 
>   [Qemu-block] block: Dirty bitmaps and COR in bdrv_move_feature_fields()
>   https://lists.nongnu.org/archive/html/qemu-block/2016-02/msg00745.html
> 
> In summary, the justification for this change is:
[snip nice list]
> 
> At the end of the day it seems to be very clear that it was a mistake to
> include dirty bitmaps in bdrv_move_feature_fields(). The functionality
> of moving bitmaps and/or attaching them to a BlockBackend instead will
> probably be needed, but it should be done with a new explicit QMP
> command or option.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

[Always interesting when a patch is way shorter than the justification
for why we need the patch]

> 
> diff --git a/block.c b/block.c
> index a75c4b3..698e2c7 100644
> --- a/block.c
> +++ b/block.c
> @@ -2284,9 +2284,6 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
>  
>      /* dev info */
>      bs_dest->enable_write_cache = bs_src->enable_write_cache;
> -
> -    /* dirty bitmap */
> -    bs_dest->dirty_bitmaps      = bs_src->dirty_bitmaps;
>  }
>  
>  static void change_parent_backing_link(BlockDriverState *from,
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add Kevin Wolf
@ 2016-03-14 16:10   ` Eric Blake
  2016-03-14 16:22     ` Kevin Wolf
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Blake @ 2016-03-14 16:10 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> The WCE bit is a frontend property and should not be part of the backend
> configuration. This is especially important because the same BDS can be
> used by different users with different WCE requirements.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9bf1b22..e3617e2 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1614,7 +1614,6 @@
>  #
>  # Includes cache-related options for block devices
>  #
> -# @writeback:   #optional enables writeback mode for any caches (default: true)
>  # @direct:      #optional enables use of O_DIRECT (bypass the host page cache;
>  #               default: false)
>  # @no-flush:    #optional ignore any flush requests for the device (default:
> @@ -1623,8 +1622,7 @@
>  # Since: 1.7
>  ##
>  { 'struct': 'BlockdevCacheOptions',
> -  'data': { '*writeback': 'bool',
> -            '*direct': 'bool',
> +  'data': { '*direct': 'bool',
>              '*no-flush': 'bool' } }

Observable through introspection.  Not quite backwards-compatible, but
at least clients can learn about it, and arguably clients shouldn't have
been using it.  I can accept it as a bug fix, even though it does risk
breaking old clients that were trying to use it.

If it helps, libvirt does not seem to have been using it.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add
  2016-03-14 16:10   ` Eric Blake
@ 2016-03-14 16:22     ` Kevin Wolf
  0 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2016-03-14 16:22 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, jsnow, armbru, qemu-block, mreitz

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

Am 14.03.2016 um 17:10 hat Eric Blake geschrieben:
> On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> > The WCE bit is a frontend property and should not be part of the backend
> > configuration. This is especially important because the same BDS can be
> > used by different users with different WCE requirements.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/block-core.json | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 9bf1b22..e3617e2 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1614,7 +1614,6 @@
> >  #
> >  # Includes cache-related options for block devices
> >  #
> > -# @writeback:   #optional enables writeback mode for any caches (default: true)
> >  # @direct:      #optional enables use of O_DIRECT (bypass the host page cache;
> >  #               default: false)
> >  # @no-flush:    #optional ignore any flush requests for the device (default:
> > @@ -1623,8 +1622,7 @@
> >  # Since: 1.7
> >  ##
> >  { 'struct': 'BlockdevCacheOptions',
> > -  'data': { '*writeback': 'bool',
> > -            '*direct': 'bool',
> > +  'data': { '*direct': 'bool',
> >              '*no-flush': 'bool' } }
> 
> Observable through introspection.  Not quite backwards-compatible, but
> at least clients can learn about it, and arguably clients shouldn't have
> been using it.  I can accept it as a bug fix, even though it does risk
> breaking old clients that were trying to use it.
> 
> If it helps, libvirt does not seem to have been using it.

I think we declared that blockdev-add is still experimental, so it
should be okay.

And I was actually pondering whether this struct even makes sense any
more or whether we should move 'direct' and 'no-flush' directly to the
parent struct.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback Kevin Wolf
@ 2016-03-14 16:46   ` Eric Blake
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2016-03-14 16:46 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On 03/14/2016 09:44 AM, Kevin Wolf wrote:
> First of all, we're generally not writing to backing files, but when we
> do, it's in the context of block jobs which know very well when to flush
> the image.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                    |  5 +++--
>  tests/qemu-iotests/142.out | 10 +++++-----
>  2 files changed, 8 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root Kevin Wolf
@ 2016-03-14 16:50   ` Eric Blake
  0 siblings, 0 replies; 13+ messages in thread
From: Eric Blake @ 2016-03-14 16:50 UTC (permalink / raw)
  To: Kevin Wolf, qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

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 <kwolf@redhat.com>
> ---
>  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 <eblake@redhat.com>

> +++ 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH 0/5] block: API changes for 2.6
  2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
                   ` (4 preceding siblings ...)
  2016-03-14 15:44 ` [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root Kevin Wolf
@ 2016-03-18 17:10 ` Kevin Wolf
  5 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2016-03-18 17:10 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, jsnow, armbru, mreitz

Am 14.03.2016 um 16:44 hat Kevin Wolf geschrieben:
> This series contains those part of my current work that I want to be in
> the 2.6 release because they change the semantics of some QMP
> interfaces.
> 
> This is especially true for the WCE changes, which concern things that
> weren't even accessible in 2.5 yet, so we're still completely free to
> change them until 2.6. The other two parts have existed for a bit
> longer, but the changes can reasonably be considered bug fixes.

Applied to the block branch.

Kevin

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-03-18 17:10 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-14 15:44 [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf
2016-03-14 15:44 ` [Qemu-devel] [PATCH 1/5] block: Remove copy-on-read from bdrv_move_feature_fields() Kevin Wolf
2016-03-14 15:51   ` Eric Blake
2016-03-14 15:44 ` [Qemu-devel] [PATCH 2/5] block: Remove dirty bitmaps " Kevin Wolf
2016-03-14 15:55   ` Eric Blake
2016-03-14 15:44 ` [Qemu-devel] [PATCH 3/5] block: Remove cache.writeback from blockdev-add Kevin Wolf
2016-03-14 16:10   ` Eric Blake
2016-03-14 16:22     ` Kevin Wolf
2016-03-14 15:44 ` [Qemu-devel] [PATCH 4/5] block: Make backing files always writeback Kevin Wolf
2016-03-14 16:46   ` Eric Blake
2016-03-14 15:44 ` [Qemu-devel] [PATCH 5/5] block: Reject writethrough mode except at the root Kevin Wolf
2016-03-14 16:50   ` Eric Blake
2016-03-18 17:10 ` [Qemu-devel] [PATCH 0/5] block: API changes for 2.6 Kevin Wolf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.