Hi,

v1 cover letter:
https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html

bdrv_replace_child_noperm() drains the child via
bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
bdrv_parent_drained_end_single() at its end will be called on an empty
child, making the BDRV_POLL_WHILE() in it poll the main AioContext
(because c->bs is NULL).

That\ufffd\ufffd\ufffds wrong, though, because it\ufffd\ufffd\ufffds supposed to operate on the parent.
bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
the parents\ufffd\ufffd\ufffd AioContext, which may be anything, not necessarily the main
context.  Therefore, we must poll the parent\ufffd\ufffd\ufffds context.

Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
Patch 1 ensures that we can legally call
bdrv_child_get_parent_aio_context() from those I/O context functions,
and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
failure if it beginning a drain can end up in blk_get_aio_context()
before blk->ctx has been updated.


v2:
- Patch 1:
  - Move bdrv_child_get_parent_aio_context() from block-global-state.h
    to block-io.h
  - Move BdrvChildClass.get_parent_aio_context into the I/O code section
  - Mark blk_root_get_parent_aio_context() as I/O code
  - Mark child_job_get_parent_aio_context(), and lock the job mutex,
    which we now need to do (as of 3ed4f708fe1)
- Patch 2:
  - Added comment similar to Kevin\ufffd\ufffd\ufffds suggestion
    (Still decided to take Kevin\ufffd\ufffd\ufffds R-b, even though I didn\ufffd\ufffd\ufffdt use the
    literal suggestion, but made it a bit more verbose)


Hanna Reitz (3):
  block: Make bdrv_child_get_parent_aio_context I/O
  block-backend: Update ctx immediately after root
  block: Start/end drain on correct AioContext

 include/block/block-global-state.h | 1 -
 include/block/block-io.h           | 2 ++
 include/block/block_int-common.h   | 4 ++--
 block.c                            | 2 +-
 block/block-backend.c              | 9 ++++++++-
 block/io.c                         | 6 ++++--
 blockjob.c                         | 3 ++-
 7 files changed, 19 insertions(+), 8 deletions(-)

-- 
2.36.1

Am 07.11.2022 um 16:13 hat Hanna Reitz geschrieben:
> Hi,
> 
> v1 cover letter:
> https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html
> 
> bdrv_replace_child_noperm() drains the child via
> bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
> bdrv_parent_drained_end_single() at its end will be called on an empty
> child, making the BDRV_POLL_WHILE() in it poll the main AioContext
> (because c->bs is NULL).
> 
> That’s wrong, though, because it’s supposed to operate on the parent.
> bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
> the parents’ AioContext, which may be anything, not necessarily the main
> context.  Therefore, we must poll the parent’s context.
> 
> Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
> Patch 1 ensures that we can legally call
> bdrv_child_get_parent_aio_context() from those I/O context functions,
> and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
> failure if it beginning a drain can end up in blk_get_aio_context()
> before blk->ctx has been updated.

Thanks, applied to the block branch.

I would still be interested in a test case as a follow-up.

Kevin

On 10.11.22 15:01, Kevin Wolf wrote:
> Am 07.11.2022 um 16:13 hat Hanna Reitz geschrieben:
>> Hi,
>>
>> v1 cover letter:
>> https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html
>>
>> bdrv_replace_child_noperm() drains the child via
>> bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
>> bdrv_parent_drained_end_single() at its end will be called on an empty
>> child, making the BDRV_POLL_WHILE() in it poll the main AioContext
>> (because c->bs is NULL).
>>
>> That’s wrong, though, because it’s supposed to operate on the parent.
>> bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
>> the parents’ AioContext, which may be anything, not necessarily the main
>> context.  Therefore, we must poll the parent’s context.
>>
>> Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
>> Patch 1 ensures that we can legally call
>> bdrv_child_get_parent_aio_context() from those I/O context functions,
>> and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
>> failure if it beginning a drain can end up in blk_get_aio_context()
>> before blk->ctx has been updated.
> Thanks, applied to the block branch.

Thanks!

I tested your drain series, and it does indeed fix the bug, too. (Sorry 
for the delay, I thought it’d take less time to write an iotest...)

> I would still be interested in a test case as a follow-up.

Got it working now and sent as “tests/stream-under-throttle: New test”.

Hanna

Am 10.11.2022 um 17:11 hat Hanna Reitz geschrieben:
> On 10.11.22 15:01, Kevin Wolf wrote:
> > Am 07.11.2022 um 16:13 hat Hanna Reitz geschrieben:
> > > Hi,
> > > 
> > > v1 cover letter:
> > > https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html
> > > 
> > > bdrv_replace_child_noperm() drains the child via
> > > bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
> > > bdrv_parent_drained_end_single() at its end will be called on an empty
> > > child, making the BDRV_POLL_WHILE() in it poll the main AioContext
> > > (because c->bs is NULL).
> > > 
> > > That’s wrong, though, because it’s supposed to operate on the parent.
> > > bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
> > > the parents’ AioContext, which may be anything, not necessarily the main
> > > context.  Therefore, we must poll the parent’s context.
> > > 
> > > Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
> > > Patch 1 ensures that we can legally call
> > > bdrv_child_get_parent_aio_context() from those I/O context functions,
> > > and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
> > > failure if it beginning a drain can end up in blk_get_aio_context()
> > > before blk->ctx has been updated.
> > Thanks, applied to the block branch.
> 
> Thanks!
> 
> I tested your drain series, and it does indeed fix the bug, too.
> (Sorry for the delay, I thought it’d take less time to write an
> iotest...)

Thanks for testing, it's good to have this confirmed.

> > I would still be interested in a test case as a follow-up.
> 
> Got it working now and sent as “tests/stream-under-throttle: New test”.

Great! I'll take a look.

Kevin

Am 07.11.2022 um 16:13 hat Hanna Reitz geschrieben:
> Hi,
> 
> v1 cover letter:
> https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html
> 
> bdrv_replace_child_noperm() drains the child via
> bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
> bdrv_parent_drained_end_single() at its end will be called on an empty
> child, making the BDRV_POLL_WHILE() in it poll the main AioContext
> (because c->bs is NULL).
> 
> That’s wrong, though, because it’s supposed to operate on the parent.
> bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
> the parents’ AioContext, which may be anything, not necessarily the main
> context.  Therefore, we must poll the parent’s context.
> 
> Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
> Patch 1 ensures that we can legally call
> bdrv_child_get_parent_aio_context() from those I/O context functions,
> and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
> failure if it beginning a drain can end up in blk_get_aio_context()
> before blk->ctx has been updated.

Hmm, I may have unintentionally made this series obsolete with the drain
series I sent today. The poll instances that you're fixing simply don't
exist any more after it.

Can you check if the bug is gone with my series? I would hope so, but if
not, we would probably need to apply a fix in a different place.

Kevin

Am 08.11.2022 um 15:13 hat Kevin Wolf geschrieben:
> Am 07.11.2022 um 16:13 hat Hanna Reitz geschrieben:
> > Hi,
> > 
> > v1 cover letter:
> > https://lists.nongnu.org/archive/html/qemu-block/2022-09/msg00389.html
> > 
> > bdrv_replace_child_noperm() drains the child via
> > bdrv_parent_drained_{begin,end}_single().  When it removes a child, the
> > bdrv_parent_drained_end_single() at its end will be called on an empty
> > child, making the BDRV_POLL_WHILE() in it poll the main AioContext
> > (because c->bs is NULL).
> > 
> > That’s wrong, though, because it’s supposed to operate on the parent.
> > bdrv_parent_drained_end_single_no_poll() will have scheduled any BHs in
> > the parents’ AioContext, which may be anything, not necessarily the main
> > context.  Therefore, we must poll the parent’s context.
> > 
> > Patch 3 does this for both bdrv_parent_drained_{begin,end}_single().
> > Patch 1 ensures that we can legally call
> > bdrv_child_get_parent_aio_context() from those I/O context functions,
> > and patch 2 fixes blk_do_set_aio_context() to not cause an assertion
> > failure if it beginning a drain can end up in blk_get_aio_context()
> > before blk->ctx has been updated.
> 
> Hmm, I may have unintentionally made this series obsolete with the drain
> series I sent today. The poll instances that you're fixing simply don't
> exist any more after it.
> 
> Can you check if the bug is gone with my series? I would hope so, but if
> not, we would probably need to apply a fix in a different place.

Actually, on second thoughts, we'd probably still apply your patches as
a fix for 7.2 and then have my patches which would get rid of the
polling only in block-next. Patch 1 and 2 of this series would stay in
the tree, and that seems to make sense to me, too. So obsolete was not
the right word.

But it would still be interesting to see if my series fixes the bug,
too, because otherwise it might introduce a regression later.

By the way, is the bug hard to test in a test case? If this series had
one, I could just have run it myself against my tree.

Kevin