[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options QDict |
Date: |
Fri, 27 Nov 2015 20:57:21 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 |
On 23.11.2015 16:59, Kevin Wolf wrote:
> This adds the cache mode options to the QDict, so that they can be
> specified for child nodes (e.g. backing.cache.direct=off).
>
> The cache modes are not removed from the flags at this point; instead,
> options and flags are kept in sync. If the user specifies both flags and
> options, the options take precedence.
>
> Child node inherit cache modes as options now, they don't use flags any
> more.
>
> Note that this forbids specifying the cache mode for empty drives. It
> didn't make sense anyway to specify it there, because it didn't have any
> effect. blockdev_init() considers the cache options now bdrv_open()
> options and therefore doesn't create an empty drive any more but calls
> into bdrv_open(). This in turn will fail with no driver and filename
> specified.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> block.c | 100
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> blockdev.c | 52 ++++++++++----------------------
> 2 files changed, 111 insertions(+), 41 deletions(-)
>
> diff --git a/block.c b/block.c
> index eff0c19..397014a 100644
> --- a/block.c
> +++ b/block.c
[...]
> @@ -1747,12 +1816,22 @@ static BlockReopenQueue
> *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
> /*
> * Precedence of options:
> * 1. Explicitly passed in options (highest)
> - * 2. TODO Set in flags (only for top level)
> + * 2. Set in flags (only for top level)
> * 3. Retained from explicitly set options of bs
> * 4. Inherited from parent node
> * 5. Retained from effective options of bs
> */
>
> + if (!parent_options) {
> + /*
> + * Any setting represented by flags is always updated. If the
> + * corresponding QDict option is set, it takes precedence. Otherwise
> + * the flag is translated into a QDict option. The old setting of bs
> is
> + * not considered.
> + */
> + update_options_from_flags(options, flags);
> + }
> +
> /* Old explicitly set values (don't overwrite by inherited value) */
> old_options = qdict_clone_shallow(bs->explicit_options);
> bdrv_join_options(bs, options, old_options);
> @@ -1923,6 +2002,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
> BlockReopenQueue *queue,
> goto error;
> }
>
> + 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;
> + }
> + }
> +
Time to get back to my question regarding bdrv_set_enable_write_cache():
1. bdrv_set_enable_write_cache() sets/unsets BDRV_O_CACHE_WB in
bs->open_flags so that it is preserved through a reopen.
2. On reopen, bdrv_reopen_queue_child() calls
update_options_from_flags() unless @parent_options is set. If that
function is called, it will set the appropriate caching options in
@options as dictated by bdrv_set_enable_write_cache().
3. If @parent_options was NULL, the update_flags_from_options() call
here in bdrv_reopen_prepare() will set BDRV_O_CACHE_WB just as
dictated by bdrv_set_enable_write_cache() (unless overwritten).
That is what we want.
4. If @parent_options was not NULL, the caching options in
bs->open_flags are completely overwritten, discarding everything that
had been set by bdrv_set_enable_write_cache(). That's not so good.
@parent_options is tested so that update_options_from_flags() is called
only for root BDSs. It is possible to call bdrv_set_enable_write_cache()
on non-root BDSs (e.g. commit_active_start() does it on the commit base
via mirror_start_job()), so I do think overriding the setting made by
bdrv_set_enable_write_cache() on reopen for any non-root BDSs is not
correct.
Maybe bdrv_set_enable_write_cache() should set cache.writeback in
bs->explicit_options to on or off instead of using bs->open_flags. But
in that case, we can no longer use bdrv_set_enable_write_cache() in
bdrv_open_common(), because the cache setting there may be implicit.
Max
> /* node-name and driver must be unchanged. Put them back into the QDict,
> so
> * that they are checked at the end of this function. */
> value = qemu_opt_get(opts, "node-name");
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v2 09/21] block: Allow specifying child options in reopen, (continued)
- [Qemu-devel] [PATCH v2 09/21] block: Allow specifying child options in reopen, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 13/21] block: Introduce bs->explicit_options, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 01/21] qcow2: Add .bdrv_join_options callback, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 15/21] qemu-iotests: Remove cache mode test without medium, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 16/21] block: reopen: Extract QemuOpts for generic block layer options, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 11/21] block: Add infrastructure for option inheritance, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options QDict, Kevin Wolf, 2015/11/23
- Re: [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options QDict,
Max Reitz <=
- [Qemu-devel] [PATCH v2 04/21] block: Allow references for backing files, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 18/21] blkdebug: Enable reopen, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 19/21] qemu-iotests: Try setting cache mode for children, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 21/21] qemu-iotests: Test reopen with node-name/driver options, Kevin Wolf, 2015/11/23
- [Qemu-devel] [PATCH v2 20/21] qemu-iotests: Test cache mode option inheritance, Kevin Wolf, 2015/11/23