Re: [PATCH v3 1/3] block/io: refactor coroutine wrappers

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

23.05.2020 00:33, Eric Blake wrote:
> On 5/22/20 11:19 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Most of coroutine wrappers already follow this notation:
> 
> s/of/of our/
> s/notation/convention/
> 
>>
>> We have coroutine_fn bdrv_co_<something>(<normal argument list>), which
>> is the core functions, and wrapper, which does polling loope is called
>> bdrv_<something>(<same argument list>).
> 
> We have 'coroutine_fn bdrv_co_<something>(<normal argument list>)' as the core function, and a wrapper 'bdrv_<something>(<same argument list>)' which does a polling loop.
> 
>>
>> The only outsiders are bdrv_prwv_co and bdrv_common_block_status_above
> 
> s/are/are the/
> 
>> wrappers. Let's refactor the to behave as the others, it simplifies
> 
> s/the/them/
> 
>> further conversion of coroutine wrappers.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c | 61 +++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 33 insertions(+), 28 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 121ce17a49..bd00a70b47 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -900,28 +900,32 @@ typedef struct RwCo {
>>       BdrvRequestFlags flags;
>>   } RwCo;
>> +static int coroutine_fn bdrv_co_prwv(BdrvChild *child, int64_t offset,
>> +                                     QEMUIOVector *qiov, bool is_write,
>> +                                     BdrvRequestFlags flags)
>> +{
>> +    if (is_write) {
>> +        return bdrv_co_pwritev(child, offset, qiov->size, qiov, flags);
>> +    } else {
>> +        return bdrv_co_preadv(child, offset, qiov->size, qiov, flags);
>> +    }
>> +}
>> +
> 
> If we're trying to avoid needless indirection, wouldn't it be simpler to quit trying to slam reads and writes through a single prwv function that then has to split back out, and instead make two separate coroutine wrappers, one for just reads, and the other for just writes, without having to mess with a 'bool is_write' parameter?

Yes, and it's simpler after the transformation than before. I even wanted to do it but forget.. Will do as a follow-up, or with next version.

> 
>>   static void coroutine_fn bdrv_rw_co_entry(void *opaque)
>>   {
> 
> That is, should we have bdrv_co_preadv_entry and bdrv_co_pwritev_entry instead of just one bdrv_rw_co_entry?
> 
> At any rate, the renames done here are mechanical enough that if we make further changes, it could be a separate commit.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir