| Message ID | 1602524605-481160-10-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apply COR-filter to the block-stream permanently | expand |
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); Ah, OK. I see. I expected this to be a separate patch. I still wonder why it isn’t. Max > goto out; > } > >
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. Actually, if driver doesn't support the PREFETCH flag we should do nothing. > > 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.
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
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. > >>> 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 >
On 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 &? > Yes, that's correct. >> + !(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: > I played with the flags to make the idea obvious for the eye of a beholder: "we neither read nor write". Comparing the BDRV_REQ_PREFETCH against the 'flags' means that the flag comes from outside of the function. And the empty section means we do nothing in that case. Eventually, I will pick up the brief expression below. Thanks, Andrey > ((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); > > Ah, OK. I see. I expected this to be a separate patch. I still wonder > why it isn’t. > > Max > >> goto out; >> } >> >> > >
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
On 14.10.2020 18: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. > > Actually, if driver doesn't support the PREFETCH flag we should do nothing. > > >> >> 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. > > We have to add the supported flags for the COR driver in the same patch. Or before handling the supported_read_flags at the generic layer (handling zero does not make a sence). Otherwise, the test #216 (where the COR-filter is applied) will not pass. Andrey
On 21.10.2020 23:43, Andrey Shinkevich wrote: > On 14.10.2020 18: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> >>>> --- [...] >>>> 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. >> >> Actually, if driver doesn't support the PREFETCH flag we should do >> nothing. >> >> >>> >>> 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. >> >> > > We have to add the supported flags for the COR driver in the same patch. > Or before handling the supported_read_flags at the generic layer > (handling zero does not make a sence). Otherwise, the test #216 (where > the COR-filter is applied) will not pass. > > Andrey I have found a workaround and am going to send all the related patches as a separate series. Andrey
22.10.2020 10:50, Andrey Shinkevich wrote: > > On 21.10.2020 23:43, Andrey Shinkevich wrote: >> On 14.10.2020 18: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> >>>>> --- > > [...] > >>>>> 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. >>> >>> Actually, if driver doesn't support the PREFETCH flag we should do nothing. >>> >>> >>>> >>>> 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. >>> >>> >> >> We have to add the supported flags for the COR driver in the same patch. Or before handling the supported_read_flags at the generic layer (handling zero does not make a sence). Otherwise, the test #216 (where the COR-filter is applied) will not pass. >> >> Andrey > > I have found a workaround and am going to send all the related patches as a separate series. > What is the problem? If in a separate patch prior to modifying COR driver, we add new field supported_read_flags and add a support for it in generic layer, when no driver support it yet, nothing should be changed and all tests should pass..
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) & + !(local_flags & BDRV_REQ_COPY_ON_READ)) { + /* Skip non-guest reads if no copy needed */ + } else { + 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); goto out; }
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(-)