Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen

[Kevin Wolf]

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