[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 05/12] block: remove AioContext locking
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?... > - 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. > +++ b/migration/block.c > @@ -270,7 +270,6 @@ static int mig_save_device_bulk(QEMUFile *f, > BlkMigDevState *bmds) > > if (bmds->shared_base) { > qemu_mutex_lock_iothread(); > - aio_context_acquire(blk_get_aio_context(bb)); ... > @@ -313,22 +311,10 @@ static int mig_save_device_bulk(QEMUFile *f, > BlkMigDevState *bmds) > block_mig_state.submitted++; > blk_mig_unlock(); > > - /* We do not know if bs is under the main thread (and thus does > - * not acquire the AioContext when doing AIO) or rather under > - * dataplane. Thus acquire both the iothread mutex and the > - * AioContext. > - * > - * This is ugly and will disappear when we make bdrv_* thread-safe, > - * without the need to acquire the AioContext. > - */ > - qemu_mutex_lock_iothread(); > - aio_context_acquire(blk_get_aio_context(bmds->blk)); Will conflict, but with trivial resolution, with your other thread renaming things to qemu_bql_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. Reviewed-by: Eric Blake <eblake@xxxxxxxxxx> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |