Re: [Qemu-devel] [PATCH 31/34] block: Move cache options into options QDict

[Max Reitz <mreitz@redhat.com>]

On 08.05.2015 19:22, 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.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block.c    | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   blockdev.c | 27 +++++---------------
>   2 files changed, 88 insertions(+), 24 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8faa5ce..2430070 100644
> --- a/block.c
> +++ b/block.c
> @@ -27,6 +27,7 @@
>   #include "block/block_int.h"
>   #include "block/blockjob.h"
>   #include "qemu/module.h"
> +#include "qapi/qmp/qbool.h"
>   #include "qapi/qmp/qjson.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/sysemu.h"
> @@ -689,9 +690,15 @@ 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);
> +

Well, this directly violates what I wanted these functions to guarantee: 
Not set any option with a dot in them. It's fine anyway because "cache" 
is never used as the bdref_key for child BDS.

>       /* Our block drivers take care to send flushes and respect unmap policy,
>        * so we can enable both unconditionally on lower layers. */

The comment reads a bit strange now (because it's referring to two 
heterogenous lines instead of one line with two flags on it), and also 
WB is no longer "unconditionally" enabled.

> -    flags |= BDRV_O_CACHE_WB | BDRV_O_UNMAP;
> +    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);
> @@ -725,6 +732,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);
>   
> @@ -758,6 +770,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_int(flags & BDRV_O_CACHE_WB));

Urgh, qbool_from_int() doesn't cast the int to bool? :-/



> +    }
> +    if (!qdict_haskey(options, BDRV_OPT_CACHE_DIRECT)) {
> +        qdict_put(options, BDRV_OPT_CACHE_DIRECT,
> +                  qbool_from_int(flags & BDRV_O_NOCACHE));
> +    }
> +    if (!qdict_haskey(options, BDRV_OPT_CACHE_NO_FLUSH)) {
> +        qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH,
> +                  qbool_from_int(flags & BDRV_O_NO_FLUSH));
> +    }
> +}
> +
>   static void bdrv_assign_node_name(BlockDriverState *bs,
>                                     const char *node_name,
>                                     Error **errp)
> @@ -804,6 +852,20 @@ 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,

Why no .help?

> +        },
> +        {
> +            .name = BDRV_OPT_CACHE_NO_FLUSH,
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Ignore flush requests",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -907,7 +969,9 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>       bs->drv = drv;
>       bs->opaque = g_malloc0(drv->instance_size);
>   
> -    bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
> +    /* Apply cache mode options */
> +    update_flags_from_options(&bs->open_flags, opts);
> +    bdrv_set_enable_write_cache(bs, bs->open_flags & BDRV_O_CACHE_WB);
>   
>       /* Open the image, either directly or using a protocol */
>       if (drv->bdrv_file_open) {
> @@ -1025,6 +1089,9 @@ static int bdrv_fill_options(QDict **options, const char **pfilename, int flags,
>           *pfilename = filename = NULL;
>       }
>   
> +    /* Translate cache options from flags into options */
> +    update_options_from_flags(*options, flags);
> +
>       /* Fetch the file name from the options QDict if necessary */
>       if (protocol && filename) {
>           if (!qdict_haskey(*options, "filename")) {
> @@ -1650,12 +1717,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);
>       qdict_join(options, old_options, false);
> @@ -1824,6 +1901,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
>           goto error;
>       }
>   
> +    update_flags_from_options(&reopen_state->flags, opts);
> +
>       /* 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");
> diff --git a/blockdev.c b/blockdev.c
> index 77cbe72..90853aa 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -391,15 +391,12 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>           }
>       }
>   
> -    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true)) {
> -        bdrv_flags |= BDRV_O_CACHE_WB;
> -    }
> -    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
> -        bdrv_flags |= BDRV_O_NOCACHE;
> -    }
> -    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
> -        bdrv_flags |= BDRV_O_NO_FLUSH;
> -    }
> +    /* bdrv_open() defaults to the values in bdrv_flags (for compatibility with
> +     * other callers) rather than what we want as the real defaults. Apply the
> +     * defaults here instead. */
> +    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_WB, "on");
> +    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
> +    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
>   
>   #ifdef CONFIG_LINUX_AIO
>       if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
> @@ -3106,18 +3103,6 @@ QemuOptsList qemu_common_drive_opts = {
>               .type = QEMU_OPT_STRING,
>               .help = "discard operation (ignore/off, unmap/on)",
>           },{
> -            .name = BDRV_OPT_CACHE_WB,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "enables writeback mode for any caches",
> -        },{
> -            .name = BDRV_OPT_CACHE_DIRECT,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "enables use of O_DIRECT (bypass the host page cache)",
> -        },{
> -            .name = BDRV_OPT_CACHE_NO_FLUSH,
> -            .type = QEMU_OPT_BOOL,
> -            .help = "ignore any flush requests for the device",
> -        },{
>               .name = "aio",
>               .type = QEMU_OPT_STRING,
>               .help = "host AIO implementation (threads, native)",

Question: bdrv_set_enable_write_cache() changes bs->open_flags so the 
option (BDRV_O_CACHE_WB) is preserved beyond a reopen (according to the 
comment there). Is this still the case? Or should it instead modify 
bs->options now?

Max