Re: [PATCH 06/12] qcow2: Fix open with 'file' in iothread

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3811 bytes --]

Am 26.05.2023 um 10:40 hat Kevin Wolf geschrieben:
> Am 25.05.2023 um 20:51 hat Stefan Hajnoczi geschrieben:
> > On Thu, May 25, 2023 at 02:47:07PM +0200, Kevin Wolf wrote:
> > > qcow2_open() doesn't work correctly when opening the 'file' child moves
> > > bs to an iothread, for several reasons:
> > > 
> > > - It uses BDRV_POLL_WHILE() to wait for the qcow2_open_entry()
> > >   coroutine, which involves dropping the AioContext lock for bs when it
> > >   is not in the main context - but we don't hold it, so this crashes.
> > > 
> > > - It runs the qcow2_open_entry() coroutine in the current thread instead
> > >   of the new AioContext of bs.
> > > 
> > > - qcow2_open_entry() doesn't notify the main loop when it's done.
> > > 
> > > This patches fixes these issues around delegating work to a coroutine.
> > > Temporarily dropping the main AioContext lock is not necessary because
> > > we know we run in the main thread.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  block/qcow2.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/qcow2.c b/block/qcow2.c
> > > index b00b4e7575..7f3948360d 100644
> > > --- a/block/qcow2.c
> > > +++ b/block/qcow2.c
> > > @@ -1904,6 +1904,8 @@ static void coroutine_fn qcow2_open_entry(void *opaque)
> > >      qoc->ret = qcow2_do_open(qoc->bs, qoc->options, qoc->flags, true,
> > >                               qoc->errp);
> > >      qemu_co_mutex_unlock(&s->lock);
> > > +
> > > +    aio_wait_kick();
> > >  }
> > >  
> > >  static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
> > > @@ -1929,8 +1931,10 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
> > >  
> > >      assert(!qemu_in_coroutine());
> > >      assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > > -    qemu_coroutine_enter(qemu_coroutine_create(qcow2_open_entry, &qoc));
> > > -    BDRV_POLL_WHILE(bs, qoc.ret == -EINPROGRESS);
> > > +
> > > +    aio_co_enter(bdrv_get_aio_context(bs),
> > > +                 qemu_coroutine_create(qcow2_open_entry, &qoc));
> > > +    AIO_WAIT_WHILE_UNLOCKED(NULL, qoc.ret == -EINPROGRESS);
> > 
> > Want to update the doc comment for bdrv_open_file_child() with a warning
> > that @parent's AioContext can change?
> 
> Ok, I'll squash in the following.
> 
> I seem to remember that bdrv_open_child() is actually wrong, too,
> regarding AioContext locking, but I didn't need to fix it for this test
> case. We need more test cases to break everything. :-)
> 
> And the earlier we can get rid of the AioContext lock the better,
> because it seems really hard to get right across the board.
> 
> Kevin
> 
> diff --git a/block.c b/block.c
> index a2f8d5a0c0..263e1e22f3 100644
> --- a/block.c
> +++ b/block.c
> @@ -3644,6 +3644,9 @@ done:
>   * BlockdevRef.
>   *
>   * The BlockdevRef will be removed from the options QDict.
> + *
> + * @parent can move to a different AioContext in this functions. Callers must

s/functions/function/, in fact. :-)

> + * make sure that their AioContext locking is still correct after this.
>   */
>  BdrvChild *bdrv_open_child(const char *filename,
>                             QDict *options, const char *bdref_key,
> @@ -3668,6 +3671,9 @@ BdrvChild *bdrv_open_child(const char *filename,
> 
>  /*
>   * Wrapper on bdrv_open_child() for most popular case: open primary child of bs.
> + *
> + * @parent can move to a different AioContext in this functions. Callers must

And here, too.

> + * make sure that their AioContext locking is still correct after this.
>   */
>  int bdrv_open_file_child(const char *filename,
>                           QDict *options, const char *bdref_key,

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]