Re: [PATCH 05/12] block: remove AioContext locking

[Stefan Hajnoczi <stefanha@xxxxxxxxxx>]

On Thu, Nov 30, 2023 at 03:31:37PM -0600, Eric Blake wrote:
> On Wed, Nov 29, 2023 at 02:55:46PM -0500, Stefan Hajnoczi wrote:
> > This is the big patch that removes
> > aio_context_acquire()/aio_context_release() from the block layer and
> > affected block layer users.
> > 
> > There isn't a clean way to split this patch and the reviewers are likely
> > the same group of people, so I decided to do it in one patch.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> > ---
> 
> > +++ b/block.c
> > @@ -7585,29 +7433,12 @@ void coroutine_fn bdrv_co_leave(BlockDriverState 
> > *bs, AioContext *old_ctx)
> >  
> >  void coroutine_fn bdrv_co_lock(BlockDriverState *bs)
> >  {
> > -    AioContext *ctx = bdrv_get_aio_context(bs);
> > -
> > -    /* In the main thread, bs->aio_context won't change concurrently */
> > -    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> > -
> > -    /*
> > -     * We're in coroutine context, so we already hold the lock of the main
> > -     * loop AioContext. Don't lock it twice to avoid deadlocks.
> > -     */
> > -    assert(qemu_in_coroutine());
> 
> Is this assertion worth keeping in the short term?...

Probably not because coroutine vs non-coroutine functions don't change
in this patch series, so it's unlikely that this will break.

> 
> > -    if (ctx != qemu_get_aio_context()) {
> > -        aio_context_acquire(ctx);
> > -    }
> > +    /* TODO removed in next patch */
> >  }
> 
> ...I guess I'll see in the next patch.
> 
> >  
> >  void coroutine_fn bdrv_co_unlock(BlockDriverState *bs)
> >  {
> > -    AioContext *ctx = bdrv_get_aio_context(bs);
> > -
> > -    assert(qemu_in_coroutine());
> > -    if (ctx != qemu_get_aio_context()) {
> > -        aio_context_release(ctx);
> > -    }
> > +    /* TODO removed in next patch */
> >  }
> 
> Same comment.
> 
> > +++ b/blockdev.c
> > @@ -1395,7 +1352,6 @@ static void 
> > external_snapshot_action(TransactionAction *action,
> >      /* File name of the new image (for 'blockdev-snapshot-sync') */
> >      const char *new_image_file;
> >      ExternalSnapshotState *state = g_new0(ExternalSnapshotState, 1);
> > -    AioContext *aio_context;
> >      uint64_t perm, shared;
> >  
> >      /* TODO We'll eventually have to take a writer lock in this function */
> 
> I'm guessing removal of the locking gets us one step closer to
> implementing this TODO at a later time?  Or is it now a stale comment?
> Either way, it doesn't affect this patch.

I'm not sure. Kevin can answer questions about the graph lock.

> > +++ b/tests/unit/test-blockjob.c
> 
> > -static void test_complete_in_standby(void)
> > -{
> 
> > @@ -531,13 +402,5 @@ int main(int argc, char **argv)
> >      g_test_add_func("/blockjob/cancel/standby", test_cancel_standby);
> >      g_test_add_func("/blockjob/cancel/pending", test_cancel_pending);
> >      g_test_add_func("/blockjob/cancel/concluded", test_cancel_concluded);
> > -
> > -    /*
> > -     * This test is flaky and sometimes fails in CI and otherwise:
> > -     * don't run unless user opts in via environment variable.
> > -     */
> > -    if (getenv("QEMU_TEST_FLAKY_TESTS")) {
> > -        g_test_add_func("/blockjob/complete_in_standby", 
> > test_complete_in_standby);
> > -    }
> 
> Looks like you ripped out this entire test, because it is no longer
> viable.  I might have mentioned it in the commit message, or squashed
> the removal of this test into the earlier 02/12 patch.

I have sent a separate patch to remove this test and once it's merged
this hunk will disappear this patch series:
https://lore.kernel.org/qemu-devel/20231127170210.422728-1-stefanha@xxxxxxxxxx/

Stefan

Attachment: signature.asc
Description: PGP signature