Re: [PATCH for-5.0 2/2] block: Fix blk->in_flight during blk_wait_while_drained()

[Max Reitz]

On 03.04.20 16:50, Kevin Wolf wrote:
> Am 03.04.2020 um 14:42 hat Max Reitz geschrieben:
>> On 03.04.20 12:44, Kevin Wolf wrote:
>>> Calling blk_wait_while_drained() while blk->in_flight is increased for
>>> the current request is wrong because it will cause the drain operation
>>> to deadlock.
>>>
>>> Many callers of blk_wait_while_drained() have already increased
>>> blk->in_flight when called in a blk_aio_*() path, but can also be called
>>> in synchonous code paths where blk->in_flight isn't increased. This
>>> means that these calls of blk_wait_while_drained() are wrong at least in
>>> some cases.
>>>
>>> In order to fix this, increase blk->in_flight even for synchronous
>>> operations and temporarily decrease the counter again in
>>> blk_wait_while_drained().
>>>
>>> Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
>>> Signed-off-by: Kevin Wolf <address@hidden>
>>> ---
>>>  block/block-backend.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> blk_co_pdiscard() and blk_co_flush() are called from outside of
>> block-backend.c (namely from mirror.c and nbd/server.c).  Is that OK?
> 
> Hm... I think you're right that the NBD server has a problem now because
> we might now decrease blk->in_flight without having increased it.
> (Mirror should be fine anyway because it sets disable_request_queuing.)
> 
> At first I was going to suggest that we could do the opposite of this
> patch and just move the dec/wait/inc sequence (which this patch removes
> for read/write) to all coroutine entry functions, so direct calls
> wouldn't incorrectly decrease the counter.
> 
> But this is not what we want either, we do want to queue requests for
> drained BlockBackends even in the blk_co_*() API.
> 
> Do you have another idea or do we have to turn blk_co_*() into wrappers
> around the existing functions, which would gain an additional bool
> parameter that tells whether we need to dec/inc or not?

So that whenever blk_co_* is called from outside of block-backend.c, we
don’t dec/inc?

Sounds reasonable to me.

The only alternative I see would be ensuring we call
blk_wait_while_drained() only outside of in_flight sections (without
having to dec/inc around it).  But we can’t call it in synchronous
sections.  And for those synchronous calls, we also have to wrap the
in_flight section around the whole asynchronous boilerplate, so there is
no place where they can call bdrv_wait_while_drained() without dec/inc
around it.

So I can’t think of another way either.

Max

signature.asc
Description: OpenPGP digital signature