[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH for-5.0 v2 3/3] block: Fix blk->in_flight during blk_wait_whi
From: |
Kevin Wolf |
Subject: |
Re: [PATCH for-5.0 v2 3/3] block: Fix blk->in_flight during blk_wait_while_drained() |
Date: |
Tue, 7 Apr 2020 10:59:15 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 <address@hidden>
> > ---
> > 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 <address@hidden>
Thanks, and I hope you agree with blk->in_flight > 0 now.
Kevin
Re: [PATCH for-5.0 v2 2/3] block: Increase BB.in_flight for coroutine interfaces, Max Reitz, 2020/04/07