[v1] block: temporarily hold the new AioContext of bs_top in bdrv_append()

[PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append()

Posted by Stefano Garzarella 1 year, 2 months ago

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 (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);
-- 
2.39.1

Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append()

Posted by Kevin Wolf 1 year, 2 months ago

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

Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append()

Posted by Stefano Garzarella 1 year, 2 months ago

On Tue, Feb 14, 2023 at 12:56 PM Kevin Wolf <kwolf@redhat.com> 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 <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.

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?

Thanks,
Stefano

Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append()

Posted by Kevin Wolf 1 year, 2 months ago

Am 14.02.2023 um 13:22 hat Stefano Garzarella geschrieben:
> On Tue, Feb 14, 2023 at 12:56 PM Kevin Wolf <kwolf@redhat.com> 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 <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.
> 
> 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. 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?

Kevin

Re: [PATCH] block: temporarily hold the new AioContext of bs_top in bdrv_append()

Posted by Stefano Garzarella 1 year, 2 months ago

On Tue, Feb 14, 2023 at 3:06 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 14.02.2023 um 13:22 hat Stefano Garzarella geschrieben:
> > On Tue, Feb 14, 2023 at 12:56 PM Kevin Wolf <kwolf@redhat.com> 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 <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.
> >
> > 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