This change has no semantic impact: all drivers either leave the
value at 0 (no inherent 32-bit limit is still translated into
fragmentation below 2G; see the previous patch for that audit), or
set it to a value less than 2G.  However, switching to a larger
type and enforcing the 2G cap at the block layer makes it easier
to audit specific drivers for their robustness to larger sizing,
by letting them specify a value larger than INT_MAX if they have
been audited to be 64-bit clean.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/block/block_int.h | 8 +++++---
 block/io.c                | 7 +++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index f605622216d..b19d94dac5f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -549,9 +549,11 @@ typedef struct BlockLimits {

     /* Maximal transfer length in bytes.  Need not be power of 2, but
      * must be multiple of opt_transfer and bl.request_alignment, or 0
-     * for no 32-bit limit.  For now, anything larger than INT_MAX is
-     * clamped down. */
-    uint32_t max_transfer;
+     * for no 32-bit limit.  The block layer fragments all actions
+     * below 2G, so setting this value to anything larger than INT_MAX
+     * implies that the driver has been audited for 64-bit
+     * cleanness. */
+    uint64_t max_transfer;

     /* memory alignment, in bytes so that no bounce buffer is needed */
     size_t min_mem_alignment;
diff --git a/block/io.c b/block/io.c
index 39d4e7a3ae1..4911a1123eb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -159,6 +159,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
     if (drv->bdrv_refresh_limits) {
         drv->bdrv_refresh_limits(bs, errp);
     }
+
+    /* Clamp max_transfer to 2G */
+    if (bs->bl.max_transfer > UINT32_MAX) {
+        bs->bl.max_transfer = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
+                                              MAX(bs->bl.opt_transfer,
+                                                  bs->bl.request_alignment));
+    }
 }

 /**
-- 
2.17.2

Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
> This change has no semantic impact: all drivers either leave the
> value at 0 (no inherent 32-bit limit is still translated into
> fragmentation below 2G; see the previous patch for that audit), or
> set it to a value less than 2G.  However, switching to a larger
> type and enforcing the 2G cap at the block layer makes it easier
> to audit specific drivers for their robustness to larger sizing,
> by letting them specify a value larger than INT_MAX if they have
> been audited to be 64-bit clean.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/block/block_int.h | 8 +++++---
>  block/io.c                | 7 +++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f605622216d..b19d94dac5f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -549,9 +549,11 @@ typedef struct BlockLimits {
> 
>      /* Maximal transfer length in bytes.  Need not be power of 2, but
>       * must be multiple of opt_transfer and bl.request_alignment, or 0
> -     * for no 32-bit limit.  For now, anything larger than INT_MAX is
> -     * clamped down. */
> -    uint32_t max_transfer;
> +     * for no 32-bit limit.  The block layer fragments all actions
> +     * below 2G, so setting this value to anything larger than INT_MAX
> +     * implies that the driver has been audited for 64-bit
> +     * cleanness. */
> +    uint64_t max_transfer;
> 
>      /* memory alignment, in bytes so that no bounce buffer is needed */
>      size_t min_mem_alignment;
> diff --git a/block/io.c b/block/io.c
> index 39d4e7a3ae1..4911a1123eb 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -159,6 +159,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>      if (drv->bdrv_refresh_limits) {
>          drv->bdrv_refresh_limits(bs, errp);
>      }
> +
> +    /* Clamp max_transfer to 2G */
> +    if (bs->bl.max_transfer > UINT32_MAX) {

UINT32_MAX is 4G, not 2G.

Would it make more sense to make BDRV_REQUEST_MAX_BYTES the maximum
anyway? Allowing higher (but not too high) explicit values than what we
clamp to feels a bit odd.

BDRV_REQUEST_MAX_BYTES is probably also what drivers really expect
today.

> +        bs->bl.max_transfer = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
> +                                              MAX(bs->bl.opt_transfer,
> +                                                  bs->bl.request_alignment));
> +    }
>  }

Kevin

On 11/15/18 9:45 AM, Kevin Wolf wrote:
> Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
>> This change has no semantic impact: all drivers either leave the
>> value at 0 (no inherent 32-bit limit is still translated into
>> fragmentation below 2G; see the previous patch for that audit), or
>> set it to a value less than 2G.  However, switching to a larger
>> type and enforcing the 2G cap at the block layer makes it easier
>> to audit specific drivers for their robustness to larger sizing,
>> by letting them specify a value larger than INT_MAX if they have
>> been audited to be 64-bit clean.
>>

>> +++ b/block/io.c
>> @@ -159,6 +159,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
>>       if (drv->bdrv_refresh_limits) {
>>           drv->bdrv_refresh_limits(bs, errp);
>>       }
>> +
>> +    /* Clamp max_transfer to 2G */
>> +    if (bs->bl.max_transfer > UINT32_MAX) {
> 
> UINT32_MAX is 4G, not 2G.
> 
> Would it make more sense to make BDRV_REQUEST_MAX_BYTES the maximum
> anyway?

D'oh.  Yes, that's what I intended, possibly by spelling it INT_MAX (the 
fact that the 'if' goes away in patch 13 is not an excuse for sloppy 
coding in the meantime).

> Allowing higher (but not too high) explicit values than what we
> clamp to feels a bit odd.
> 
> BDRV_REQUEST_MAX_BYTES is probably also what drivers really expect
> today.

Correct.  Well, at least the unaudited drivers (the rest of this series 
audits a few drivers that can handle larger byte values).

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

Am 15.11.2018 um 17:28 hat Eric Blake geschrieben:
> On 11/15/18 9:45 AM, Kevin Wolf wrote:
> > Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
> > > This change has no semantic impact: all drivers either leave the
> > > value at 0 (no inherent 32-bit limit is still translated into
> > > fragmentation below 2G; see the previous patch for that audit), or
> > > set it to a value less than 2G.  However, switching to a larger
> > > type and enforcing the 2G cap at the block layer makes it easier
> > > to audit specific drivers for their robustness to larger sizing,
> > > by letting them specify a value larger than INT_MAX if they have
> > > been audited to be 64-bit clean.
> > > 
> 
> > > +++ b/block/io.c
> > > @@ -159,6 +159,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
> > >       if (drv->bdrv_refresh_limits) {
> > >           drv->bdrv_refresh_limits(bs, errp);
> > >       }
> > > +
> > > +    /* Clamp max_transfer to 2G */
> > > +    if (bs->bl.max_transfer > UINT32_MAX) {
> > 
> > UINT32_MAX is 4G, not 2G.
> > 
> > Would it make more sense to make BDRV_REQUEST_MAX_BYTES the maximum
> > anyway?
> 
> D'oh.  Yes, that's what I intended, possibly by spelling it INT_MAX (the
> fact that the 'if' goes away in patch 13 is not an excuse for sloppy coding
> in the meantime).

INT_MAX is not a different spelling of BDRV_REQUEST_MAX_BYTES. The
latter is slightly lower (0x7ffffe00).

Kevin

On 11/16/18 9:32 AM, Kevin Wolf wrote:
> Am 15.11.2018 um 17:28 hat Eric Blake geschrieben:
>> On 11/15/18 9:45 AM, Kevin Wolf wrote:
>>> Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
>>>> This change has no semantic impact: all drivers either leave the
>>>> value at 0 (no inherent 32-bit limit is still translated into
>>>> fragmentation below 2G; see the previous patch for that audit), or
>>>> set it to a value less than 2G.  However, switching to a larger
>>>> type and enforcing the 2G cap at the block layer makes it easier
>>>> to audit specific drivers for their robustness to larger sizing,
>>>> by letting them specify a value larger than INT_MAX if they have
>>>> been audited to be 64-bit clean.
>>>>

>>>> +
>>>> +    /* Clamp max_transfer to 2G */
>>>> +    if (bs->bl.max_transfer > UINT32_MAX) {
>>>
>>> UINT32_MAX is 4G, not 2G.
>>>
>>> Would it make more sense to make BDRV_REQUEST_MAX_BYTES the maximum
>>> anyway?
>>
>> D'oh.  Yes, that's what I intended, possibly by spelling it INT_MAX (the
>> fact that the 'if' goes away in patch 13 is not an excuse for sloppy coding
>> in the meantime).
> 
> INT_MAX is not a different spelling of BDRV_REQUEST_MAX_BYTES. The
> latter is slightly lower (0x7ffffe00).

Ah, but:

> +    if (bs->bl.max_transfer > UINT32_MAX) {
> +        bs->bl.max_transfer = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
> +                                              MAX(bs->bl.opt_transfer,
> +                                                  bs->bl.request_alignment));
> +    }

QEMU_ALIGN_DOWN() will change INT_MAX to BDRV_REQUEST_MAX_BYTES if 
bs->bl.request_alignment is 512 and opt_transfer is 0; and if either 
alignment number is larger than 512, max_transfer is capped even lower 
regardless of whether I was aligning INT_MAX or BDRV_REQUEST_MAX_BYTES 
down to an alignment boundary.

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

Am 16.11.2018 um 16:54 hat Eric Blake geschrieben:
> On 11/16/18 9:32 AM, Kevin Wolf wrote:
> > Am 15.11.2018 um 17:28 hat Eric Blake geschrieben:
> > > On 11/15/18 9:45 AM, Kevin Wolf wrote:
> > > > Am 15.11.2018 um 03:03 hat Eric Blake geschrieben:
> > > > > This change has no semantic impact: all drivers either leave the
> > > > > value at 0 (no inherent 32-bit limit is still translated into
> > > > > fragmentation below 2G; see the previous patch for that audit), or
> > > > > set it to a value less than 2G.  However, switching to a larger
> > > > > type and enforcing the 2G cap at the block layer makes it easier
> > > > > to audit specific drivers for their robustness to larger sizing,
> > > > > by letting them specify a value larger than INT_MAX if they have
> > > > > been audited to be 64-bit clean.
> > > > > 
> 
> > > > > +
> > > > > +    /* Clamp max_transfer to 2G */
> > > > > +    if (bs->bl.max_transfer > UINT32_MAX) {
> > > > 
> > > > UINT32_MAX is 4G, not 2G.
> > > > 
> > > > Would it make more sense to make BDRV_REQUEST_MAX_BYTES the maximum
> > > > anyway?
> > > 
> > > D'oh.  Yes, that's what I intended, possibly by spelling it INT_MAX (the
> > > fact that the 'if' goes away in patch 13 is not an excuse for sloppy coding
> > > in the meantime).
> > 
> > INT_MAX is not a different spelling of BDRV_REQUEST_MAX_BYTES. The
> > latter is slightly lower (0x7ffffe00).
> 
> Ah, but:
> 
> > +    if (bs->bl.max_transfer > UINT32_MAX) {
> > +        bs->bl.max_transfer = QEMU_ALIGN_DOWN(BDRV_REQUEST_MAX_BYTES,
> > +                                              MAX(bs->bl.opt_transfer,
> > +                                                  bs->bl.request_alignment));
> > +    }
> 
> QEMU_ALIGN_DOWN() will change INT_MAX to BDRV_REQUEST_MAX_BYTES if
> bs->bl.request_alignment is 512 and opt_transfer is 0; and if either
> alignment number is larger than 512, max_transfer is capped even lower
> regardless of whether I was aligning INT_MAX or BDRV_REQUEST_MAX_BYTES down
> to an alignment boundary.

That's true. But why use INT_MAX and argue that it will be rounded to
the right value later when you can just use BDRV_REQUEST_MAX_BYTES,
which is obviously correct without additional correction?

Kevin