[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reo
From: |
Kevin Wolf |
Subject: |
Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen |
Date: |
Wed, 5 May 2021 18:18:54 +0200 |
Am 18.03.2021 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 17.03.2021 20:15, Alberto Garcia wrote:
> > Signed-off-by: Alberto Garcia <berto@igalia.com>
> > ---
> > qapi/block-core.json | 18 +++++----
> > blockdev.c | 78 +++++++++++++++++++++++---------------
> > tests/qemu-iotests/155 | 9 +++--
> > tests/qemu-iotests/165 | 4 +-
> > tests/qemu-iotests/245 | 27 +++++++------
> > tests/qemu-iotests/248 | 2 +-
> > tests/qemu-iotests/248.out | 2 +-
> > tests/qemu-iotests/296 | 9 +++--
> > tests/qemu-iotests/298 | 4 +-
> > 9 files changed, 92 insertions(+), 61 deletions(-)
> >
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 9f555d5c1d..9150f765da 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4181,13 +4181,15 @@
> > ##
> > # @x-blockdev-reopen:
> > #
> > -# Reopens a block device using the given set of options. Any option
> > -# not specified will be reset to its default value regardless of its
> > -# previous status. If an option cannot be changed or a particular
> > +# Reopens one or more block devices using the given set of options.
> > +# Any option not specified will be reset to its default value regardless
> > +# of its previous status. If an option cannot be changed or a particular
> > # driver does not support reopening then the command will return an
> > -# error.
> > +# error. All devices in the list are reopened in one transaction, so
> > +# if one of them fails then the whole transaction is cancelled.
> > #
> > -# The top-level @node-name option (from BlockdevOptions) must be
> > +# The command receives a list of block devices to reopen. For each one
> > +# of them, the top-level @node-name option (from BlockdevOptions) must be
> > # specified and is used to select the block device to be reopened.
> > # Other @node-name options must be either omitted or set to the
> > # current name of the appropriate node. This command won't change any
> > @@ -4207,8 +4209,8 @@
> > #
> > # 4) NULL: the current child (if any) is detached.
> > #
> > -# Options (1) and (2) are supported in all cases, but at the moment
> > -# only @backing allows replacing or detaching an existing child.
> > +# Options (1) and (2) are supported in all cases. Option (3) is
> > +# supported for @file and @backing, and option (4) for @backing only.
>
> A bit of it should be already updated in "[PATCH v4 2/6] block: Allow
> changing bs->file on reopen"
>
> > #
> > # Unlike with blockdev-add, the @backing option must always be present
> > # unless the node being reopened does not have a backing file and its
> > @@ -4218,7 +4220,7 @@
> > # Since: 4.0
> > ##
> > { 'command': 'x-blockdev-reopen',
> > - 'data': 'BlockdevOptions', 'boxed': true }
> > + 'data': { 'options': ['BlockdevOptions'] } }
> > ##
> > # @blockdev-del:
> > diff --git a/blockdev.c b/blockdev.c
> > index 825d40aa11..7019397b05 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3580,46 +3580,64 @@ fail:
> > visit_free(v);
> > }
> > -void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
> > +void qmp_x_blockdev_reopen(BlockdevOptionsList *reopen_list, Error **errp)
> > {
> > - BlockDriverState *bs;
> > - AioContext *ctx;
> > - QObject *obj;
> > - Visitor *v = qobject_output_visitor_new(&obj);
> > - BlockReopenQueue *queue;
> > - QDict *qdict;
> > + BlockReopenQueue *queue = NULL;
> > + GSList *aio_ctxs = NULL;
> > + GSList *visitors = NULL;
> > + GSList *drained = NULL;
> > - /* Check for the selected node name */
> > - if (!options->has_node_name) {
> > - error_setg(errp, "node-name not specified");
> > - goto fail;
> > - }
> > + /* Add each one of the BDS that we want to reopen to the queue */
> > + for (; reopen_list != NULL; reopen_list = reopen_list->next) {
> > + BlockdevOptions *options = reopen_list->value;
> > + BlockDriverState *bs;
> > + AioContext *ctx;
> > + QObject *obj;
> > + Visitor *v;
> > + QDict *qdict;
> > - bs = bdrv_find_node(options->node_name);
> > - if (!bs) {
> > - error_setg(errp, "Failed to find node with node-name='%s'",
> > + /* Check for the selected node name */
> > + if (!options->has_node_name) {
> > + error_setg(errp, "node-name not specified");
> > + goto fail;
> > + }
> > +
> > + bs = bdrv_find_node(options->node_name);
> > + if (!bs) {
> > + error_setg(errp, "Failed to find node with node-name='%s'",
> > options->node_name);
> > - goto fail;
> > + goto fail;
> > + }
> > +
> > + v = qobject_output_visitor_new(&obj);
> > + visitors = g_slist_prepend(visitors, v);
>
> I'd better just call visit_free inside the block instead of putting v
> to list be freed later after the block..
Yes, I had the same thought.
> > +
> > + /* Put all options in a QDict and flatten it */
> > + visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> > + visit_complete(v, &obj);
> > + qdict = qobject_to(QDict, obj);
> > +
> > + qdict_flatten(qdict);
> > +
> > + ctx = bdrv_get_aio_context(bs);
> > + if (!g_slist_find(aio_ctxs, ctx)) {
> > + aio_ctxs = g_slist_prepend(aio_ctxs, ctx);
> > + aio_context_acquire(ctx);
> > + }
> > + bdrv_subtree_drained_begin(bs);
>
> I expect Kevin will complain that aquiring several context and drain
> them all is a bad idea as it leads to deadlocks..
No need for me to complain when you already complained. :-)
> For more information look at the branches
> [PATCH v2 29/36] blockdev: qmp_x_blockdev_reopen: acquire all contexts
> amd
> [PATCH v2 30/36] block: bdrv_reopen_multiple: refresh permissions on
> updated graph
>
> So, probably here we should acquire context in a loop to call
> bdrv_reopen_queue() (which I think shoud not require drained section).
I think the aio_context_acquire() is right (it is needed for drain) and
we can even keep it across bdrv_reopen_queue() if we want (though there
is probably no reason to do so), but we need to release it again after
the loop iteration (i.e. after bdrv_reopen_queue) so that we won't cause
deadlocks in the next loop iteration.
> And then, bdrv_reopen_multiple() is called with no aio context
> acquired, and no drained section.. And it shoud be refactored to
> properly operate with acquiring and realeasing the contexts and
> drained sections when needed...
The drained section shouldn't be a problem, we can keep this. In fact,
we need this, it is a documented requirement of bdrv_reopen_multiple().
We just need to drop the AioContext lock after drained_begin.
bdrv_reopen_multiple() should really document its requirements regarding
AioContext locks. It probably doesn't really care, but requires that
it be called from the main thread.
> (note preexisting problem of reopen, that during reopen the whole tree
> may be moved to another aio context, but we continue operations with
> acquired old aio context which is wrong).
Do we even acquire the old AioContext?
> > + queue = bdrv_reopen_queue(queue, bs, qdict, false);
> > + drained = g_slist_prepend(drained, bs);
> > }
> > - /* Put all options in a QDict and flatten it */
> > - visit_type_BlockdevOptions(v, NULL, &options, &error_abort);
> > - visit_complete(v, &obj);
> > - qdict = qobject_to(QDict, obj);
> > -
> > - qdict_flatten(qdict);
> > -
> > /* Perform the reopen operation */
> > - ctx = bdrv_get_aio_context(bs);
> > - aio_context_acquire(ctx);
> > - bdrv_subtree_drained_begin(bs);
> > - queue = bdrv_reopen_queue(NULL, bs, qdict, false);
> > bdrv_reopen_multiple(queue, errp);
> > - bdrv_subtree_drained_end(bs);
> > - aio_context_release(ctx);
> > + queue = NULL;
> > fail:
> > - visit_free(v);
> > + bdrv_reopen_queue_free(queue);
This has the same leak that we just fixed in Vladimir's series:
qobject_unref(bs_entry->state.explicit_options);
qobject_unref(bs_entry->state.options);
This part from abort in bdrv_reopen_multiple() is required even before
calling prepare, these QDicts are created in bdrv_reopen_queue().
We can't just move it unconditionally to bdrv_reopen_queue_free(),
though, because commit makes use of them and then we can't just unref
them. Either commit needs to take an extra reference or we'd need a bool
parameter in bdrv_reopen_queue_free(). I guess taking the extra
reference is cleanest.
> > + g_slist_free_full(drained, (GDestroyNotify) bdrv_subtree_drained_end);
> > + g_slist_free_full(aio_ctxs, (GDestroyNotify) aio_context_release);
> > + g_slist_free_full(visitors, (GDestroyNotify) visit_free);
> > }
Kevin
- Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen,
Kevin Wolf <=