From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55417) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bpRfX-0003dz-Fw for qemu-devel@nongnu.org; Wed, 28 Sep 2016 23:05:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bpRfV-0002pq-5B for qemu-devel@nongnu.org; Wed, 28 Sep 2016 23:05:54 -0400 Date: Thu, 29 Sep 2016 11:05:43 +0800 From: Fam Zheng Message-ID: <20160929030543.GD6412@lemon> References: <1474958276-7715-1-git-send-email-famz@redhat.com> <1474958276-7715-6-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [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: Max Reitz Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Kevin Wolf , qemu-stable@nongnu.org, stefanha@redhat.com, pbonzini@redhat.com On Wed, 09/28 19:47, Max Reitz wrote: > On 27.09.2016 08:37, Fam Zheng wrote: > > 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()); > > I don't really see the point behind this assertion. I know it's not > currently possible, but you are basically asserting that we do not move > a BDS tree directly from some non-main-loop context to another > non-main-loop context, which in theory sounds completely fine to me. > > Based on the "Write code for now and not for the future" rule, I'm fine > with this assertion if you can tell me what good it does us now. > > The only thing I can personally imagine is that it's a safeguard that we > don't try to place a BDS tree into some other AioContext while having > ignored that there are still some other BBs attached to it which don't > want to agree on that new AioContext. But I think that should rather be > fixed before patch 2, i.e. as I said we need an infrastructure which can > tell us beforehand (and without failing assertions) whether we can move > a certain BDS tree to some other context. > > So whether we can move a certain BB from some context to another depends > on what the frontend supports, I don't think there is a generic answer > we can implement here in the generic BB code. NBD for instance allows > any movement; but devices probably only allow movements they have > initiated themselves (e.g. dataplane will allow exactly what you > describe here with that assertion, and any other device will probably > not allow anything but the main loop). Indeed, you make me think this should be an op blocker (that applies on whole graph). > > Max > > > if (blk->public.throttle_state) { > > throttle_timers_detach_aio_context(&blk->public.throttle_timers); > > } > > > >