On Tue, Feb 14, 2023 at 3:06 PM Kevin Wolf <[email protected]> wrote: > > Am 14.02.2023 um 13:22 hat Stefano Garzarella geschrieben: > > On Tue, Feb 14, 2023 at 12:56 PM Kevin Wolf <[email protected]> wrote: > > > > > > Am 14.02.2023 um 11:51 hat Stefano Garzarella geschrieben: > > > > bdrv_append() is called with bs_top AioContext held, but > > > > bdrv_attach_child_noperm() could change the AioContext of bs_top. > > > > > > > > bdrv_replace_node_noperm() calls bdrv_drained_begin() starting from > > > > commit 2398747128 ("block: Don't poll in bdrv_replace_child_noperm()"). > > > > bdrv_drained_begin() can call BDRV_POLL_WHILE that assumes the new lock > > > > is taken, so let's temporarily hold the new AioContext to prevent QEMU > > > > from failing in BDRV_POLL_WHILE when it tries to release the wrong > > > > AioContext. > > > > > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2168209 > > > > Reported-by: Aihua Liang <[email protected]> > > > > Signed-off-by: Stefano Garzarella <[email protected]> > > > > --- > > > > I'm not sure whether to use the following Fixes tag. That commit added > > > > the > > > > calls to bdrv_drained_begin() in bdrv_replace_node_noperm(), but maybe > > > > the > > > > problem was pre-existing. > > > > > > > > Fixes: 2398747128 ("block: Don't poll in bdrv_replace_child_noperm()") > > > > > > > > Note: a local reproducer is attached in the BZ, it is based on the > > > > Aihua Liang > > > > report and it hits the issue with a 20% ratio. > > > > --- > > > > block.c | 23 +++++++++++++++++++++++ > > > > 1 file changed, 23 insertions(+) > > > > > > > > diff --git a/block.c b/block.c > > > > index aa9062f2c1..0e2bc11e0b 100644 > > > > --- a/block.c > > > > +++ b/block.c > > > > @@ -5266,6 +5266,8 @@ int bdrv_drop_filter(BlockDriverState *bs, Error > > > > **errp) > > > > * child. > > > > * > > > > * This function does not create any image files. > > > > + * > > > > + * The caller must hold the AioContext lock for @bs_top. > > > > */ > > > > int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, > > > > Error **errp) > > > > @@ -5273,11 +5275,14 @@ int bdrv_append(BlockDriverState *bs_new, > > > > BlockDriverState *bs_top, > > > > int ret; > > > > BdrvChild *child; > > > > Transaction *tran = tran_new(); > > > > + AioContext *old_context, *new_context; > > > > > > > > GLOBAL_STATE_CODE(); > > > > > > > > assert(!bs_new->backing); > > > > > > > > + old_context = bdrv_get_aio_context(bs_top); > > > > + > > > > child = bdrv_attach_child_noperm(bs_new, bs_top, "backing", > > > > &child_of_bds, > > > > bdrv_backing_role(bs_new), > > > > tran, errp); > > > > @@ -5286,11 +5291,29 @@ int bdrv_append(BlockDriverState *bs_new, > > > > BlockDriverState *bs_top, > > > > goto out; > > > > } > > > > > > > > + /* > > > > + * bdrv_attach_child_noperm could change the AioContext of bs_top. > > > > + * bdrv_replace_node_noperm calls bdrv_drained_begin, so let's > > > > temporarily > > > > + * hold the new AioContext, since bdrv_drained_begin calls > > > > BDRV_POLL_WHILE > > > > + * that assumes the new lock is taken. > > > > + */ > > > > + new_context = bdrv_get_aio_context(bs_top); > > > > + > > > > + if (old_context != new_context) { > > > > + aio_context_release(old_context); > > > > + aio_context_acquire(new_context); > > > > + } > > > > + > > > > ret = bdrv_replace_node_noperm(bs_top, bs_new, true, tran, errp); > > > > if (ret < 0) { > > > > goto out; > > > > > > If we take the error path, we return with new_context locked instead of > > > old_context now. > > > > Grr, I'm blind... > > > > > > > > > } > > > > > > > > + if (old_context != new_context) { > > > > + aio_context_release(new_context); > > > > + aio_context_acquire(old_context); > > > > + } > > > > + > > > > ret = bdrv_refresh_perms(bs_new, tran, errp); > > > > out: > > > > tran_finalize(tran, ret); > > > > > > Strictly speaking, don't we need to hold the lock across > > > tran_finalize(), too? It completes the bdrv_replace_node_noperm() call > > > you covered above. > > > > Right! > > > > > > > > Maybe bdrv_refresh_perms() and bdrv_refresh_limits(), too, in fact. We > > > never clearly defined which functions need the lock and which don't, so > > > hard to tell. > > > > Okay, so to be on the safe side, I'll switch them back just before return. > > > > > It's really time to get rid of it. > > > > How could one disagree? :-) > > > > What about the Fixes tag? Should I include it? > > I'm not sure. Before the patch, bdrv_replace_child_noperm() had a drain > which could have caused the same kind of problems.
I see. > But we're now > draining two BDSes, maybe that is the relevant difference. I guess we've > always had a bug, but that commit made it more likely to actually > trigger? Yes, my same doubt. I also guess it was pre-existing. Maybe, since we are in the same release, we can just mention it in the message, without tagging. I'll send a v2 with the fixes. Thanks, Stefano
