Re: [PATCH for-5.0 v3 2/3] block: Increase BB.in_flight for coroutine and sync interfaces

[Kevin Wolf]

Am 07.04.2020 um 16:56 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 07.04.2020 17:42, Kevin Wolf wrote:
> > Am 07.04.2020 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > > 07.04.2020 15:12, Kevin Wolf wrote:
> > > > External callers of blk_co_*() and of the synchronous blk_*() functions
> > > > don't currently increase the BlockBackend.in_flight counter, but calls
> > > > from blk_aio_*() do, so there is an inconsistency whether the counter
> > > > has been increased or not.
> > > > 
> > > > This patch moves the actual operations to static functions that can
> > > > later know they will always be called with in_flight increased exactly
> > > > once, even for external callers using the blk_co_*() coroutine
> > > > interfaces.
> > > > 
> > > > If the public blk_co_*() interface is unused, remove it.
> > > > 
> > > > Signed-off-by: Kevin Wolf <address@hidden>
> > > 
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > 
> > > side question:
> > > 
> > > Should we inc/dec in blk_make_zero, blk_truncate?
> > 
> > I don't think it's necessary. They call into their bdrv_* counterpart
> > immediately, so the node-level counter should be enough.
> > 
> 
> bdrv_make_zero is not one request, it does block_status/pwrite_zeroes
> in a loop. So drained section may occur during bdrv_make_zero.
> Possibly, nothing bad in it?

It would potentially be a problem if it were called in coroutine
context. But it's a synchronous function that must be called in the main
thread (and also only used in qemu-img), so I don't see how drain could
happen while it runs.

If we did want to make it safe for use in coroutine context, it would be
by using bdrv_inc/dec_in_flight() in bdrv_make_zero().

> blk_truncate may do coroutine_enter before incrementing node-level
> counter, which may only schedule it..

This is bdrv_truncate(), not blk_truncate(). If you address it in
blk_truncate(), you miss the direct callers of bdrv_truncate().

But you're right that it could potentially be a problem. Not sure if it
really is, but maybe better safe than sorry, so if you want to send a
patch, go ahead.

Kevin