Am 09.11.2022 um 17:00 hat Vladimir Sementsov-Ogievskiy geschrieben:
> In subject: individual
>
> On 11/8/22 15:37, Kevin Wolf wrote:
> > bdrv_reopen() and friends use subtree drains as a lazy way of covering
> > all the nodes they touch. Turns out that this lazy way is a lot more
> > complicated than just draining the nodes individually, even not
> > accounting for the additional complexity in the drain mechanism itself.
> >
> > Simplify the code by switching to draining the individual nodes that are
> > already managed in the BlockReopenQueue anyway.
> >
> > Signed-off-by: Kevin Wolf <kw...@redhat.com>
> > ---
> > block.c | 11 ++++-------
> > block/replication.c | 6 ------
> > blockdev.c | 13 -------------
> > 3 files changed, 4 insertions(+), 26 deletions(-)
> >
>
> [..]
>
> > bdrv_reopen_queue_free(queue);
> > - for (p = drained; p; p = p->next) {
> > - BlockDriverState *bs = p->data;
> > - AioContext *ctx = bdrv_get_aio_context(bs);
> > -
> > - aio_context_acquire(ctx);
>
> In bdrv_reopen_queue_free() we don't have this acquire()/release()
> pair around bdrv_drained_end(). We don't need it anymore?
Good catch, I think we do.
Reopen is a bit messy with AioContext locks. I think the rule is
supposed to be that bdrv_reopen_queue() requires that the lock for
bs->aio_context is held, and bdrv_reopen_multiple() requires that no
AioContext lock is held, right?
Because the former is not actually true: qmp_blockdev_reopen() and the
'replication' block driver do indeed take the lock, but bdrv_reopen()
drops it for both functions!
So I think we also need an additional fix for bdrv_reopen() to drop the
lock only after calling bdrv_reopen_queue(). It may not have made a
difference before, but now that we call bdrv_drained_begin() in it, it
seems important.
Kevin
