| Message ID | 20210623150143.188184-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: block-status cache for data regions | expand |
23.06.2021 18:01, Max Reitz wrote: > .bdrv_co_block_status() implementations are free to return a *pnum that > exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp > *pnum as necessary. > > On the other hand, if drivers' implementations return values for *pnum > that are as large as possible, our recently introduced block-status > cache will become more effective. > > So, make a note in block_int.h that @bytes is no upper limit for *pnum. > > Suggested-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > include/block/block_int.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index fcb599dd1c..f85b96ed99 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -347,6 +347,11 @@ struct BlockDriver { > * clamped to bdrv_getlength() and aligned to request_alignment, > * as well as non-NULL pnum, map, and file; in turn, the driver > * must return an error or set pnum to an aligned non-zero value. > + * > + * Note that @bytes is just a hint on how big of a region the > + * caller wants to inspect. It is not a limit on *pnum. > + * Implementations are free to return larger values of *pnum if > + * doing so does not incur a performance penalty. Worth mention that the cache will benefit of it? Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > */ > int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs, > bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum, >
On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote: > 23.06.2021 18:01, Max Reitz wrote: >> .bdrv_co_block_status() implementations are free to return a *pnum that >> exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp >> *pnum as necessary. >> >> On the other hand, if drivers' implementations return values for *pnum >> that are as large as possible, our recently introduced block-status >> cache will become more effective. >> >> So, make a note in block_int.h that @bytes is no upper limit for *pnum. >> >> Suggested-by: Eric Blake <eblake@redhat.com> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> include/block/block_int.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index fcb599dd1c..f85b96ed99 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -347,6 +347,11 @@ struct BlockDriver { >> * clamped to bdrv_getlength() and aligned to request_alignment, >> * as well as non-NULL pnum, map, and file; in turn, the driver >> * must return an error or set pnum to an aligned non-zero value. >> + * >> + * Note that @bytes is just a hint on how big of a region the >> + * caller wants to inspect. It is not a limit on *pnum. >> + * Implementations are free to return larger values of *pnum if >> + * doing so does not incur a performance penalty. > > Worth mention that the cache will benefit of it? Oh, right, absolutely. Like so: "block/io.c's bdrv_co_block_status() will clamp *pnum before returning it to its caller, but it itself can still make use of the unclamped *pnum value. Specifically, the block-status cache for protocol nodes will benefit from storing as large a region as possible." ? Max
24.06.2021 13:16, Max Reitz wrote: > On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote: >> 23.06.2021 18:01, Max Reitz wrote: >>> .bdrv_co_block_status() implementations are free to return a *pnum that >>> exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp >>> *pnum as necessary. >>> >>> On the other hand, if drivers' implementations return values for *pnum >>> that are as large as possible, our recently introduced block-status >>> cache will become more effective. >>> >>> So, make a note in block_int.h that @bytes is no upper limit for *pnum. >>> >>> Suggested-by: Eric Blake <eblake@redhat.com> >>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>> --- >>> include/block/block_int.h | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index fcb599dd1c..f85b96ed99 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -347,6 +347,11 @@ struct BlockDriver { >>> * clamped to bdrv_getlength() and aligned to request_alignment, >>> * as well as non-NULL pnum, map, and file; in turn, the driver >>> * must return an error or set pnum to an aligned non-zero value. >>> + * >>> + * Note that @bytes is just a hint on how big of a region the >>> + * caller wants to inspect. It is not a limit on *pnum. >>> + * Implementations are free to return larger values of *pnum if >>> + * doing so does not incur a performance penalty. >> >> Worth mention that the cache will benefit of it? > > Oh, right, absolutely. Like so: > > "block/io.c's bdrv_co_block_status() will clamp *pnum before returning it to its caller, but it itself can still make use of the unclamped *pnum value. Specifically, the block-status cache for protocol nodes will benefit from storing as large a region as possible." > Sounds good. Do you mean this as an addition or substitution? If the latter, I'd keep "if doing so does not incur a performance penalty"
On 24.06.21 12:25, Vladimir Sementsov-Ogievskiy wrote: > 24.06.2021 13:16, Max Reitz wrote: >> On 24.06.21 11:15, Vladimir Sementsov-Ogievskiy wrote: >>> 23.06.2021 18:01, Max Reitz wrote: >>>> .bdrv_co_block_status() implementations are free to return a *pnum >>>> that >>>> exceeds @bytes, because bdrv_co_block_status() in block/io.c will >>>> clamp >>>> *pnum as necessary. >>>> >>>> On the other hand, if drivers' implementations return values for *pnum >>>> that are as large as possible, our recently introduced block-status >>>> cache will become more effective. >>>> >>>> So, make a note in block_int.h that @bytes is no upper limit for >>>> *pnum. >>>> >>>> Suggested-by: Eric Blake <eblake@redhat.com> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> include/block/block_int.h | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>>> index fcb599dd1c..f85b96ed99 100644 >>>> --- a/include/block/block_int.h >>>> +++ b/include/block/block_int.h >>>> @@ -347,6 +347,11 @@ struct BlockDriver { >>>> * clamped to bdrv_getlength() and aligned to request_alignment, >>>> * as well as non-NULL pnum, map, and file; in turn, the driver >>>> * must return an error or set pnum to an aligned non-zero >>>> value. >>>> + * >>>> + * Note that @bytes is just a hint on how big of a region the >>>> + * caller wants to inspect. It is not a limit on *pnum. >>>> + * Implementations are free to return larger values of *pnum if >>>> + * doing so does not incur a performance penalty. >>> >>> Worth mention that the cache will benefit of it? >> >> Oh, right, absolutely. Like so: >> >> "block/io.c's bdrv_co_block_status() will clamp *pnum before >> returning it to its caller, but it itself can still make use of the >> unclamped *pnum value. Specifically, the block-status cache for >> protocol nodes will benefit from storing as large a region as possible." >> > > Sounds good. Do you mean this as an addition or substitution? If the > latter, I'd keep "if doing so does not incur a performance penalty I meant it as an addition, just a new paragraph. Max
> > > +++ b/include/block/block_int.h > > > @@ -347,6 +347,11 @@ struct BlockDriver { > > > * clamped to bdrv_getlength() and aligned to request_alignment, > > > * as well as non-NULL pnum, map, and file; in turn, the driver > > > * must return an error or set pnum to an aligned non-zero value. > > > + * > > > + * Note that @bytes is just a hint on how big of a region the > > > + * caller wants to inspect. It is not a limit on *pnum. > > > + * Implementations are free to return larger values of *pnum if > > > + * doing so does not incur a performance penalty. > > > > Worth mention that the cache will benefit of it? > > Oh, right, absolutely. Like so: > > "block/io.c's bdrv_co_block_status() will clamp *pnum before returning it to > its caller, but it itself can still make use of the unclamped *pnum value. > Specifically, the block-status cache for protocol nodes will benefit from > storing as large a region as possible." How about this tweak to the wording to make it flow a little better: block/io.c's bdrv_co_block_status() will utilize an unclamped *pnum value for the block-status cache on protocol nodes, prior to clamping *pnum for return to its caller.
On 28.06.21 21:10, Eric Blake wrote: >>>> +++ b/include/block/block_int.h >>>> @@ -347,6 +347,11 @@ struct BlockDriver { >>>> * clamped to bdrv_getlength() and aligned to request_alignment, >>>> * as well as non-NULL pnum, map, and file; in turn, the driver >>>> * must return an error or set pnum to an aligned non-zero value. >>>> + * >>>> + * Note that @bytes is just a hint on how big of a region the >>>> + * caller wants to inspect. It is not a limit on *pnum. >>>> + * Implementations are free to return larger values of *pnum if >>>> + * doing so does not incur a performance penalty. >>> Worth mention that the cache will benefit of it? >> Oh, right, absolutely. Like so: >> >> "block/io.c's bdrv_co_block_status() will clamp *pnum before returning it to >> its caller, but it itself can still make use of the unclamped *pnum value. >> Specifically, the block-status cache for protocol nodes will benefit from >> storing as large a region as possible." > How about this tweak to the wording to make it flow a little better: > > block/io.c's bdrv_co_block_status() will utilize an unclamped *pnum > value for the block-status cache on protocol nodes, prior to clamping > *pnum for return to its caller. Sure, thanks! Max
diff --git a/include/block/block_int.h b/include/block/block_int.h index fcb599dd1c..f85b96ed99 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -347,6 +347,11 @@ struct BlockDriver { * clamped to bdrv_getlength() and aligned to request_alignment, * as well as non-NULL pnum, map, and file; in turn, the driver * must return an error or set pnum to an aligned non-zero value. + * + * Note that @bytes is just a hint on how big of a region the + * caller wants to inspect. It is not a limit on *pnum. + * Implementations are free to return larger values of *pnum if + * doing so does not incur a performance penalty. */ int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs, bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
.bdrv_co_block_status() implementations are free to return a *pnum that exceeds @bytes, because bdrv_co_block_status() in block/io.c will clamp *pnum as necessary. On the other hand, if drivers' implementations return values for *pnum that are as large as possible, our recently introduced block-status cache will become more effective. So, make a note in block_int.h that @bytes is no upper limit for *pnum. Suggested-by: Eric Blake <eblake@redhat.com> Signed-off-by: Max Reitz <mreitz@redhat.com> --- include/block/block_int.h | 5 +++++ 1 file changed, 5 insertions(+)