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 <kw...@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");
signature.asc
Description: OpenPGP digital signature