[Qemu-devel] [PATCHv6 0/6] introduce max_transfer_length

[Qemu-devel] [PATCHv6 0/6] introduce max_transfer_length

[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

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

-- 
1.7.9.5

[Qemu-devel] [PATCHv6 1/6] util: introduce MIN_NON_ZERO

[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 <pl@kamp.de>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/osdep.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1565404..c032434 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -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
-- 
1.7.9.5

[Qemu-devel] [PATCHv6 2/6] BlockLimits: introduce max_transfer_length

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |    4 ++++
 include/block/block_int.h |    3 +++
 2 files changed, 7 insertions(+)

diff --git a/block.c b/block.c
index 88f6d9b..76fcc1d 100644
--- a/block.c
+++ b/block.c
@@ -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);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..a293e92 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -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;
-- 
1.7.9.5

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

[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 <pl@kamp.de>
---
 block/iscsi.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 233f462..6b229b1 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -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,
-- 
1.7.9.5

[Qemu-devel] [PATCHv6 4/6] block: avoid creating oversized writes in multiwrite_merge

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Ronnie Sahlberg <ronniesahlberg@gmail.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block.c b/block.c
index 76fcc1d..4179341 100644
--- a/block.c
+++ b/block.c
@@ -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));
-- 
1.7.9.5

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

[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 <pl@kamp.de>
---
 block/iscsi.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index 6b229b1..a4ba33b 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -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
-- 
1.7.9.5

[Qemu-devel] [PATCHv6 6/6] block/iscsi: check for oversized requests

[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 <mreitz@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/iscsi.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/block/iscsi.c b/block/iscsi.c
index a4ba33b..111065d 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -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;
-- 
1.7.9.5

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

[Max Reitz]

On 2014-10-27 at 10:18, Peter Lieven wrote:
> 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 <pl@kamp.de>
> ---
>   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 <mreitz@redhat.com>

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

[Max Reitz]

On 2014-10-27 at 10:18, Peter Lieven wrote:
> 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 <pl@kamp.de>
> ---
>   block/iscsi.c |   20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCHv6 0/6] introduce max_transfer_length

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On Mon, Oct 27, 2014 at 10:18:42AM +0100, Peter Lieven wrote:
> 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(-)

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

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]