From: Kevin Wolf <kwolf@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Cc: Alberto Garcia <berto@igalia.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org,
Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v4 4/6] block: Support multiple reopening with x-blockdev-reopen
Date: Wed, 5 May 2021 18:18:54 +0200 [thread overview]
Message-ID: <YJLFboRME/eFXetd@merkur.fritz.box> (raw)
In-Reply-To: <b0f51127-d3c0-7334-6dcf-45f7d66270c2@virtuozzo.com>
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
next prev parent reply other threads:[~2021-05-05 16:46 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-17 17:15 [PATCH v4 0/6] Allow changing bs->file on reopen Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 1/6] block: Add bdrv_reopen_queue_free() Alberto Garcia
2021-03-18 13:32 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 2/6] block: Allow changing bs->file on reopen Alberto Garcia
2021-03-18 14:25 ` Vladimir Sementsov-Ogievskiy
2021-03-24 12:25 ` Alberto Garcia
2021-03-24 15:08 ` Vladimir Sementsov-Ogievskiy
2021-05-05 13:58 ` Kevin Wolf
2021-05-07 7:11 ` Vladimir Sementsov-Ogievskiy
2021-05-07 14:09 ` Kevin Wolf
2021-05-10 9:26 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 3/6] iotests: Test replacing files with x-blockdev-reopen Alberto Garcia
2021-05-05 15:57 ` Kevin Wolf
2021-03-17 17:15 ` [PATCH v4 4/6] block: Support multiple reopening " Alberto Garcia
2021-03-18 14:45 ` Vladimir Sementsov-Ogievskiy
2021-05-05 16:18 ` Kevin Wolf [this message]
2021-05-06 9:21 ` Vladimir Sementsov-Ogievskiy
2021-03-17 17:15 ` [PATCH v4 5/6] iotests: Test reopening multiple devices at the same time Alberto Garcia
2021-03-17 17:15 ` [PATCH v4 6/6] block: Make blockdev-reopen stable API Alberto Garcia
2021-05-14 15:53 ` [PATCH v4 0/6] Allow changing bs->file on reopen Vladimir Sementsov-Ogievskiy
2021-06-09 15:53 ` Kevin Wolf
2021-06-09 16:40 ` Vladimir Sementsov-Ogievskiy
2021-06-10 12:10 ` Vladimir Sementsov-Ogievskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YJLFboRME/eFXetd@merkur.fritz.box \
--to=kwolf@redhat.com \
--cc=berto@igalia.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).