Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/17] block-backend: Decrease in_flight only after callback

[Kevin Wolf]

Am 17.09.2018 um 17:59 hat Paolo Bonzini geschrieben:
> On 17/09/2018 14:53, Kevin Wolf wrote:
> >>> I think I can drop the ref/unref pair, but not the whole patch (whose
> >>> main point is reordering dec_in_flight vs. the AIO callback).
> >>
> >> You're right, though I think I did that on purpose back in the day.
> >> IIRC it was related to bdrv_drain, which might never complete if called
> >> from an AIO callback.
> > 
> > Hm... This seems to become a common pattern, it's the same as for the
> > job completion callbacks (only improved enough for the bug at hand to
> > disappear instead of properly fixed in "blockjob: Lie better in
> > child_job_drained_poll()").
> > 
> > Either you say there is no activity even though there is still a
> > callback pending, then bdrv_drain() called from elsewhere will return
> > too early and we get a bug. Or you say there is activity, then any
> > nested drain inside that callback will deadlock and we get a bug, too.
> > 
> > So I suppose we need some way to know which activities to ignore during
> > drain, depending on who is the caller? :-/
> 
> Some alternatives:
> 
> 1) anything that needs to do invoke I/O for callbacks must use inc/dec
> in flight manually.  Simplest but hard to assert though.  At least SCSI
> IDE are broken.
> 
> 2) callbacks cannot indirectly result in bdrv_drain.  Sounds easier
> since there are not many AIO callers anymore - plus it also seems easier
> to add debugging code for.
> 
> 3) we provide device callbacks for "am I busy" and invoke them from
> bdrv_drain's poll loop.
> 
> Just off the top of my head.  Not sure which is best.

I don't see how 1) and 3) can solve the problem. You still need to
declare something busy when someone else drains (so the drain doesn't
return too early) and not busy when it calls a nested drain (so that it
doesn't deadlock).

2) would obviously solve the problem, but I'm afraid it's not realistic
when you consider that block jobs happen _only_ in callbacks. That much
of this is disguised as coroutine code doesn't change the call chains.
You also can't use BHs to escape the AIO callback, because then the BH is
logically part of the operation that needs to be completed for an
external drain to return, so you would still have to call the job busy
and would deadlock in a nested drain in that BH. So you're back to
square one.

For devices, it might be more realistic (devices rarely have a reason to
drain a block node, much less in a request completion), but note that
"cannot indirectly result in a drain" forbids any kind of nested
aio_poll(). I'm not sure if we can audit all code to fulfill this
condition (or how to assert it in the code).

Kevin