.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.
      */
     int coroutine_fn (*bdrv_co_block_status)(BlockDriverState *bs,
         bool want_zero, int64_t offset, int64_t bytes, int64_t *pnum,
-- 
2.31.1

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,
> 


-- 
Best regards,
Vladimir

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"



-- 
Best regards,
Vladimir

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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