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