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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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