From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57407) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bom24-0004Sp-S3 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 02:38:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bom23-0003fu-QD for qemu-devel@nongnu.org; Tue, 27 Sep 2016 02:38:24 -0400 From: Fam Zheng Date: Tue, 27 Sep 2016 14:37:56 +0800 Message-Id: <1474958276-7715-6-git-send-email-famz@redhat.com> In-Reply-To: <1474958276-7715-1-git-send-email-famz@redhat.com> References: <1474958276-7715-1-git-send-email-famz@redhat.com> Subject: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , qemu-stable@nongnu.org, Max Reitz , stefanha@redhat.com, pbonzini@redhat.com From: Stefan Hajnoczi blk_get/set_aio_context() delegate to BlockDriverState without storing the AioContext pointer in BlockBackend. There are two flaws: 1. BlockBackend falls back to the QEMU main loop AioContext when there is no root BlockDriverState. This means the drive loses its AioContext during media change and would break dataplane. 2. BlockBackend state used from multiple threads has no lock. Race conditions will creep in as functionality is moved from BlockDriverState to BlockBackend due to the absense of a lock. The monitor cannot access BlockBackend state safely while an IOThread is also accessing the state. Issue #1 can be triggered by "change" on virtio-scsi dataplane, causing a assertion failure (virtio-blk is fine because medium change is not possible). #2 may be possible with block accounting statistics in BlockBackend but I'm not aware of a crash that can be triggered. This patch stores the AioContext pointer in BlockBackend and puts newly inserted BlockDriverStates into the AioContext. Signed-off-by: Stefan Hajnoczi Signed-off-by: Fam Zheng --- block/block-backend.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/block/block-backend.c b/block/block-backend.c index b71babe..cda67cc 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -31,6 +31,7 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb); struct BlockBackend { char *name; int refcnt; + AioContext *aio_context; BdrvChild *root; DriveInfo *legacy_dinfo; /* null unless created by drive_new() */ QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */ @@ -121,6 +122,7 @@ static BlockBackend *blk_new_with_ctx(AioContext *ctx) blk = g_new0(BlockBackend, 1); blk->refcnt = 1; + blk->aio_context = ctx; blk_set_enable_write_cache(blk, true); qemu_co_queue_init(&blk->public.throttled_reqs[0]); @@ -510,6 +512,8 @@ void blk_remove_bs(BlockBackend *blk) void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs) { bdrv_ref(bs); + + assert(blk->aio_context == bdrv_get_aio_context(bs)); blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk); notifier_list_notify(&blk->insert_bs_notifiers, blk); @@ -1413,13 +1417,7 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason) AioContext *blk_get_aio_context(BlockBackend *blk) { - BlockDriverState *bs = blk_bs(blk); - - if (bs) { - return bdrv_get_aio_context(bs); - } else { - return qemu_get_aio_context(); - } + return blk->aio_context; } static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb) @@ -1432,7 +1430,19 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context) { BlockDriverState *bs = blk_bs(blk); + blk->aio_context = new_context; + if (bs) { + AioContext *ctx = bdrv_get_aio_context(bs); + + if (ctx == new_context) { + return; + } + /* Moving context around happens when a block device is + * enabling/disabling data plane, in which case we own the root BDS and + * it cannot be associated with another AioContext. */ + assert(ctx == qemu_get_aio_context() || + new_context == qemu_get_aio_context()); if (blk->public.throttle_state) { throttle_timers_detach_aio_context(&blk->public.throttle_timers); } -- 2.7.4