introduce max_transfer_length
v4 cover introduce max_transfer_length
This series adds the basics for introducing a maximum transfer length to the block layer. Its main purpose is currently avoiding that a multiwrite_merge exceeds the max_xfer_len of an attached iSCSI LUN. This is a required bug fix.
Splitting up requests according to the max_transfer_length will follow in a later series.
v3->v4: introduce MIN_NON_ZERO to correctly calculate minimum of 2 limits. v2->v3: remove Patch 2 completely [Paolo]v1->v2: do not throw errors but generate trace events in Patch 2 [Paolo]
Peter Lieven (4):
util: introduce MIN_NON_ZERO
BlockLimits: introduce max_transfer_length block/iscsi: set max_transfer_length
block: avoid creating oversized writes in multiwrite_merge
block.c | 9 +++++++++ block/iscsi.c | 12 ++++++++++-- include/block/block_int.h | 3 +++ include/qemu/osdep.h | 4 ++++ 4 files changed, 26 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v4 1/4 util: introduce MIN_NON_ZERO
at least in block layer we have the case of limits being defined for a BlockDriverState. However, in this context often zero (0) has the special meanining of undefined which means no limit. If two of those limits are combined and the minimum is needed the minimum function should only return zero if both parameters are zero.
--- include/qemu/osdep.h | 4 ++++ 1 file changed, 4 insertions(+)
include/qemu/osdep.h +4 -0
@@ -68,6 +68,10 @@ typedef signed int int_fast16_t;#define MAX (a, b) (((a) > (b)) ? (a) : (b))#endif +#ifndef MIN_NON_ZERO +#define MIN_NON_ZERO (a, b) ((!!(a) && (a) < (b)) ? (a) : (b)) +#endif +#ifndef ROUND_UP #define ROUND_UP (n,d) (((n) + (d) - 1) & -(d))#endif
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv4 1/4] util: introduce MIN_NON_ZERO
Quoted 16 lines · Peter Lieven
at least in block layer we have the case of limits being defined for a BlockDriverState. However, in this context often zero (0) has the special meanining of undefined which means no limit. If two of those limits are combined and the minimum is needed the minimum function should only return zero if both parameters are zero.
Signed-off-by Peter Lieven <[email protected]>--- include/qemu/osdep.h | 4 ++++ 1 file changed, 4 insertions(+)include/qemu/osdep.h +0
-0
include/qemu/osdep.h typedef signed int int_fast16_t; · line 72
Quoted 5 lines
include/qemu/osdep.h +2
-0@@ -68,6 +68,10 @@ typedef signed int int_fast16_t;#define MAX (a, b) (((a) > (b)) ? (a) : (b))#endif +#ifndef MIN_NON_ZERO +#define MIN_NON_ZERO (a, b) ((!!(a) && (a) < (b)) ? (a) : (b))
'(a) && expr' is already forcing the evaluation of (a) as a boolean; thus rendering the '!!(a)' conversion to boolean redundant.
The patch is correct as is, so I'll leave a positive review, but if you have a reason to respin, making it two characters shorter is probably worth it. Maybe it's also worth a comment that this macro is only designed to work on unsigned values.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCHv4 1/4] util: introduce MIN_NON_ZERO
In reply to Peter LievenQuoted 16 lines · Peter Lieven
at least in block layer we have the case of limits being defined for a BlockDriverState. However, in this context often zero (0) has the special meanining of undefined which means no limit. If two of those limits are combined and the minimum is needed the minimum function should only return zero if both parameters are zero.
Signed-off-by Peter Lieven <[email protected]>--- include/qemu/osdep.h | 4 ++++ 1 file changed, 4 insertions(+)include/qemu/osdep.h +0
-0
include/qemu/osdep.h typedef signed int int_fast16_t; · line 72
Quoted 5 lines
include/qemu/osdep.h +2
-0@@ -68,6 +68,10 @@ typedef signed int int_fast16_t;#define MAX (a, b) (((a) > (b)) ? (a) : (b))#endif +#ifndef MIN_NON_ZERO +#define MIN_NON_ZERO (a, b) ((!!(a) && (a) < (b)) ? (a) : (b))
I contrast to Eric I'd even like (a) != 0 instead of !!(a). I normally use just "foo" instead of "foo != 0" or "foo != NULL", but in this case I'd like (a) != 0 (or (a) > 0) because of the name "NON_ZERO".
Now you have three alternatives to choose from. You're welcome. :-)
Anyway, I'm fine with any, so:
Quoted 5 lines
include/qemu/osdep.h +2
-0+#endif +#ifndef ROUND_UP #define ROUND_UP (n,d) (((n) + (d) - 1) & -(d))#endif
PATCH v4 2/4 BlockLimits: introduce max_transfer_length
In reply to Peter Lieven--- block.c | 4 ++++ include/block/block_int.h | 3 +++ 2 files changed, 7 insertions(+)
block.c +4 -0
@@ -536,6 +536,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)return ; } bs->bl.opt_transfer_length = bs->file->bl.opt_transfer_length; + bs->bl.max_transfer_length = bs->file->bl.max_transfer_length; bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment; }else { bs->bl.opt_mem_alignment =512 ;
@@ -550,6 +551,9 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp) bs->bl.opt_transfer_length =MAX (bs->bl.opt_transfer_length, bs->backing_hd->bl.opt_transfer_length); + bs->bl.max_transfer_length = +MIN_NON_ZERO (bs->bl.max_transfer_length, + bs->backing_hd->bl.max_transfer_length); bs->bl.opt_mem_alignment =MAX (bs->bl.opt_mem_alignment, bs->backing_hd->bl.opt_mem_alignment);
include/block/block_int.h +3 -0
@@ -289,6 +289,9 @@ typedef struct BlockLimits {/* optimal transfer length in sectors */ int opt_transfer_length; +/* maximal transfer length in sectors */ +int max_transfer_length; +/* memory alignment so that no bounce buffer is needed */ size_t opt_mem_alignment; } BlockLimits;
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv4 2/4] BlockLimits: introduce max_transfer_length
Quoted 5 lines · Peter Lieven
Signed-off-by Peter Lieven <[email protected]>--- block.c | 4 ++++ include/block/block_int.h | 3 +++ 2 files changed, 7 insertions(+)
PATCH v4 3/4 block/iscsi: set max_transfer_length
In reply to Peter Lieventhe limit of 0xffffff for 16 byte CDBs is intentional to avoid overflows on 32-bit architectures.
--- block/iscsi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
block/iscsi.c +10 -2
@@ -1449,10 +1449,18 @@ static void iscsi_close(BlockDriverState *bs)static void iscsi_refresh_limits (BlockDriverState *bs,Error **errp) { -IscsiLun *iscsilun = bs->opaque; -/* We don't actually refresh here, but just return data queried in * iscsi_open(): iscsi targets don't change their limits. */ + +IscsiLun *iscsilun = bs->opaque; +uint32_t max_xfer_len = iscsilun->use_16_for_rw ?0xffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =sector_lun2qemu (max_xfer_len, iscsilun); +if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length
Quoted 2 lines · Peter Lieven
the limit of
0xfffffffor 16 byte CDBs is intentional to avoid overflows on 32-bit architectures.
How is it related to 32 bit? I somehow feel like it has to do something with the result of sector_lun2qemu() which involves block_size...
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>Reviewed-by Ronnie Sahlberg <[email protected]>--- block/iscsi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)block/iscsi.c +10
-2@@ -1449,10 +1449,18 @@ static void iscsi_close(BlockDriverState *bs)static void iscsi_refresh_limits (BlockDriverState *bs,Error **errp) { -IscsiLun *iscsilun = bs->opaque; -/* We don't actually refresh here, but just return data queried in * iscsi_open(): iscsi targets don't change their limits. */ + +IscsiLun *iscsilun = bs->opaque; +uint32_t max_xfer_len = iscsilun->use_16_for_rw ?0xffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =sector_lun2qemu (max_xfer_len, iscsilun); +if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length
Am 23.10.2014 um 13:18 schrieb Max Reitz:
Quoted 5 lines · Peter Lieven
Quoted 2 lines
the limit of
0xfffffffor 16 byte CDBs is intentional to avoid overflows on 32-bit architectures.How is it related to 32 bit? I somehow feel like it has to do something with the result of
sector_lun2qemu()which involvesblock_size...
iscsilun->bl.max_xfer_len is 32-bit unsigned while nb_sectors is usually signed. Furthermore as you suspected nb_sectors is always 512Byte sectors while iscsilun->block_size can be 4k or even more.
I will change the code to set max_xfer_len to 0xffffffff and limit the output of sector_lun2qemu() to INT_MAX in the bs->bl.max_transfer_length case.
However, in real life you will never want to have a transfer of 0x3fffffff blocks, won't you?
Peter
Quoted 32 lines
Quoted 31 lines
Signed-off-by Peter Lieven <[email protected]>Reviewed-by Ronnie Sahlberg <[email protected]>--- block/iscsi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)block/iscsi.c +10
-2@@ -1449,10 +1449,18 @@ static void iscsi_close(BlockDriverState *bs)static void iscsi_refresh_limits (BlockDriverState *bs,Error **errp) { -IscsiLun *iscsilun = bs->opaque; -/* We don't actually refresh here, but just return data queried in * iscsi_open(): iscsi targets don't change their limits. */ + +IscsiLun *iscsilun = bs->opaque; +uint32_t max_xfer_len = iscsilun->use_16_for_rw ?0xffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =sector_lun2qemu (max_xfer_len, iscsilun); +if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
Re: [Qemu-devel] [PATCHv4 3/4] block/iscsi: set max_transfer_length
Quoted 11 lines · Peter Lieven
Am 23.10.2014 um 13:18 schrieb Max Reitz:
Quoted 4 lines · Peter Lieven
Quoted 2 lines
the limit of
0xfffffffor 16 byte CDBs is intentional to avoid overflows on 32-bit architectures.How is it related to 32 bit? I somehow feel like it has to do something with the result of
sector_lun2qemu()which involvesblock_size...
iscsilun->bl.max_xfer_lenis 32-bit unsigned whilenb_sectorsis usually signed. Furthermore as you suspectednb_sectorsis always 512Byte sectors whileiscsilun->block_sizecan be 4k or even more.I will change the code to set
max_xfer_lento0xffffffffand limit the output ofsector_lun2qemu()toINT_MAXin thebs->bl.max_transfer_lengthcase.However, in real life you will never want to have a transfer of
0x3fffffffblocks, won't you?
I'd be fine with a single block, but QEMU guests matter more than me. :-)
I was just wondering how you ended up with 2^24 - 1 as it would be (2^32 - 1) / 256. I do remember IDE doing 16-bit-transfers, so I could imagine how this works out with 512 byte sector size, but this isn't IDE, so...
(sorry, it's been a while since I worked with the SCSI protocol myself)
Max
Quoted 33 lines
Peter
Quoted 30 lines
Quoted 30 lines
Signed-off-by Peter Lieven <[email protected]>Reviewed-by Ronnie Sahlberg <[email protected]>--- block/iscsi.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)block/iscsi.c +10
-2@@ -1449,10 +1449,18 @@ static void iscsi_close(BlockDriverState *bs)static void iscsi_refresh_limits (BlockDriverState *bs,Error **errp) { -IscsiLun *iscsilun = bs->opaque; -/* We don't actually refresh here, but just return data queried in * iscsi_open(): iscsi targets don't change their limits. */ + +IscsiLun *iscsilun = bs->opaque; +uint32_t max_xfer_len = iscsilun->use_16_for_rw ?0xffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =sector_lun2qemu (max_xfer_len, iscsilun); +if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { bs->bl.max_discard = sector_lun2qemu(iscsilun->bl.max_unmap,
PATCH v4 4/4 block: avoid creating oversized writes in multiwrite_merge
In reply to Peter Lieven--- block.c | 5 +++++ 1 file changed, 5 insertions(+)
block.c +5 -0
@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ; } +if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv4 4/4] block: avoid creating oversized writes in multiwrite_merge
Quoted 12 lines · Peter Lieven
Signed-off-by Peter Lieven <[email protected]>Reviewed-by Ronnie Sahlberg <[email protected]>--- block.c | 5 +++++ 1 file changed, 5 insertions(+)block.c +0
-0@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ;
block.c static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, · line 4564
Quoted 10 lines
block.c +5
-0} +if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));
I feel like we should respect max_transfer_length in more than just this function, though. Every block device (or block driver) that sets a maximum transfer length should check requests against it as well, so we don't need to duplicate the same tests in the block layer functions, but maybe if at some point there are no things left to be done on the block layer, we could do that.
Max
Re: [Qemu-devel] [PATCHv4 4/4] block: avoid creating oversized writes in multiwrite_merge
Am 23.10.2014 um 13:23 schrieb Max Reitz:
Quoted 25 lines · Peter Lieven
Quoted 11 lines
Signed-off-by Peter Lieven <[email protected]>Reviewed-by Ronnie Sahlberg <[email protected]>--- block.c | 5 +++++ 1 file changed, 5 insertions(+)block.c +0
-0block.c static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, · line 4563
Quoted 10 lines
block.c +4
-0@@ -4554,6 +4554,11 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, merge =0 ; } + if (bs->bl.max_transfer_length && reqs[outidx].nb_sectors + + reqs[i].nb_sectors > bs->bl.max_transfer_length) { + merge =0 ; + } +if (merge) {size_t size;QEMUIOVector *qiov =g_malloc0 (sizeof (*qiov));Reviewed-by Max Reitz <[email protected]>
Quoted 1 line
I feel like we should respect
max_transfer_lengthin more than just this function, though. Every block device (or block driver) that sets a maximum transfer length should check requests against it as well, so we don't need to duplicate the same tests in the block layer functions, but maybe if at some point there are no things left to be done on the block layer, we could do that.
I have on my todo to add a splitting logic to block.c to
bdrv_co_do_preadv() and bdrv_co_do_pwritev(), but I agreed with Kevin to introduce this *after* 2.2.
Peter