| Message ID | 1458325289-17848-18-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18.03.2016 19:21, Kevin Wolf wrote: > We must forbid changing the WCE flag in bdrv_reopen() in the same patch, > as otherwise the behaviour would change so that the flag takes > precedence over the explicitly specified option. > > The correct value of the WCE flag depends on the BlockBackend user (e.g. > guest device) and isn't a decision that the QMP client makes, so this > change is what we want. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 18 ++++++------------ > qemu-io-cmds.c | 14 +++++++++++++- > tests/qemu-iotests/142 | 2 +- > tests/qemu-iotests/142.out | 2 +- > 4 files changed, 21 insertions(+), 15 deletions(-) > [...] > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > index e929d24..7de3754 100644 > --- a/qemu-io-cmds.c > +++ b/qemu-io-cmds.c [...] > @@ -2136,14 +2137,25 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) > return qemuio_command_usage(&reopen_cmd); > } > > + if (writethrough != blk_enable_write_cache(blk) && > + blk_get_attached_dev(blk)) > + { > + error_report("Cannot change cache.writeback: Device attached"); > + qemu_opts_reset(&reopen_opts); > + return 0; > + } > + > qopts = qemu_opts_find(&reopen_opts, NULL); > opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; > qemu_opts_reset(&reopen_opts); > > + flags |= blk_enable_write_cache(blk) ? BDRV_O_CACHE_WB : 0; Shouldn't this be bdrv_enable_write_cache(bs)? With blk_enable_write_cache(blk), reopening a non-WB BB should always fail because bdrv_reopen_multiple() accuses us of trying to change the WB mode (whereas we really don't want to change the BDS's mode). Max > brq = bdrv_reopen_queue(NULL, bs, opts, flags); > bdrv_reopen_multiple(brq, &local_err); > if (local_err) { > error_report_err(local_err); > + } else { > + blk_set_enable_write_cache(blk, !writethrough); > } > > return 0;
Am 26.03.2016 um 22:05 hat Max Reitz geschrieben: > On 18.03.2016 19:21, Kevin Wolf wrote: > > We must forbid changing the WCE flag in bdrv_reopen() in the same patch, > > as otherwise the behaviour would change so that the flag takes > > precedence over the explicitly specified option. > > > > The correct value of the WCE flag depends on the BlockBackend user (e.g. > > guest device) and isn't a decision that the QMP client makes, so this > > change is what we want. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > block.c | 18 ++++++------------ > > qemu-io-cmds.c | 14 +++++++++++++- > > tests/qemu-iotests/142 | 2 +- > > tests/qemu-iotests/142.out | 2 +- > > 4 files changed, 21 insertions(+), 15 deletions(-) > > > > [...] > > > diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c > > index e929d24..7de3754 100644 > > --- a/qemu-io-cmds.c > > +++ b/qemu-io-cmds.c > > [...] > > > @@ -2136,14 +2137,25 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) > > return qemuio_command_usage(&reopen_cmd); > > } > > > > + if (writethrough != blk_enable_write_cache(blk) && > > + blk_get_attached_dev(blk)) > > + { > > + error_report("Cannot change cache.writeback: Device attached"); > > + qemu_opts_reset(&reopen_opts); > > + return 0; > > + } > > + > > qopts = qemu_opts_find(&reopen_opts, NULL); > > opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; > > qemu_opts_reset(&reopen_opts); > > > > + flags |= blk_enable_write_cache(blk) ? BDRV_O_CACHE_WB : 0; > > Shouldn't this be bdrv_enable_write_cache(bs)? > > With blk_enable_write_cache(blk), reopening a non-WB BB should always > fail because bdrv_reopen_multiple() accuses us of trying to change the > WB mode (whereas we really don't want to change the BDS's mode). bdrv_enable_write_cache() is only a wrapper around blk_enable_write_cache(), so that wouldn't make any difference. It also means that bdrv_reopen() checks the cache mode of the BlockBackend rather than always returning true. This line exists precisely to avoid the error message you mention. And as you noticed, the line goes away anyway when BDRV_O_CACHE_WB is removed later in the series. Kevin
diff --git a/block.c b/block.c index 4e95d01..3775e65 100644 --- a/block.c +++ b/block.c @@ -2028,18 +2028,12 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, update_flags_from_options(&reopen_state->flags, opts); - /* If a guest device is attached, it owns WCE */ - if (reopen_state->bs->blk && blk_get_attached_dev(reopen_state->bs->blk)) { - bool old_wce = bdrv_enable_write_cache(reopen_state->bs); - bool new_wce = (reopen_state->flags & BDRV_O_CACHE_WB); - if (old_wce != new_wce) { - error_setg(errp, "Cannot change cache.writeback: Device attached"); - ret = -EINVAL; - goto error; - } - } - if (!reopen_state->bs->blk && !(reopen_state->flags & BDRV_O_CACHE_WB)) { - error_setg(errp, "Cannot disable cache.writeback: No BlockBackend"); + /* WCE is a BlockBackend level option, can't change it */ + bool old_wce = bdrv_enable_write_cache(reopen_state->bs); + bool new_wce = (reopen_state->flags & BDRV_O_CACHE_WB); + + if (old_wce != new_wce) { + error_setg(errp, "Cannot change cache.writeback"); ret = -EINVAL; goto error; } diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c index e929d24..7de3754 100644 --- a/qemu-io-cmds.c +++ b/qemu-io-cmds.c @@ -2104,6 +2104,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) QDict *opts; int c; int flags = bs->open_flags; + bool writethrough = !blk_enable_write_cache(blk); BlockReopenQueue *brq; Error *local_err = NULL; @@ -2111,7 +2112,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) while ((c = getopt(argc, argv, "c:o:r")) != -1) { switch (c) { case 'c': - if (bdrv_parse_cache_flags(optarg, &flags) < 0) { + if (bdrv_parse_cache_mode(optarg, &flags, &writethrough) < 0) { error_report("Invalid cache option: %s", optarg); return 0; } @@ -2136,14 +2137,25 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) return qemuio_command_usage(&reopen_cmd); } + if (writethrough != blk_enable_write_cache(blk) && + blk_get_attached_dev(blk)) + { + error_report("Cannot change cache.writeback: Device attached"); + qemu_opts_reset(&reopen_opts); + return 0; + } + qopts = qemu_opts_find(&reopen_opts, NULL); opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL; qemu_opts_reset(&reopen_opts); + flags |= blk_enable_write_cache(blk) ? BDRV_O_CACHE_WB : 0; brq = bdrv_reopen_queue(NULL, bs, opts, flags); bdrv_reopen_multiple(brq, &local_err); if (local_err) { error_report_err(local_err); + } else { + blk_set_enable_write_cache(blk, !writethrough); } return 0; diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142 index 8bbbfde..a035747 100755 --- a/tests/qemu-iotests/142 +++ b/tests/qemu-iotests/142 @@ -216,7 +216,7 @@ echo # BDS initialised with the json: pseudo-protocol, but still have it inherit # options from its parent node. -hmp_cmds="qemu-io none0 \"reopen -o cache.writeback=off,cache.direct=on,cache.no-flush=on\" +hmp_cmds="qemu-io none0 \"reopen -o cache.direct=on,cache.no-flush=on\" info block none0 info block image info block blkdebug diff --git a/tests/qemu-iotests/142.out b/tests/qemu-iotests/142.out index c922490..3d5ef5f 100644 --- a/tests/qemu-iotests/142.out +++ b/tests/qemu-iotests/142.out @@ -414,7 +414,7 @@ cache.no-flush=on on backing-file --- Change cache mode in parent, child has explicit option in JSON --- - Cache mode: writethrough, direct, ignore flushes + Cache mode: writeback, direct, ignore flushes Cache mode: writeback, direct, ignore flushes Cache mode: writeback, direct, ignore flushes Cache mode: writeback, ignore flushes
We must forbid changing the WCE flag in bdrv_reopen() in the same patch, as otherwise the behaviour would change so that the flag takes precedence over the explicitly specified option. The correct value of the WCE flag depends on the BlockBackend user (e.g. guest device) and isn't a decision that the QMP client makes, so this change is what we want. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block.c | 18 ++++++------------ qemu-io-cmds.c | 14 +++++++++++++- tests/qemu-iotests/142 | 2 +- tests/qemu-iotests/142.out | 2 +- 4 files changed, 21 insertions(+), 15 deletions(-)