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@xxxxxxxxxxxxx>
>>>>> ---
>>>>> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
