Re: [PATCH v2 6/9] block/io: expand in_flight inc/dec section: block-status

[Kevin Wolf <kwolf@redhat.com>]

Am 02.05.2020 um 00:00 hat Eric Blake geschrieben:
> On 4/27/20 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> > It's safer to expand in_flight request to start before enter to
> > coroutine in synchronous wrappers and end after BDRV_POLL_WHILE loop.
> > Note that qemu_coroutine_enter may only schedule the coroutine in some
> > circumstances.
> 
> Wording suggestion:
> 
> It's safer to expand the region protected by an in_flight request to begin
> in the synchronous wrapper and end after the BDRV_POLL_WHILE loop.  Leaving
> the in_flight request in the coroutine itself risks a race where calling
> qemu_coroutine_enter() may have only scheduled, rather than started, the
> coroutine, allowing some other thread a chance to not realize an operation
> is in flight.
> 
> > 
> > block-status requests are complex, they involve querying different
> > block driver states across backing chain. Let's expand only in_flight
> > section for the top bs, keeping other sections as is.
> 
> block-status requests are complex, involving a query of different block
> driver states across the backing chain.  Let's expand only the in_flight
> section for the top bs, and keep the other sections as-is.
> 
> I'd welcome Kevin's review on my next comment, but if I'm correct, I think
> we can further add the following justification to the commit message:
> 
> Gathering block status only requires reads from the block device, and
> backing devices are typically read-only, so losing any in_flight race on a
> backing device is less likely to cause problems with concurrent
> modifications on the overall backing chain.

Actually, my question is what we gain by increasing in_flight only for
the top level. It feels wrong to me, though maybe it doesn't actually
lead to bugs because in practice, we completely drain the parents
instead of just draining requests going to one specific child.

But as this patch shows, not increasing in_flight in some cases is a lot
more work than doing it, and it's harder to understand why it's correct.
So why not simply increase it unconditionally?

This is how other requests work as well. If you make a read request to a
qcow2 image, you'll get in_flight increased for both the qcow2 node and
the file-posix node.

Kevin