Re: [PATCH v10 6/9] copy-on-read: skip non-guest reads if no copy needed

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

07.10.2020 22:01, Andrey Shinkevich wrote:
> 
> On 07.10.2020 13:06, Vladimir Sementsov-Ogievskiy wrote:
>> 29.09.2020 15:38, Andrey Shinkevich wrote:
>>> If the flag BDRV_REQ_PREFETCH was set, pass it further to the
>>> COR-driver to skip unneeded reading. It can be taken into account for
>>> the COR-algorithms optimization. That check is being made during the
>>> block stream job by the moment.
>>>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   block/copy-on-read.c | 14 ++++++++++----
>>>   block/io.c           |  2 +-
>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>> index f53f7e0..5389dca 100644
>>> --- a/block/copy-on-read.c
>>> +++ b/block/copy-on-read.c
>>> @@ -145,10 +145,16 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
>>>               }
>>>           }
>>> -        ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
>>> -                                  local_flags);
>>> -        if (ret < 0) {
>>> -            return ret;
>>> +        if ((flags & BDRV_REQ_PREFETCH) &
>>
>> BDRV_REQ_PREFETCH is documented to be only used with BDRV_REQ_COPY_ON_READ. But here
>> BDRV_REQ_COPY_ON_READ appears intermediately. We should change documentation in block.h
>> in a separate patch (and probably code in bdrv_aligned_preadv())
>>
> 
> OK, we will come here without the BDRV_REQ_PREFETCH flag set.

flag BDRV_REQ_PREFETCH should be set in stream job. Where should it be handled, I don't follow?

> To differ between guest reads and the stream job ones, we would set it here by checking for the qiov NULL pointer:
> 
> 
> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
> index 4e3b1c5..df2c2ab 100644
> --- a/block/copy-on-read.c
> +++ b/block/copy-on-read.c
> @@ -144,6 +144,9 @@ static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
>                                             n, &n);
>               if (ret) {
>                   local_flags |= BDRV_REQ_COPY_ON_READ;
> +                if (!qiov) {
> +                    local_flags |= BDRV_REQ_PREFETCH;

if qiov is NULL, this means that flags must include BDRV_REQ_PREFETCH. local_flags should inherit flags I think.

> +                }
>               }
>           }
> 
> Andrey
> 
>>> +            !(local_flags & BDRV_REQ_COPY_ON_READ)) {
>>> +            /* Skip non-guest reads if no copy needed */
>>> +        } else {
>>> +
>>
>> extra new-line ?
>>
>>> +            ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
>>> +                                      local_flags);
>>> +            if (ret < 0) {
>>> +                return ret;
>>> +            }
>>>           }
>>>           offset += n;
>>> diff --git a/block/io.c b/block/io.c
>>> index 11df188..62b75a5 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -1388,7 +1388,7 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
>>>               qemu_iovec_init_buf(&local_qiov, bounce_buffer, pnum);
>>>               ret = bdrv_driver_preadv(bs, cluster_offset, pnum,
>>> -                                     &local_qiov, 0, 0);
>>> +                                     &local_qiov, 0, flags & BDRV_REQ_PREFETCH);
>>
>> Why? In this place we want to read. We'll write back the data a few lines below. What will we write,
>> if underlying driver decide to do nothing because of BDRV_REQ_PREFETCH?
>>
> 
> See my comment above please.

Anyway, BDRV_REQ_PREFETCH here is wrong. You should not pass any qiov, if you set BDRV_REQ_PREFETCH flag.

If we come to bdrv_co_do_copy_on_readv, it means that we have COPY_ON_READ flag. And therefore, we will handle
PREFETCH and COPY_ON_READ here in generic layer. And therefore, we shouldn't pass them to driver.

On the contrary, if we have PREFETCH flag in bdrv_co_aligned_preadv, but don't have COPY_ON_READ in the same time,
this means that we must pass PREFETCH flag to the driver if it supports it. And do nothing if driver
doesn't support PREFETCH. That's how I see it.

> 
>>>               if (ret < 0) {
>>>                   goto err;
>>>               }
>>>
>>
>>


-- 
Best regards,
Vladimir