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 <[email protected]> > --- > 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
