Re: [PATCH v8 5/7] copy-on-read: limit guest writes to base in COR driver

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

04.09.2020 15:50, Max Reitz wrote:
> On 28.08.20 18:52, Andrey Shinkevich wrote:
>> Limit the guest's COR operations by the base node in the backing chain
>> during a stream job.
> 
> I don’t understand.   Shouldn’t we limit the areas where we set the COR
> flag?
> 
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/copy-on-read.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index 1f858bb..ecbd1f8 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -57,6 +57,37 @@ static BlockDriverState *get_base_by_name(BlockDriverState *bs,
>>       return base_bs;
>>   }
>>   
>> +/*
>> + * Returns 1 if the block is allocated in a node between cor_filter_bs->file->bs
>> + * and the base_bs in the backing chain. Otherwise, returns 0.
>> + * The COR operation is allowed if the base_bs is not specified - return 1.
>> + * Returns negative errno on failure.
>> + */
>> +static int node_above_base(BlockDriverState *cor_filter_bs, uint64_t offset,
>> +                           uint64_t bytes)
>> +{
>> +    int ret;
>> +    int64_t dummy;
>> +    BlockDriverState *file = NULL;
>> +    BDRVStateCOR *state = cor_filter_bs->opaque;
>> +
>> +    if (!state->base_bs) {
>> +        return 1;
>> +    }
>> +
>> +    ret = bdrv_block_status_above(cor_filter_bs->file->bs, state->base_bs,
>> +                                  offset, bytes, &dummy, NULL, &file);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (file) {
> 
> Why check file and not the return value?
> 
>> +        return 1;
>> +    }
>> +
>> +    return 0;
> 
> “dummy” should really not be called that way, it should be evaluated
> whether it’s smaller than bytes.
> 
> First, [offset, offset + dummy) may not be allocated above the base –
> but [offset + dummy, offset + bytes) may be.  Then this function returns
> 0 here, even though there is something in that range that’s allocated.
> 
> Second, in that case we still shouldn’t return 1, but return the
> shrunken offset instead.  Or, if there are multiple distinct allocated
> areas, they should probably even all be copied separately.
> 
> 
> (But all of that of course only if this function is intended to be used
> to limit where we set the COR flag, because I don’t understand why we’d
> want to limit where something can be written.)
> 

Agree to all.

1. Write path shouldn't be changed: it's a copy-on-read filter.

2. On read we need is_allocated_above-driven loop, to insert the flag only to regions allocated above base
  (and other regions we read just without the flag, don't skip them). qiov_offset will help very well.

3. Like in many other places, let's ignore block-status errors (and just add the flag if block_status fails)

>> +}
>> +
>>   static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>                       Error **errp)
>>   {
>> @@ -153,6 +184,12 @@ static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
>>                                               QEMUIOVector *qiov,
>>                                               size_t qiov_offset, int flags)
>>   {
>> +    int ret = node_above_base(bs, offset, bytes);
>> +
>> +    if (!ret || ret < 0) {
>> +        return ret;
>> +    }
>> +
>>       return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
>>                                   flags);
>>   }
>> @@ -162,6 +199,12 @@ static int coroutine_fn cor_co_pwrite_zeroes(BlockDriverState *bs,
>>                                                int64_t offset, int bytes,
>>                                                BdrvRequestFlags flags)
>>   {
>> +    int ret = node_above_base(bs, offset, bytes);
>> +
>> +    if (!ret || ret < 0) {
>> +        return ret;
>> +    }
>> +
>>       return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
>>   }
>>   
>> @@ -178,6 +221,12 @@ static int coroutine_fn cor_co_pwritev_compressed(BlockDriverState *bs,
>>                                                     uint64_t bytes,
>>                                                     QEMUIOVector *qiov)
>>   {
>> +    int ret = node_above_base(bs, offset, bytes);
>> +
>> +    if (!ret || ret < 0) {
>> +        return ret;
>> +    }
>> +
>>       return bdrv_co_pwritev(bs->file, offset, bytes, qiov,
>>                              BDRV_REQ_WRITE_COMPRESSED);
>>   }
>>
> 
> 


-- 
Best regards,
Vladimir