From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60019) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YtKZp-0006x7-MP for qemu-devel@nongnu.org; Fri, 15 May 2015 14:43:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YtKZl-0008Mq-Tq for qemu-devel@nongnu.org; Fri, 15 May 2015 14:43:17 -0400 Message-ID: <55563E3C.8040201@redhat.com> Date: Fri, 15 May 2015 20:43:08 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-32-git-send-email-kwolf@redhat.com> In-Reply-To: <1431105726-3682-32-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 31/34] block: Move cache options into options QDict List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org 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 > --- > 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