introduce max_transfer_length

PATCH Applied Peter Lieven 3 participants 10 messages Oct 27, 2014 – Oct 28, 2014 Block layer

Share & tools

Peter Lieven

author

2014-10-27 09:20 UTC #0 lore raw

v6 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.

v5->v6: - fix parentheses in Patch 1 [Max]
- rename helper function in Patch 3 [Max]
- cap the limits to highest power of 2 fitting in INT_MAX [Max]
- removed full stops in error messages in Patch 6 [Max]

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: use sector_limits_lun2qemu 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

Peter Lieven

author

2014-10-27 09:20 UTC #1 lore raw

PATCH v6 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.

Signed-off-by Peter Lieven <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
 include/qemu/osdep.h |    6 ++++++
 1 file changed, 6 insertions(+)

include/qemu/osdep.h +6 -0

index 1565404..c032434 100644
@@ -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

Peter Lieven

author

2014-10-27 09:20 UTC #2 lore raw

PATCH v6 2/6 BlockLimits: introduce max_transfer_length

In reply to Peter Lieven

Signed-off-by Peter Lieven <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
 block.c                   |    4 ++++
 include/block/block_int.h |    3 +++
 2 files changed, 7 insertions(+)

block.c +4 -0

index 88f6d9b..76fcc1d 100644
@@ -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

index 8898c6c..a293e92 100644
@@ -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

Peter Lieven

author

2014-10-27 09:20 UTC #3 lore raw

PATCH v6 3/6 block/iscsi: set max_transfer_length

In reply to Peter Lieven

Copy the max_xfer_len from the BlockLimits VPD or use the maximum value fitting in the CDB.

The helper function sector_limits_lun2qemu is introduced to convert and cap the limits from the VPD to the maximum power of two fitting in an integer; integer is the range for nb_sectors throughout the block layer.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

block/iscsi.c +15 -2

index 233f462..6b229b1 100644
@@ -1447,12 +1447,25 @@ static void iscsi_close(BlockDriverState *bs)
     memset(iscsilun, 0, sizeof(IscsiLun));
 }
 
-static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
+static int sector_limits_lun2qemu(int64_t sector, IscsiLun *iscsilun)
 {
-    IscsiLun *iscsilun = bs->opaque;
+    return MIN(sector_lun2qemu(sector, iscsilun), INT_MAX / 2 + 1);
+}
 
+static void iscsi_refresh_limits(BlockDriverState *bs, Error **errp)
+{
     /* 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 = sector_limits_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

Max Reitz

Reviewed-by on v6 3/6

2014-10-27 09:23 UTC #4 lore raw

Re: [Qemu-devel] [PATCHv6 3/6] block/iscsi: set max_transfer_length

Quoted 8 lines · Peter Lieven

Copy the max_xfer_len from the BlockLimits VPD or use the maximum value fitting in the CDB.
The helper function sector_limits_lun2qemu is introduced to convert and cap the limits from the VPD to the maximum power of two fitting in an integer; integer is the range for nb_sectors throughout the block layer.

Quoted 4 lines

Signed-off-by Peter Lieven <[email protected]>
---
  block/iscsi.c |   17 +++++++++++++++--
  1 file changed, 15 insertions(+), 2 deletions(-)

Okay, I think finding the comment in the commit message through git blame is reasonable.

Reviewed-by Max Reitz <[email protected]>

Peter Lieven

author

2014-10-27 09:20 UTC #5 lore raw

PATCH v6 4/6 block: avoid creating oversized writes in multiwrite_merge

In reply to Peter Lieven

Signed-off-by Peter Lieven <[email protected]>

Reviewed-by Ronnie Sahlberg <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
 block.c |    5 +++++
 1 file changed, 5 insertions(+)

block.c +5 -0

index 76fcc1d..4179341 100644
@@ -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

Peter Lieven

author

2014-10-27 09:21 UTC #6 lore raw

PATCH v6 5/6 block/iscsi: use sector_limits_lun2qemu throughout iscsi_refresh_limits

In reply to Peter Lieven

As Max pointed out there is a hidden cast from int64_t to int for all limits. So use the newly introduced sector_limits_lun2qemu for all limits received from the target.

Signed-off-by Peter Lieven <[email protected]>

---
 block/iscsi.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

block/iscsi.c +10 ~~-10~~

index 6b229b1..a4ba33b 100644
@@ -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 =
+                sector_limits_lun2qemu(iscsilun->bl.max_unmap, iscsilun);
         }
-        bs->bl.discard_alignment = sector_lun2qemu(iscsilun->bl.opt_unmap_gran,
-                                                   iscsilun);
+        bs->bl.discard_alignment =
+            sector_limits_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 =
+            sector_limits_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 =
+            sector_limits_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 =
+        sector_limits_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

Max Reitz

Reviewed-by on v6 5/6

2014-10-27 09:24 UTC #7 lore raw

Re: [Qemu-devel] [PATCHv6 5/6] block/iscsi: use sector_limits_lun2qemu throughout iscsi_refresh_limits

Quoted 8 lines · Peter Lieven

As Max pointed out there is a hidden cast from int64_t to int for all limits. So use the newly introduced sector_limits_lun2qemu for all limits received from the target.
Signed-off-by Peter Lieven <[email protected]>
---
  block/iscsi.c |   20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by Max Reitz <[email protected]>

Peter Lieven

author

2014-10-27 09:21 UTC #8 lore raw

PATCH v6 6/6 block/iscsi: check for oversized requests

In reply to 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 +12 -0

index a4ba33b..111065d 100644
@@ -362,6 +362,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);

@@ -529,6 +535,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

Stefan Hajnoczi

Applied

2014-10-28 13:31 UTC #9 lore raw

In reply to Peter Lieven

Quoted 30 lines · Peter Lieven

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.
v5->v6: - fix parentheses in Patch 1 [Max]
- rename helper function in Patch 3 [Max]
- cap the limits to highest power of 2 fitting in INT_MAX [Max]
- removed full stops in error messages in Patch 6 [Max]
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: use sector_limits_lun2qemu throughout
iscsi_refresh_limits
block/iscsi: check for oversized requests

Quoted 5 lines

 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(-)

Please use "PATCH v6" in the Subject line instead of "PATCHv6". My patch scanning tools missed the series.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan