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

[Kevin Wolf]

Am 27.11.2015 um 20:57 hat Max Reitz geschrieben:
> 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 
> > ---
> >  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(_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():

Alles kaputt. :-)

> 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.

I think you didn't interpret @parent_options correctly. It is NULL for
the node that bdrv_reopen() is called for. It is non-NULL for
(recursive) child nodes of that node that inherit options from it. Not
sure if it matters for this case, though.

In my opinion, what these block jobs are doing is insane. They should
never touch the cache mode of a node as long as this cache mode can be
shared with other users. It doesn't matter much with block jobs that can
only work on a BDS that they just created, but with blockdev-backup I'd
consider this a real bug.

> Maybe 

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

[Max Reitz]

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 
> ---
>  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(_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-block] [PATCH v2 17/21] block: Move cache options into options QDict

[Kevin Wolf]

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 
---
 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
@@ -29,6 +29,7 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
@@ -706,9 +707,16 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
 /* Enable protocol handling, disable format probing for bs->file */
 flags |= BDRV_O_PROTOCOL;
 
+/* If the cache mode isn't explicitly set, inherit direct and no-flush from
+ * the parent. */
+qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+
 /* Our block drivers take care to send flushes and respect unmap policy,
- * so we can enable both unconditionally on lower layers. */
-flags |= BDRV_O_CACHE_WB | BDRV_O_UNMAP;
+ * so we can default to enable both on lower layers regardless of the
+ * corresponding parent options. */
+qdict_set_default_str(child_options, BDRV_OPT_CACHE_WB, "on");
+flags |= BDRV_O_UNMAP;
 
 /* Clear flags that only apply to the top layer */
 flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
@@ -747,6 +755,11 @@ static void bdrv_backing_options(int *child_flags, QDict 
*child_options,
 {
 int flags = parent_flags;
 
+/* The cache mode is inherited unmodified for backing files */
+qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_WB);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
+qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+
 /* backing files always opened read-only */
 flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
 
@@ -780,6 +793,42 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
 return open_flags;
 }
 
+static void update_flags_from_options(int *flags, QemuOpts *opts)
+{
+*flags &= ~BDRV_O_CACHE_MASK;
+
+assert(qemu_opt_find(opts, BDRV_OPT_CACHE_WB));
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, false)) {
+*flags |= BDRV_O_CACHE_WB;
+}
+
+assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+*flags |= BDRV_O_NO_FLUSH;
+}
+
+assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+*flags |= BDRV_O_NOCACHE;
+}
+}
+
+static void update_options_from_flags(QDict *options, int flags)
+{
+if (!qdict_haskey(options, BDRV_OPT_CACHE_WB)) {
+qdict_put(options, BDRV_OPT_CACHE_WB,
+  qbool_from_bool(flags & BDRV_O_CACHE_WB));
+}
+if (!qdict_haskey(options, BDRV_OPT_CACHE_DIRECT)) {
+qdict_put(options, BDRV_OPT_CACHE_DIRECT,
+  qbool_from_bool(flags & BDRV_O_NOCACHE));
+}
+if (!qdict_haskey(options, BDRV_OPT_CACHE_NO_FLUSH)) {
+qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH,
+  qbool_from_bool(flags & BDRV_O_NO_FLUSH));
+}
+}
+
 static void bdrv_assign_node_name(BlockDriverState *bs,
   const char *node_name,
   Error **errp)
@@ -831,6 +880,21 @@ static QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "Block driver to use for the node",
 },
+{
+.name = BDRV_OPT_CACHE_WB,
+.type = QEMU_OPT_BOOL,
+.help = "Enable writeback mode",
+},
+{
+.name = BDRV_OPT_CACHE_DIRECT,
+.type = QEMU_OPT_BOOL,
+.help = "Bypass software writeback cache on the host",
+},
+{
+.name = BDRV_OPT_CACHE_NO_FLUSH,
+.type = QEMU_OPT_BOOL,
+.help = "Ignore flush requests",
+},
 { /* end of list */