| Message ID | 20181115020334.1189829-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: byte-based blocking read/write | expand |
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).
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.
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
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)); + } } /**
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(-)