Re: [RFC PATCH 1/3] block: Factor out bdrv_run_co()

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 5/12/20 5:37 PM, Eric Blake wrote:
> On 5/12/20 9:43 AM, Kevin Wolf wrote:
>> We have a few bdrv_*() functions that can either spawn a new coroutine
>> and wait for it with BDRV_POLL_WHILE() or use a fastpath if they are
>> alreeady running in a coroutine. All of them duplicate basically the
> 
> already
> 
>> same code.
>>
>> Factor the common code into a new function bdrv_run_co().
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/io.c | 104 +++++++++++++++--------------------------------------
>>   1 file changed, 28 insertions(+), 76 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 7d30e61edc..c1badaadc9 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -891,6 +891,22 @@ static int 
>> bdrv_check_byte_request(BlockDriverState *bs, int64_t offset,
>>       return 0;
>>   }
>> +static int bdrv_run_co(BlockDriverState *bs, CoroutineEntry *entry,
>> +                       void *opaque, int *ret)
>> +{
>> +    if (qemu_in_coroutine()) {
>> +        /* Fast-path if already in coroutine context */
>> +        entry(opaque);
>> +    } else {
>> +        Coroutine *co = qemu_coroutine_create(entry, opaque);
>> +        *ret = NOT_DONE;
>> +        bdrv_coroutine_enter(bs, co);
>> +        BDRV_POLL_WHILE(bs, *ret == NOT_DONE);
> 
> For my reference, NOT_DONE is defined as INT_MAX, which does not seem to 
> be used as a return value in other situations.
> 
>> @@ -923,25 +939,15 @@ static int bdrv_prwv_co(BdrvChild *child, 
>> int64_t offset,
>>                           QEMUIOVector *qiov, bool is_write,
>>                           BdrvRequestFlags flags)
>>   {
>> -    Coroutine *co;
>>       RwCo rwco = {
>>           .child = child,
>>           .offset = offset,
>>           .qiov = qiov,
>>           .is_write = is_write,
>> -        .ret = NOT_DONE,
>>           .flags = flags,
>>       };
>> -    if (qemu_in_coroutine()) {
>> -        /* Fast-path if already in coroutine context */
>> -        bdrv_rw_co_entry(&rwco);
>> -    } else {
>> -        co = qemu_coroutine_create(bdrv_rw_co_entry, &rwco);
>> -        bdrv_coroutine_enter(child->bs, co);
>> -        BDRV_POLL_WHILE(child->bs, rwco.ret == NOT_DONE);
>> -    }
>> -    return rwco.ret;
>> +    return bdrv_run_co(child->bs, bdrv_rw_co_entry, &rwco, &rwco.ret);
> 
> So code that previously looped on NOT_DONE is obviously safe, while...
> 
>>   }
>>   int bdrv_pwrite_zeroes(BdrvChild *child, int64_t offset,
>> @@ -2230,7 +2236,6 @@ typedef struct BdrvCoBlockStatusData {
>>       int64_t *map;
>>       BlockDriverState **file;
>>       int ret;
>> -    bool done;
>>   } BdrvCoBlockStatusData;
>>   int coroutine_fn bdrv_co_block_status_from_file(BlockDriverState *bs,
>> @@ -2492,7 +2497,6 @@ static void coroutine_fn 
>> bdrv_block_status_above_co_entry(void *opaque)
>>                                              data->want_zero,
>>                                              data->offset, data->bytes,
>>                                              data->pnum, data->map, 
>> data->file);
>> -    data->done = true;
>>       aio_wait_kick();
> 
> ...code that looped on something else now has to be checked that 
> data->ret is still being set to something useful.  Fortunately that is 
> true here.
> 
>> @@ -2669,22 +2663,13 @@ static inline int
>>   bdrv_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
>>                   bool is_read)
>>   {
>> -    if (qemu_in_coroutine()) {
>> -        return bdrv_co_rw_vmstate(bs, qiov, pos, is_read);
>> -    } else {
>> -        BdrvVmstateCo data = {
>> -            .bs         = bs,
>> -            .qiov       = qiov,
>> -            .pos        = pos,
>> -            .is_read    = is_read,
>> -            .ret        = -EINPROGRESS,
>> -        };
>> -        Coroutine *co = 
>> qemu_coroutine_create(bdrv_co_rw_vmstate_entry, &data);
>> -
>> -        bdrv_coroutine_enter(bs, co);
>> -        BDRV_POLL_WHILE(bs, data.ret == -EINPROGRESS);
>> -        return data.ret;
> 
> It's a little harder to see whether -EINPROGRESS might ever be returned 
> by a driver, but again this looks safe.

Maybe add a comment regarding -EINPROGRESS before calling bdrv_run_co() 
in bdrv_rw_vmstate()?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Here, it's a little less obvious whether any driver might return 
> -EINPROGRESS, but it looks like if they did that,
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Conflicts with Vladimir's patches which try to add more coroutine 
> wrappers (but those need a rebase anyway):
> https://lists.gnu.org/archive/html/qemu-devel/2020-04/msg04559.html
>