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 <stefa...@redhat.com>
> > ---
>
> > +++ 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-stefa...@redhat.com/
Stefan
signature.asc
Description: PGP signature
