[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: |
Thu, 15 Oct 2020 17:49:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 |
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
- 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