Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change

From: Kevin Wolf <kwolf@redhat.com>
To: Fam Zheng <famz@redhat.com>
Cc: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org, mreitz@redhat.com,
	stefanha@redhat.com, pbonzini@redhat.com, den@openvz.org,
	jsnow@redhat.com
Subject: Re: [Qemu-devel] [PATCH] blk: fix aio context loss on media change
Date: Wed, 15 Mar 2017 13:06:56 +0100	[thread overview]
Message-ID: <20170315120656.GI4030@noname.str.redhat.com> (raw)
In-Reply-To: <20170315111445.GE3088@lemon.lan>

Am 15.03.2017 um 12:14 hat Fam Zheng geschrieben:
> On Wed, 03/15 12:03, Kevin Wolf wrote:
> > Am 14.03.2017 um 18:11 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > If we have separate iothread for cdrom, we lose connection to it on
> > > qmp_blockdev_change_medium, as aio_context is on bds which is dropped
> > > and switched with new one.
> > > 
> > > As an example result, after such media change we have crash on
> > > virtio_scsi_ctx_check: Assertion `blk_get_aio_context(d->conf.blk) == s->ctx' failed.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > > ---
> > > 
> > > Hi all!
> > > 
> > > We've faced into this assert, and there some kind of fix. I don't sure that
> > > such fix doesn't break some conceptions, in this case, I hope, someone will
> > > propose a true-way solution.
> > 
> > The "true way" would be proper AioContext management in the sense that
> > all users of a BDS can specify a specific AioContext that they need and
> > if they all agree, callbacks are invoked to change everyone to that
> > AioContext. If they conflict, attaching the new user would have to error
> > out.
> > 
> > But we discussed this earlier, and while I'm not completely sure any
> > more about the details, I seem to remeber that Paolo said something
> > along the lines that AioContext is going away anyway and building the
> > code for proper management would be wasted time.
> 
> Matches my impression.
> 
> > 
> > Stefan, Paolo, do you remember the details why we didn't even do a
> > simple fix like the one below? I think there were some patches on the
> > list, no?
> 
> ISTM the concern was mostly "what about other BB in the graph?"
> 
> Should the new op blocker API be used in this one (a new type of perm)?

If we know what operation to block, that's an option. Would "change the
node's AioContext" work for it?

I think it would effectively mean that you need to attach the device
first and then jobs etc. respect the AioContext, whereas the opposite
order breaks because they don't have callbacks to adjust the AioContext
after the fact.

This seems to match what's actually safe, so it might really be as easy
as this.

Kevin