Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed

[Max Reitz]

On 14.10.20 18:39, Vladimir Sementsov-Ogievskiy wrote:
> 14.10.2020 19:30, Max Reitz wrote:
>> On 14.10.20 17:22, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.10.2020 15:51, Max Reitz wrote:
>>>> On 12.10.20 19:43, 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 | 13 +++++++++----
>>>>>    block/io.c           |  3 ++-
>>>>>    2 files changed, 11 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/block/copy-on-read.c b/block/copy-on-read.c
>>>>> index b136895..278a11a 100644
>>>>> --- a/block/copy-on-read.c
>>>>> +++ b/block/copy-on-read.c
>>>>> @@ -148,10 +148,15 @@ 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) &
>>>>
>>>> How about dropping the double negation and using a logical &&
>>>> instead of
>>>> the binary &?
>>>>
>>>>> +            !(local_flags & BDRV_REQ_COPY_ON_READ)) {
>>>>> +            /* Skip non-guest reads if no copy needed */
>>>>> +        } else {
>>>>
>>>> Hm.  I would have just written the negated form
>>>>
>>>> (!(flags & BDRV_REQ_PREFETCH) || (local_flags & BDRV_REQ_COPY_ON_READ))
>>>>
>>>> and put the “skip” comment above that condition.
>>>>
>>>> (Since local_flags is initialized to flags, it can be written as a
>>>> single comparison, but that’s a matter of taste and I’m not going to
>>>> recommend either over the other:
>>>>
>>>> ((local_flags & (BDRV_REQ_PREFETCH | BDRV_REQ_COPY_ON_READ)) !=
>>>> BDRV_REQ_PREFETCH)
>>>>
>>>> )
>>>>
>>>>> +            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..bff1808 100644
>>>>> --- a/block/io.c
>>>>> +++ b/block/io.c
>>>>> @@ -1512,7 +1512,8 @@ static int coroutine_fn
>>>>> bdrv_aligned_preadv(BdrvChild *child,
>>>>>          max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
>>>>>        if (bytes <= max_bytes && bytes <= max_transfer) {
>>>>> -        ret = bdrv_driver_preadv(bs, offset, bytes, qiov,
>>>>> qiov_offset, 0);
>>>>> +        ret = bdrv_driver_preadv(bs, offset, bytes, qiov,
>>>>> qiov_offset,
>>>>> +                                 flags & bs->supported_read_flags);
>>>
>>>
>>> When BDRV_REQ_PREFETCH is passed, qiov may be (and generally should be)
>>> NULL. This means, that we can't just drop the flag when call the driver
>>> that doesn't support it.
>>
>> True. :/
>>
>>> Actually, if driver doesn't support the PREFETCH flag we should do
>>> nothing.
>>
>> Hm.  But at least in the case of COR, PREFETCH is not something that can
>> be optimized to be a no-op (unless the COR is a no-op); it still denotes
>> a command that must be executed.
>>
>> So if we can’t pass it to the driver, I don’t think we should do
>> nothing, but to return an error.  Or maybe we could even assert that it
>> isn’t set for drivers that don’t support it, because at least right now
>> such a case would just be a bug.
> 
> Hmm. Reasonable..
> 
> So, let me summarize the cases:
> 
> 1. bdrv_co_preadv(.. , PREFETCH | COR)
> 
>   In this case generic layer should handle both flags and pass flags=0
> to driver
> 
> 2. bdrv_co_preadv(.., PREFETCH)
> 
> 2.1 driver supporst PREFETCH
>     OK, pass PREFETCH to driver, no problems
> 
> 2.2 driver doesn't support PREFETCH
> 
>   We can just abort() here, as the only source of PREFETCH without COR
> would be
>   stream job driver, which must read from COR filter.
> 
>   More generic solution is to allocate temporary buffer (at least if
> qiov is NULL)
>   and call underlying driver .preadv with flags=0 on that temporary
> buffer. But
>   just abort() is simpler and should work for now.

Agreed.

Max

signature.asc
Description: OpenPGP digital signature