[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy need
From: |
Max Reitz |
Subject: |
Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed |
Date: |
Wed, 14 Oct 2020 18:30:02 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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.
>> Ah, OK. I see. I expected this to be a separate patch. I still wonder
>> why it isn’t.
>>
>
>
> Could it be part of patch 07? I mean introduce new field
> supported_read_flags and handle it in generic code in one patch, prior
> to implementing support for it in COR driver.
Sure.
Max
signature.asc
Description: OpenPGP digital signature
- Re: [PATCH v11 06/13] block: modify the comment for BDRV_REQ_PREFETCH flag, (continued)
- [PATCH v11 07/13] block: include supported_read_flags into BDS structure, Andrey Shinkevich, 2020/10/12
- [PATCH v11 08/13] copy-on-read: add support for BDRV_REQ_PREFETCH to COR-filter, Andrey Shinkevich, 2020/10/12
- [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed, Andrey Shinkevich, 2020/10/12
- Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed, Andrey Shinkevich, 2020/10/21
- Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed, Andrey Shinkevich, 2020/10/22
- Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed, Vladimir Sementsov-Ogievskiy, 2020/10/22
- Re: [PATCH v11 09/13] copy-on-read: skip non-guest reads if no copy needed, Andrey Shinkevich, 2020/10/14
[PATCH v11 10/13] stream: skip filters when writing backing file name to QCOW2 header, Andrey Shinkevich, 2020/10/12
[PATCH v11 11/13] stream: mark backing-file argument as deprecated, Andrey Shinkevich, 2020/10/12