introduce max_transfer_length
v5 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.
v4->v5: - Added check for (a != 0 instead of !!a) [Max] and some comments
[Eric]
- Correctly limited max_transfer_length to INT_MAX in Patch 3 [Max]
- Added Patch 5 and Patch 6.
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 (6):
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/iscsi: limit to INT_MAX throughout iscsi_refresh_limits block/iscsi: check for oversized requests
block.c | 9 +++++++++ block/iscsi.c | 49 ++++++++++++++++++++++++++++++++++----------- include/block/block_int.h | 3 +++ include/qemu/osdep.h | 6 ++++++ 4 files changed, 55 insertions(+), 12 deletions(-)
Signature · 1 line
1.7.9.5
PATCH v5 1/6 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 | 6 ++++++ 1 file changed, 6 insertions(+)
include/qemu/osdep.h +6 -0
@@ -68,6 +68,12 @@ typedef signed int int_fast16_t;#define MAX (a, b) (((a) > (b)) ? (a) : (b))#endif +/* Minimum function that returns zero only iff both values are zero. +* Intended for use with unsigned values only. */ +#ifndef MIN_NON_ZERO +#define MIN_NON_ZERO (a, b) (((a != 0) && (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] [PATCHv5 1/6] 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 | 6 ++++++ 1 file changed, 6 insertions(+)include/qemu/osdep.h +0
-0
include/qemu/osdep.h typedef signed int int_fast16_t; · line 74
Quoted 7 lines
include/qemu/osdep.h +4
-0@@ -68,6 +68,12 @@ typedef signed int int_fast16_t;#define MAX (a, b) (((a) > (b)) ? (a) : (b))#endif +/* Minimum function that returns zero only iff both values are zero. +* Intended for use with unsigned values only. */ +#ifndef MIN_NON_ZERO +#define MIN_NON_ZERO (a, b) (((a != 0) && (a) < (b)) ? (a) : (b))
*cough* This should be (((a) != 0 && (a) < (b)) ? (a) : (b)), so the parentheses need to enclose the "a", not the "a != 0".
I guess the maintainer could fix that up, though.
And with that fixed, of course,
Quoted 5 lines
include/qemu/osdep.h +2
-0+#endif +#ifndef ROUND_UP #define ROUND_UP (n,d) (((n) + (d) - 1) & -(d))#endif
PATCH v5 2/6 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
@@ -519,6 +519,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 ;
@@ -533,6 +534,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
PATCH v5 3/6 block/iscsi: set max_transfer_length
In reply to Peter LievenCopy the max_xfer_len from the BlockLimits VPD or use the maximum value fitting in the CDB.
--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
block/iscsi.c +15 -2
@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu (int64_t sector,IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;
@@ -1449,10 +1454,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 ?0xffffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =nb_sectors_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] [PATCHv5 3/6] block/iscsi: set max_transfer_length
Quoted 35 lines · Peter Lieven
Copy the
max_xfer_lenfrom the BlockLimits VPD or use the maximum value fitting in the CDB.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)block/iscsi.c +8
-2@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu (int64_t sector,IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;@@ -1449,10 +1454,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 ?0xffffffff :0xffff ;
block/iscsi.c static void iscsi_close(BlockDriverState *bs) · line 1471
Quoted 10 lines
block/iscsi.c +7
-0+ +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =nb_sectors_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,
Hm, seems strange to have a function called nb_sectors_lun2qemu() not only convert values, but also cap the result. But anyway:
Re: [Qemu-devel] [PATCHv5 3/6] block/iscsi: set max_transfer_length
Quoted 38 lines · Max Reitz
Quoted 37 lines · Peter Lieven
Copy the
max_xfer_lenfrom the BlockLimits VPD or use the maximum value fitting in the CDB.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)block/iscsi.c +11
-2@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu(int64_t sector, IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;@@ -1449,10 +1454,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 ?0xffffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + }
Quoted 8 lines
block/iscsi.c static void iscsi_close(BlockDriverState *bs) · line 1471
Quoted 6 lines
block/iscsi.c +3
-0+ + bs->bl.max_transfer_length =nb_sectors_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,Hm, seems strange to have a function called
nb_sectors_lun2qemu()not only convert values, but also cap the result. But anyway:
Would you give it another name?
Peter
Re: [Qemu-devel] [PATCHv5 3/6] block/iscsi: set max_transfer_length
Quoted 54 lines · Peter Lieven
Quoted 44 lines · Max Reitz
Quoted 43 lines · Peter Lieven
Copy the
max_xfer_lenfrom the BlockLimits VPD or use the maximum value fitting in the CDB.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)block/iscsi.c +4
-2@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu(int64_t sector, IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;@@ -1449,10 +1454,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. */block/iscsi.c +3
-0+ +IscsiLun *iscsilun = bs->opaque; + uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff :
0xffff;block/iscsi.c +6
-0+ +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length = nb_sectors_lun2qemu(max_xfer_len,Quoted 8 lines
Quoted 6 lines
iscsilun);
+if (iscsilun->lbp.lbpu) { if (iscsilun->bl.max_unmap < 0xffffffff) { bs->bl.max_discard =sector_lun2qemu(iscsilun->bl.max_unmap,
Hm, seems strange to have a function called
nb_sectors_lun2qemu()not only convert values, but also cap the result. But anyway:
Quoted 1 line
Would you give it another name?
Aaah, looking for names... It's always the same horror...
Hm, I just realized why it's nb_sectors, it just wasn't obvious to me that nb_sectors is always an int (and I think it might not be to others either).
So maybe int_sector_lun2qemu() or even better sector_lun2qemu_int()? I always consider everything on the left of the "2" to be the original unit and everything on the right to be the result unit, maybe that confused me further (so sector_lun2qemu_nb_sectors() is what I'd expect, although it's rather long).
But I now see why you called the function that way and it seems fine to me. Just leaving it is fine, too. ;-)
Max
Re: [Qemu-devel] [PATCHv5 3/6] block/iscsi: set max_transfer_length
Quoted 51 lines · Max Reitz
Quoted 48 lines · Peter Lieven
Quoted 38 lines · Max Reitz
Quoted 37 lines · Peter Lieven
Copy the
max_xfer_lenfrom the BlockLimits VPD or use the maximum value fitting in the CDB.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)block/iscsi.c +11
-2@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu(int64_t sector, IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;@@ -1449,10 +1454,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 ?0xffffffff :0xffff ; + +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + }Quoted 8 lines
block/iscsi.c static void iscsi_close(BlockDriverState *bs) · line 1471
Quoted 6 lines
block/iscsi.c +3
-0+ + bs->bl.max_transfer_length =nb_sectors_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,Hm, seems strange to have a function called
nb_sectors_lun2qemu()not only convert values, but also cap the result. But anyway:Quoted 1 line
Would you give it another name?
Quoted 7 lines
Aaah, looking for names... It's always the same horror...
Hm, I just realized why it's
nb_sectors, it just wasn't obvious to me thatnb_sectorsis always an int (and I think it might not be to others either).So maybe
int_sector_lun2qemu()or even bettersector_lun2qemu_int()? I always consider everything on the left of the "2" to be the original unit and everything on the right to be the result unit, maybe that confused me further (sosector_lun2qemu_nb_sectors()is what I'd expect, although it's rather long).But I now see why you called the function that way and it seems fine to me. Just leaving it is fine, too. ;-)
what about limits_lun2qemu ? It might give a hint that nobody uses it for sth else than limits?
Peter
Re: [Qemu-devel] [PATCHv5 3/6] block/iscsi: set max_transfer_length
Quoted 60 lines · Peter Lieven
Quoted 59 lines · Max Reitz
Quoted 54 lines · Peter Lieven
Quoted 44 lines · Max Reitz
Quoted 43 lines · Peter Lieven
Copy the
max_xfer_lenfrom the BlockLimits VPD or use the maximum value fitting in the CDB.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)block/iscsi.c +4
-2@@ -297,6 +297,11 @@ static int64_t sector_lun2qemu(int64_t sector, IscsiLun *iscsilun)return sector * iscsilun->block_size /BDRV_SECTOR_SIZE ; } +static int nb_sectors_lun2qemu(int64_t sector, IscsiLun *iscsilun) +{ +return MIN (sector_lun2qemu (sector, iscsilun),INT_MAX ); +} +static int64_t sector_qemu2lun (int64_t sector,IscsiLun *iscsilun) {return sector *BDRV_SECTOR_SIZE / iscsilun->block_size;@@ -1449,10 +1454,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. */block/iscsi.c +3
-0+ +IscsiLun *iscsilun = bs->opaque; + uint32_t max_xfer_len = iscsilun->use_16_for_rw ? 0xffffffff:
0xffff;block/iscsi.c +6
-0+ +if (iscsilun->bl.max_xfer_len) { + max_xfer_len =MIN (max_xfer_len, iscsilun->bl.max_xfer_len); + } + + bs->bl.max_transfer_length =Quoted 8 lines
Quoted 6 lines
nb_sectors_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,
Hm, seems strange to have a function called
nb_sectors_lun2qemu()not only convert values, but also cap the result. But anyway:Quoted 1 line
Would you give it another name?
Aaah, looking for names... It's always the same horror...
Quoted 7 lines
Quoted 5 lines
Hm, I just realized why it's
nb_sectors, it just wasn't obvious to me thatnb_sectorsis always an int (and I think it might not be to others either).So maybe
int_sector_lun2qemu()or even bettersector_lun2qemu_int()? I always consider everything on the left of the "2" to be the original unit and everything on the right to be the result unit, maybe that confused me further (sosector_lun2qemu_nb_sectors()is what I'd expect, although it's rather long).But I now see why you called the function that way and it seems fine to me. Just leaving it is fine, too. ;-)
what about
limits_lun2qemu? It might give a hint that nobody uses it for sth else than limits?
sector_limits_lun2qemu()? :-)
Max
PATCH v5 4/6 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
@@ -4446,6 +4446,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
PATCH v5 5/6 block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits
In reply to Peter LievenAs Max pointed out there is a hidden cast from int64_t to int. So use the newly introduced nb_sectors_lun2qemu for all limits.
--- block/iscsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
block/iscsi.c +10 -10
@@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { - bs->bl.max_discard =sector_lun2qemu (iscsilun->bl.max_unmap, - iscsilun); + bs->bl.max_discard =nb_sectors_lun2qemu (iscsilun->bl.max_unmap, + iscsilun); } - bs->bl.discard_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.discard_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun); }if (iscsilun->bl.max_ws_len <0xffffffff ) { - bs->bl.max_write_zeroes =sector_lun2qemu (iscsilun->bl.max_ws_len, - iscsilun); + bs->bl.max_write_zeroes =nb_sectors_lun2qemu (iscsilun->bl.max_ws_len, + iscsilun); }if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.write_zeroes_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun); } - bs->bl.opt_transfer_length =sector_lun2qemu (iscsilun->bl.opt_xfer_len, - iscsilun); + bs->bl.opt_transfer_length =nb_sectors_lun2qemu (iscsilun->bl.opt_xfer_len, + iscsilun); }/* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv5 5/6] block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits
Quoted 16 lines · Peter Lieven
As Max pointed out there is a hidden cast from
int64_tto int. So use the newly introducednb_sectors_lun2qemufor all limits.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)block/iscsi.c +0
-0@@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)if (iscsilun->lbp.lbpu) {
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1475
Quoted 10 lines
block/iscsi.c +4
-4if (iscsilun->bl.max_unmap <0xffffffff ) { - bs->bl.max_discard =sector_lun2qemu (iscsilun->bl.max_unmap, - iscsilun); + bs->bl.max_discard =nb_sectors_lun2qemu (iscsilun->bl.max_unmap, + iscsilun); } - bs->bl.discard_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.discard_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun);
This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned INT_MAX. I think it should be capped to something like INT_MAX / 2 + 1 or just simply written out 0x40000000.
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1484
Quoted 13 lines
block/iscsi.c +4
-4}if (iscsilun->bl.max_ws_len <0xffffffff ) { - bs->bl.max_write_zeroes =sector_lun2qemu (iscsilun->bl.max_ws_len, - iscsilun); + bs->bl.max_write_zeroes =nb_sectors_lun2qemu (iscsilun->bl.max_ws_len, + iscsilun); }if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.write_zeroes_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun);
Same here.
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1490
Quoted 8 lines
block/iscsi.c +2
-2} - bs->bl.opt_transfer_length =sector_lun2qemu (iscsilun->bl.opt_xfer_len, - iscsilun); + bs->bl.opt_transfer_length =nb_sectors_lun2qemu (iscsilun->bl.opt_xfer_len, + iscsilun); }/* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in
Max
Re: [Qemu-devel] [PATCHv5 5/6] block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits
Quoted 20 lines · Max Reitz
Quoted 19 lines · Peter Lieven
As Max pointed out there is a hidden cast from
int64_tto int. So use the newly introducednb_sectors_lun2qemufor all limits.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)block/iscsi.c +1
-2@@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { - bs->bl.max_discard =sector_lun2qemu (iscsilun->bl.max_unmap, - iscsilun); + bs->bl.max_discard = nb_sectors_lun2qemu(iscsilun->bl.max_unmap,
Quoted 8 lines
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1475
Quoted 6 lines
block/iscsi.c +3
-2+ iscsilun); } - bs->bl.discard_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.discard_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun);This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned
INT_MAX. I think it should be capped to something likeINT_MAX/ 2 + 1 or just simply written out0x40000000.
Good point, I would cap all limits to the highest power of two and use INT_MAX / 2 + 1 directly in nb_sectors_lun2qemu?
Peter
Quoted 25 lines
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1484
Quoted 13 lines
block/iscsi.c +4
-4}if (iscsilun->bl.max_ws_len <0xffffffff ) { - bs->bl.max_write_zeroes =sector_lun2qemu (iscsilun->bl.max_ws_len, - iscsilun); + bs->bl.max_write_zeroes =nb_sectors_lun2qemu (iscsilun->bl.max_ws_len, + iscsilun); }if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.write_zeroes_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun);Same here.
block/iscsi.c static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp) · line 1490
Quoted 7 lines
block/iscsi.c +2
-2} - bs->bl.opt_transfer_length =sector_lun2qemu (iscsilun->bl.opt_xfer_len, - iscsilun); + bs->bl.opt_transfer_length =nb_sectors_lun2qemu (iscsilun->bl.opt_xfer_len, + iscsilun); }/* Since iscsi_open() ignores bdrv_flags, there is nothing to do here in Max
Signature · 16 lines
Mit freundlichen Grüßen Peter Lieven ........................................................... KAMP Netzwerkdienste GmbH Vestische Str. 89-91 | 46117 Oberhausen Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40 [email protected] | http://www.kamp.de Geschäftsführer: Heiner Lante | Michael Lante Amtsgericht Duisburg | HRB Nr. 12154 USt-Id-Nr.: DE 120607556 ...........................................................
Re: [Qemu-devel] [PATCHv5 5/6] block/iscsi: limit to INT_MAX throughout iscsi_refresh_limits
Quoted 35 lines · Peter Lieven
Quoted 25 lines · Max Reitz
Quoted 24 lines · Peter Lieven
As Max pointed out there is a hidden cast from
int64_tto int. So use the newly introducednb_sectors_lun2qemufor all limits.Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)block/iscsi.c +2
-2@@ -1468,23 +1468,23 @@ static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)if (iscsilun->lbp.lbpu) {if (iscsilun->bl.max_unmap <0xffffffff ) { - bs->bl.max_discard =sector_lun2qemu (iscsilun->bl.max_unmap, - iscsilun); + bs->bl.max_discard =nb_sectors_lun2qemu (iscsilun->bl.max_unmap, + iscsilun); }Quoted 8 lines
block/iscsi.c static void · line 1479
Quoted 6 lines
block/iscsi.c +2
-2- bs->bl.discard_alignment =sector_lun2qemu (iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.discard_alignment =nb_sectors_lun2qemu (iscsilun->bl.opt_unmap_gran, + iscsilun);This looks wrong. I think an alignment should always be a power of two. The function however may return the unaligned
INT_MAX. I think it should be capped to something likeINT_MAX/ 2 + 1 or just simply written out0x40000000.
Quoted 1 line
Good point, I would cap all limits to the highest power of two and use
INT_MAX/ 2 + 1 directly innb_sectors_lun2qemu?
Seems good enough. I'd just like a comment in that function why you're doing that (in order to limit both alignments and "normal" lengths properly).
I'm counting on nobody noticing that INT_MAX may be something different than 2^x - 1. *g*
Max
Quoted 36 lines
Peter
Quoted 32 lines
Quoted 15 lines
block/iscsi.c +3
-4}if (iscsilun->bl.max_ws_len <0xffffffff ) { - bs->bl.max_write_zeroes =sector_lun2qemu (iscsilun->bl.max_ws_len, - iscsilun); + bs->bl.max_write_zeroes =nb_sectors_lun2qemu (iscsilun->bl.max_ws_len, + iscsilun); } if (iscsilun->lbp.lbpws) { - bs->bl.write_zeroes_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran, - iscsilun); + bs->bl.write_zeroes_alignment =Quoted 2 lines
nb_sectors_lun2qemu(iscsilun->bl.opt_unmap_gran,
+ iscsilun);Same here.
Quoted 10 lines
}-
bs->bl.opt_transfer_length=
sector_lun2qemu(iscsilun->bl.opt_xfer_len,block/iscsi.c +1
-1- iscsilun); + bs->bl.opt_transfer_length =nb_sectors_lun2qemu(iscsilun->bl.opt_xfer_len,
+ iscsilun);
}/* Since iscsi_open() ignores bdrv_flags, there is nothing to dohere in
Max
PATCH v5 6/6 block/iscsi: check for oversized requests
In reply to Peter LievenCancel oversized requests early. They would generate an iSCSI protocol error anyway; after having transferred possibly a lot of data over the wire.
--- block/iscsi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
block/iscsi.c +12 -0
@@ -367,6 +367,12 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("iSCSI Error: Write of %d sectors exceeds max_xfer_len " +"of %d sectors." , nb_sectors, bs->bl.max_transfer_length); +return -EINVAL ; + } + lba =sector_qemu2lun (sector_num, iscsilun); num_sectors =sector_qemu2lun (nb_sectors, iscsilun);iscsi_co_init_iscsitask (iscsilun, &iTask);
@@ -534,6 +540,12 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("iSCSI Error: Read of %d sectors exceeds max_xfer_len " +"of %d sectors." , nb_sectors, bs->bl.max_transfer_length); +return -EINVAL ; + } +if (iscsilun->lbprz && nb_sectors >=ISCSI_CHECKALLOC_THRES && !iscsi_allocationmap_is_allocated (iscsilun, sector_num, nb_sectors)) {int64_t ret;
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCHv5 6/6] block/iscsi: check for oversized requests
Quoted 30 lines · Peter Lieven
Cancel oversized requests early. They would generate an iSCSI protocol error anyway; after having transferred possibly a lot of data over the wire.
Suggested-By Max Reitz <[email protected]>Signed-off-by Peter Lieven <[email protected]>--- block/iscsi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)block/iscsi.c +6
-0@@ -367,6 +367,12 @@ static int coroutine_fn iscsi_co_writev(BlockDriverState *bs,return -EINVAL ; } +if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("iSCSI Error: Write of %d sectors exceeds max_xfer_len " +"of %d sectors." , nb_sectors, bs->bl.max_transfer_length); +return -EINVAL ; + } + lba =sector_qemu2lun (sector_num, iscsilun); num_sectors =sector_qemu2lun (nb_sectors, iscsilun);iscsi_co_init_iscsitask (iscsilun, &iTask);@@ -534,6 +540,12 @@ static int coroutine_fn iscsi_co_readv(BlockDriverState *bs,return -EINVAL ; }
block/iscsi.c static int coroutine_fn iscsi_co_readv(BlockDriverState *bs, · line 551
Quoted 10 lines
block/iscsi.c +6
-0+if (bs->bl.max_transfer_length && nb_sectors > bs->bl.max_transfer_length) { +error_report ("iSCSI Error: Read of %d sectors exceeds max_xfer_len " +"of %d sectors." , nb_sectors, bs->bl.max_transfer_length); +return -EINVAL ; + } +if (iscsilun->lbprz && nb_sectors >=ISCSI_CHECKALLOC_THRES && !iscsi_allocationmap_is_allocated (iscsilun, sector_num, nb_sectors)) {int64_t ret;
I think to remember someone telling me there should not be full stops at the end of error messages. I can cope with it, though.