On Wed, 09/28 19:47, Max Reitz wrote:
> On 27.09.2016 08:37, Fam Zheng wrote:
> > From: Stefan Hajnoczi <stefa...@redhat.com>
> >
> > 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 <stefa...@redhat.com>
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > 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);
> > }
> >
>
>
