[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv
From: |
Kevin Wolf |
Subject: |
Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append() |
Date: |
Tue, 14 Feb 2023 12:56:18 +0100 |
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 <aliang@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 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.
> }
>
> + 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.
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. It's really time to get rid of it.
Kevin