Re: [PATCH v7 13/13] block: Convert 'block_resize' to coroutine

[Kevin Wolf]

Am 28.09.2020 um 11:05 hat Stefan Hajnoczi geschrieben:
> On Fri, Sep 25, 2020 at 06:07:50PM +0200, Kevin Wolf wrote:
> > Am 15.09.2020 um 16:57 hat Stefan Hajnoczi geschrieben:
> > > On Wed, Sep 09, 2020 at 05:11:49PM +0200, Kevin Wolf wrote:
> > > > @@ -2456,8 +2456,7 @@ void qmp_block_resize(bool has_device, const char 
> > > > *device,
> > > >          return;
> > > >      }
> > > >  
> > > > -    aio_context = bdrv_get_aio_context(bs);
> > > > -    aio_context_acquire(aio_context);
> > > > +    old_ctx = bdrv_co_move_to_aio_context(bs);
> > > >  
> > > >      if (size < 0) {
> > > >          error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "size", "a >0 
> > > > size");
> > > 
> > > Is it safe to call blk_new() outside the BQL since it mutates global 
> > > state?
> > > 
> > > In other words, could another thread race with us?
> > 
> > Hm, probably not.
> > 
> > Would it be safer to have the bdrv_co_move_to_aio_context() call only
> > immediately before the drain?
> 
> Yes, sounds good.
> 
> > > > @@ -2479,8 +2478,8 @@ void qmp_block_resize(bool has_device, const char 
> > > > *device,
> > > >      bdrv_drained_end(bs);
> > > >  
> > > >  out:
> > > > +    aio_co_reschedule_self(old_ctx);
> > > >      blk_unref(blk);
> > > > -    aio_context_release(aio_context);
> > > 
> > > The following precondition is violated by the blk_unref -> bdrv_drain ->
> > > AIO_WAIT_WHILE() call if blk->refcnt is 1 here:
> > > 
> > >  * The caller's thread must be the IOThread that owns @ctx or the main 
> > > loop
> > >  * thread (with @ctx acquired exactly once).
> > > 
> > > blk_unref() is called from the main loop thread without having acquired
> > > blk's AioContext.
> > > 
> > > Normally blk->refcnt will be > 1 so bdrv_drain() won't be called, but
> > > I'm not sure if that can be guaranteed.
> > > 
> > > The following seems safer although it's uglier:
> > > 
> > >   aio_context = bdrv_get_aio_context(bs);
> > >   aio_context_acquire(aio_context);
> > >   blk_unref(blk);
> > >   aio_context_release(aio_context);
> > 
> > May we actually acquire aio_context if blk is in the main thread? I
> > think we must only do this if it's in a different iothread because we'd
> > end up with a recursive lock and drain would hang.
> 
> Right :). Maybe an aio_context_acquire_once() API would help.

If you want it to work in the general case, how would you implement
this? As far as I know there is no way to tell whether we already own
the lock or not.

Something like aio_context_acquire_unless_self() might be easier to
implement.

Kevin

signature.asc
Description: PGP signature