Re: [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options QDict

[Max Reitz <mreitz@redhat.com>]

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

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 <kwolf@redhat.com>
> ---
>  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");


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