Re: [PATCH v12 11/14] copy-on-read: add support for read flags to COR-filter

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

27.10.2020 17:46, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2020 21:13, Andrey Shinkevich wrote:
>> Add the BDRV_REQ_COPY_ON_READ and BDRV_REQ_PREFETCH flags to the
>> supported_read_flags of the COR-filter.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   block/copy-on-read.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>> index 8178a91..a2b180a 100644
>> --- a/block/copy-on-read.c
>> +++ b/block/copy-on-read.c
>> @@ -50,6 +50,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
>>           return -EINVAL;
>>       }
>> +    bs->supported_read_flags = BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH;
>> +
>>       bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
>>           (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
>>
> 
> This should be merged with the following patch, otherwise it doesn't make sense. You mark filter as supporting PREFETCH, but actually it just ignores it (and may crash on trying to read into qiov=NULL).
> 

Ah, no, problem is not in qiov=NULL, but in that we will just pass PREFETCH to bs->file, which may not support it and crash in block.io in the new abort() from patch 10.


Also, any reason to add support for BDRV_REQ_COPY_ON_READ ? What it means for cor filter? I don't know. It make sense only for generic layer and handled in generic layer. It never passed to driver, so let's not declare support for it.

-- 
Best regards,
Vladimir