Re: [PATCH 3/3] block-backend: delay application of request queuing

[Hanna Czenczek <hreitz@redhat.com>]

On 05.04.23 18:31, Paolo Bonzini wrote:
> Request queuing prevents new requests from being submitted to the
> BlockDriverState, not allowing them to start instead of just letting
> them complete before bdrv_drained_begin() returns.
>
> The reason for this was to ensure progress and avoid a livelock
> in blk_drain(), blk_drain_all_begin(), bdrv_drained_begin() or
> bdrv_drain_all_begin(), if there is an endless stream of requests to
> a BlockBackend.  However, this is prone to deadlocks.
>
> In particular, IDE TRIM wants to elevate its BB's in-flight counter for a
> "macro" operation that consists of several actual I/O operations.  Each of
> those operations is individually started and awaited.  It does this so
> that blk_drain() will drain the whole TRIM, and not just a single one
> of the many discard operations it may encompass.  When request queuing
> is enabled, this leads to a deadlock: The currently ongoing discard is
> drained, and the next one is queued, waiting for the drain to stop.
> Meanwhile, TRIM still keeps the in-flight counter elevated, waiting
> for all discards to stop -- which will never happen, because with the
> in-flight counter elevated, the BB is never considered drained, so the
> drained section does not begin and cannot end.
>
> Fixing the implementation of request queuing is hard to do in general,
> and even harder to do without adding more hacks.  As the name suggests,
> deadlocks are worse than livelocks :) so let's avoid them: turn the
> request queuing on only after the BlockBackend has quiesced, and leave
> the second functionality of bdrv_drained_begin() to the BQL or to the
> individual BlockDevOps implementations.
>
> In fact, devices such as IDE that run in the vCPU thread do not suffer
> from this livelock because they only submit requests while they are
> allowed to hold the big QEMU lock (i.e., not during bdrv_drained_begin()
> or bdrv_drain_all_begin().  Other devices can avoid it through external
> file descriptor (so that aio_disable_external() will prevent submission
> of new requests) or with a .drained_begin callback in their BlockDevOps.
>
> Note that this change does not affect the safety of bdrv_drained_begin(),
> since the patch does not completely get away with request queuing.
>
> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
> Fixes: 7e5cdb345f77 ("ide: Increment BB in-flight counter for TRIM BH")
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   block/block-backend.c | 40 ++++++++++++++++++++++++++++++----------
>   1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 10419f8be91e..acb4cb91a5ee 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -2600,7 +2610,14 @@ static bool blk_root_drained_poll(BdrvChild *child)
>       if (blk->dev_ops && blk->dev_ops->drained_poll) {
>           busy = blk->dev_ops->drained_poll(blk->dev_opaque);
>       }
> -    return busy || !!blk->in_flight;
> +    if (busy || blk->in_flight) {
> +        return true;
> +    }
> +
> +    if (qatomic_read(&blk->request_queuing) == BLK_QUEUE_READY) {
> +        qatomic_set(&blk->request_queuing, BLK_QUEUE_QUIESCENT);
> +    }
> +    return false;
>   }

This implicitly relies on nobody increasing blk->in_flight (or 
dev_ops->drained_poll() returning `true` again) while the BB is starting 
to be drained, because if the function were to be called again after it 
has returned `false` once per drained section (not sure if that’s 
possible![1]), then we’d end up in the original situation, with 
in_flight elevated and queuing enabled.

Is that really strictly guaranteed somehow or is it rather a complex 
conglomerate of many cases that in the end happen to work out 
individually?  I mean, I could imagine that running 
BlockDevOps.drained_begin() is supposed to guarantee that, but it can’t, 
because only NBD seems to implement it.  The commit message talks about 
IDE being fine (by accident?) because it needs BQL availability to 
submit new requests.  But that’s very complex and I’d rather have a 
strict requirement to guarantee correctness.

[1] If the blk_root_drained_poll() isn’t called anymore after returning 
`false`, all will be good, but I assume it will be, because we have a 
quiesce_counter, not a quiesce_bool.  We could kind of emulate this by 
continuing to return `false` after blk_root_drained_poll() has returned 
`false` once, until the quiesce_counter becomes 0.

We could also have blk_root_drained_poll(), if it sees in_flight > 0 && 
request_queuing == BLK_QUEUE_QUIESCENT, revert request_queuing to 
BLK_QUEUE_READY and resume all queued requests.

But, admittedly, I’m making a lot of assumptions and leaps by this 
point.  It all hinges on whether we can guarantee that in_flight won’t 
be increased while a drained section starts.

Hanna