Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
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
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
> > > +++ 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
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 Signed-off-by: Max Reitz --- 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
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
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 Signed-off-by: Max Reitz --- 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" -- Best regards, Vladimir
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *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 Signed-off-by: Max Reitz --- 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
Re: [PATCH v2 3/6] block: Clarify that @bytes is no limit on *pnum
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 Signed-off-by: Max Reitz --- 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 */ int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs, bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum, -- Best regards, Vladimir
[PATCH v2 3/6] block: Clarify that @bytes is no limit on *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 Signed-off-by: Max Reitz --- 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. */ int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs, bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum, -- 2.31.1