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

[Kevin Wolf <kwolf@redhat.com>]

Am 07.04.2020 um 08:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 06.04.2020 20:14, Kevin Wolf wrote:
> > Waiting in 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.
> > 
> > This patch makes sure that blk_wait_while_drained() is called with
> > blk->in_flight increased exactly once for the current request, and that
> > it temporarily decreases the counter while it waits.
> > 
> > Fixes: cf3129323f900ef5ddbccbe86e4fa801e88c566e
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/block-backend.c | 17 +++++------------
> >   1 file changed, 5 insertions(+), 12 deletions(-)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index d330e08b05..f621435f0b 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -1140,10 +1140,15 @@ static int blk_check_byte_request(BlockBackend *blk, int64_t offset,
> >       return 0;
> >   }
> > +/* To be called between exactly one pair of blk_inc/dec_in_flight() */
> >   static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)
> >   {
> > +    assert(blk->in_flight > 0);
> 
> Hmm. You promise to make sure that in_flight increased exactly once.
> Shouldn't it be assert(blk->in_flight == 1) ?

Exactly once for this specific request, but if you have multiple
requests in flight, blk->in_flight will be the sum of all requests.

Just asserting > 0 should still catch potential bugs because you won't
always have multiple requests in flight.

> > +
> >       if (blk->quiesce_counter && !blk->disable_request_queuing) {
> > +        blk_dec_in_flight(blk);
> >           qemu_co_queue_wait(&blk->queued_requests, NULL);
> > +        blk_inc_in_flight(blk);
> >       }
> >   }
> > @@ -1416,12 +1421,6 @@ static void blk_aio_read_entry(void *opaque)
> >       BlkRwCo *rwco = &acb->rwco;
> >       QEMUIOVector *qiov = rwco->iobuf;
> > -    if (rwco->blk->quiesce_counter) {
> > -        blk_dec_in_flight(rwco->blk);
> > -        blk_wait_while_drained(rwco->blk);
> > -        blk_inc_in_flight(rwco->blk);
> > -    }
> 
> Hm, you drop it as it's called from blk_do_preadv too. I think it
> worth mentioning in commit message still.

Okay, I can add a sentence like "The blk_wait_while_drained() call in
blk_aio_read/write_entry is redundant with the one in blk_co_*(), so
drop it."

> > -
> >       assert(qiov->size == acb->bytes);
> >       rwco->ret = blk_do_preadv(rwco->blk, rwco->offset, acb->bytes,
> >                                 qiov, rwco->flags);
> > @@ -1434,12 +1433,6 @@ static void blk_aio_write_entry(void *opaque)
> >       BlkRwCo *rwco = &acb->rwco;
> >       QEMUIOVector *qiov = rwco->iobuf;
> > -    if (rwco->blk->quiesce_counter) {
> > -        blk_dec_in_flight(rwco->blk);
> > -        blk_wait_while_drained(rwco->blk);
> > -        blk_inc_in_flight(rwco->blk);
> > -    }
> > -
> >       assert(!qiov || qiov->size == acb->bytes);
> >       rwco->ret = blk_do_pwritev_part(rwco->blk, rwco->offset, acb->bytes,
> >                                       qiov, 0, rwco->flags);
> > 
> 
> With assert(blk->in_flight == 1) and mention extra wait removing in commit message:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks, and I hope you agree with blk->in_flight > 0 now.

Kevin