Re: [PATCH v10 5/9] copy-on-read: limit guest COR activity to base in COR driver

[Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>]

On 05.10.2020 17:58, Vladimir Sementsov-Ogievskiy wrote:
> 29.09.2020 15:38, Andrey Shinkevich wrote:
>> Limit the guest's COR operations by the base node in the backing chain
>> when the base node name is given. It will be useful for a block stream
>> job when the COR-filter is applied.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/copy-on-read.c | 38 ++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index e04092f..f53f7e0 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -121,8 +121,42 @@ static int coroutine_fn 
>> cor_co_preadv_part(BlockDriverState *bs,
>>                                              size_t qiov_offset,
>>                                              int flags)
>>   {
>> -    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, 
>> qiov_offset,
>> -                               flags | BDRV_REQ_COPY_ON_READ);
>> +    int64_t n = 0;
>> +    int64_t size = offset + bytes;
>> +    int local_flags;
>> +    int ret;
>> +    BDRVStateCOR *state = bs->opaque;
>> +
>> +    if (!state->base_bs) {
>> +        return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, 
>> qiov_offset,
>> +                                   flags | BDRV_REQ_COPY_ON_READ);
>> +    }
>> +
>> +    while (offset < size) {
>> +        local_flags = flags;
>> +
>> +        /* In case of failure, try to copy-on-read anyway */
> 
> But you add the flag only in case of success.. On any failure of furhter 
> is*allocated calls we should set the flag.
> 

Actually, myself would prefer returning the error instead.

Andrey

>> +        ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
>> +        if (!ret) {
>> +            ret = bdrv_is_allocated_above(bdrv_cow_bs(bs->file->bs),
>> +                                          state->base_bs, false, 
>> offset, n, &n);
>> +            if (ret > 0) {
>> +                local_flags |= BDRV_REQ_COPY_ON_READ;
>> +            }
>> +        }
>> +
>> +        ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, 
>> qiov_offset,
>> +                                  local_flags);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +
>> +        offset += n;
>> +        qiov_offset += n;
>> +        bytes -= n;
>> +    }
>> +
>> +    return 0;
>>   }
>>
> 
>