[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 05/12] block: remove AioContext locking
From: |
Eric Blake |
Subject: |
Re: [PATCH 05/12] block: remove AioContext locking |
Date: |
Thu, 30 Nov 2023 15:31:37 -0600 |
User-agent: |
NeoMutt/20231103 |
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@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?...
> - 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@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
- [PATCH 03/12] aio: make aio_context_acquire()/aio_context_release() a no-op, (continued)
- [PATCH 03/12] aio: make aio_context_acquire()/aio_context_release() a no-op, Stefan Hajnoczi, 2023/11/29
- [PATCH 02/12] tests: remove aio_context_acquire() tests, Stefan Hajnoczi, 2023/11/29
- [PATCH 01/12] virtio-scsi: replace AioContext lock with tmf_bh_lock, Stefan Hajnoczi, 2023/11/29
- [PATCH 04/12] graph-lock: remove AioContext locking, Stefan Hajnoczi, 2023/11/29
- [PATCH 05/12] block: remove AioContext locking, Stefan Hajnoczi, 2023/11/29
- [PATCH 06/12] scsi: remove AioContext locking, Stefan Hajnoczi, 2023/11/29
- [PATCH 08/12] aio: remove aio_context_acquire()/aio_context_release() API, Stefan Hajnoczi, 2023/11/29
- [PATCH 07/12] aio-wait: draw equivalence between AIO_WAIT_WHILE() and AIO_WAIT_WHILE_UNLOCKED(), Stefan Hajnoczi, 2023/11/29
- [PATCH 09/12] docs: remove AioContext lock from IOThread docs, Stefan Hajnoczi, 2023/11/29
- [PATCH 10/12] scsi: remove outdated AioContext lock comment, Stefan Hajnoczi, 2023/11/29