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. Stefan
signature.asc
Description: PGP signature