Re: [PATCH 5/9] block/io: expand in_flight inc/dec section: simple cases

[Vladimir Sementsov-Ogievskiy]

20.04.2020 19:22, Stefan Hajnoczi wrote:
> On Wed, Apr 08, 2020 at 12:30:47PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > It's safer to expand in_flight request to start before enter to
>
> Please explain what exeactly "safer" means.  If I understand correctly
> this is just a refactoring and does not fix bugs that have been hit in
> the real world.
>
> Is this just a generate attempt to avoid accidentally performing
> operations that need to happen as part of the request after the dec
> call?

Consider write.

It's possible, that qemu_coroutine_enter only schedules execution, assume such 
case.
Then we may possibly have the following:

1. Somehow check that we are not in drained section in outer code

2. call bdrv_pwritev(), assuming that it will increse in_flight, which will 
protect us from starting drained section

3. it calls bdrv_prwv_co -> bdrv_coroutine_enter (not yet increased in_flight)

4. assume coroutine not yet actually entered, only scheduled, and we go to some 
code, which starts drained section (as in_flight is zero)

5. scheduled coroutine starts, and blindly increases in_flight, and we are in 
drained section with in_flight request.

The series does the same thing for block/io.c like Kevin's "block: Fix 
blk->in_flight during blk_wait_while_drained()" for blk layer.

> > @@ -2718,17 +2746,18 @@ bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector 
> > *qiov, int64_t pos,
> >               ret = drv->bdrv_save_vmstate(bs, qiov, pos);
> >           }
> >       } else if (bs->file) {
> > -        ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
> > +        bdrv_inc_in_flight(bs->file->bs);
> > +        ret = bdrv_do_rw_vmstate(bs->file->bs, qiov, pos, is_read);
> > +        bdrv_dec_in_flight(bs->file->bs);
>
> Here we inc/dec...
>
> >       }
> >   
> > -    bdrv_dec_in_flight(bs);
> >       return ret;
> >   }
> >   
> >   static void coroutine_fn bdrv_co_rw_vmstate_entry(void *opaque)
> >   {
> >       BdrvVmstateCo *co = opaque;
> > -    co->ret = bdrv_co_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
> > +    co->ret = bdrv_do_rw_vmstate(co->bs, co->qiov, co->pos, co->is_read);
>
> ...here we don't.  The code is correct, but bdrv_co_rw_vmstate_entry()
> should also document that its caller must inc/dec.
>
> > @@ -2950,7 +2994,7 @@ static void coroutine_fn bdrv_flush_co_entry(void *opaque)
> >   {
> >       FlushCo *rwco = opaque;
> >   
> > -    rwco->ret = bdrv_co_flush(rwco->bs);
> > +    rwco->ret = bdrv_do_flush(rwco->bs);
> >       aio_wait_kick();
> >   }
>
> This function should also document that the caller must inc/dec.


--
Best regards,
Vladimir