[Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

v2:
 * Default to IOV_MAX instead of INT_MAX [Peter Lieven]

This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".

IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
refuse to take more iovecs per call.  That's actually an implementation detail
of raw-posix and we shouldn't spread that across the codebase.

This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().

By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing but
follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
one will be bothered by the reverse naming anymore.

Suggested-by: Kevin Wolf <kwolf@redhat.com>

Stefan Hajnoczi (4):
  block: add BlockLimits.max_iov field
  block-backend: add blk_get_max_iov()
  block: replace IOV_MAX with BlockLimits.max_iov
  block/mirror: replace IOV_MAX with blk_get_max_iov()

 block/block-backend.c          | 5 +++++
 block/io.c                     | 8 +++++++-
 block/mirror.c                 | 6 ++++--
 hw/block/virtio-blk.c          | 2 +-
 include/block/block_int.h      | 3 +++
 include/sysemu/block-backend.h | 1 +
 6 files changed, 21 insertions(+), 4 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 v2 1/4] block: add BlockLimits.max_iov field

[Stefan Hajnoczi]

The maximum number of struct iovec elements depends on the
BlockDriverState.  The raw-posix and iSCSI protocols have a maximum of
IOV_MAX but others could have different values.

Cc: Peter Lieven <pl@kamp.de>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
v2:
 * Change default from INT_MAX to IOV_MAX [Peter Lieven]
---
 block/io.c                | 5 +++++
 include/block/block_int.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/block/io.c b/block/io.c
index d4bc83b..37bc0dc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -165,9 +165,13 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.max_transfer_length = bs->file->bl.max_transfer_length;
         bs->bl.min_mem_alignment = bs->file->bl.min_mem_alignment;
         bs->bl.opt_mem_alignment = bs->file->bl.opt_mem_alignment;
+        bs->bl.max_iov = bs->file->bl.max_iov;
     } else {
         bs->bl.min_mem_alignment = 512;
         bs->bl.opt_mem_alignment = getpagesize();
+
+        /* Safe default since most protocols use readv()/writev()/etc */
+        bs->bl.max_iov = IOV_MAX;
     }
 
     if (bs->backing_hd) {
@@ -188,6 +192,7 @@ void bdrv_refresh_limits(BlockDriverState *bs, Error **errp)
         bs->bl.min_mem_alignment =
             MAX(bs->bl.min_mem_alignment,
                 bs->backing_hd->bl.min_mem_alignment);
+        bs->bl.max_iov = MIN(bs->bl.max_iov, bs->backing_hd->bl.max_iov);
     }
 
     /* Then let the driver override it */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8996baf..7d578c4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -315,6 +315,9 @@ typedef struct BlockLimits {
 
     /* memory alignment for bounce buffer */
     size_t opt_mem_alignment;
+
+    /* maximum number of iovec elements */
+    int max_iov;
 } BlockLimits;
 
 typedef struct BdrvOpBlocker BdrvOpBlocker;
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 v2 2/4] block-backend: add blk_get_max_iov()

[Stefan Hajnoczi]

Add a function to query BlockLimits.max_iov.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index aee8a12..5108aec 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -777,6 +777,11 @@ int blk_get_max_transfer_length(BlockBackend *blk)
     return blk->bs->bl.max_transfer_length;
 }
 
+int blk_get_max_iov(BlockBackend *blk)
+{
+    return blk->bs->bl.max_iov;
+}
+
 void blk_set_guest_block_size(BlockBackend *blk, int align)
 {
     bdrv_set_guest_block_size(blk->bs, align);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8fc960f..c114440 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -135,6 +135,7 @@ void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
 int blk_get_max_transfer_length(BlockBackend *blk);
+int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
 void *blk_blockalign(BlockBackend *blk, size_t size);
 bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp);
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 v2 3/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

Request merging must not result in a huge request that exceeds the
maximum number of iovec elements.  Use BlockLimits.max_iov instead of
hardcoding IOV_MAX.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c            | 3 ++-
 hw/block/virtio-blk.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 37bc0dc..66ae770 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1854,7 +1854,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
             merge = 1;
         }
 
-        if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1 > IOV_MAX) {
+        if (reqs[outidx].qiov->niov + reqs[i].qiov->niov + 1 >
+            bs->bl.max_iov) {
             merge = 0;
         }
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6aefda4..a186956 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -407,7 +407,7 @@ void virtio_blk_submit_multireq(BlockBackend *blk, MultiReqBuffer *mrb)
             bool merge = true;
 
             /* merge would exceed maximum number of IOVs */
-            if (niov + req->qiov.niov > IOV_MAX) {
+            if (niov + req->qiov.niov > blk_get_max_iov(blk)) {
                 merge = false;
             }
 
-- 
2.4.3

[Qemu-devel] [PATCH for-2.5 v2 4/4] block/mirror: replace IOV_MAX with blk_get_max_iov()

[Stefan Hajnoczi]

Use blk_get_max_iov() instead of hardcoding IOV_MAX, which may not apply
to all BlockDrivers.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/mirror.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index adde12b..74d475b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -161,13 +161,15 @@ static void mirror_read_complete(void *opaque, int ret)
 static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
 {
     BlockDriverState *source = s->common.bs;
-    int nb_sectors, sectors_per_chunk, nb_chunks;
+    int nb_sectors, sectors_per_chunk, nb_chunks, max_iov;
     int64_t end, sector_num, next_chunk, next_sector, hbitmap_next_sector;
     uint64_t delay_ns = 0;
     MirrorOp *op;
     int pnum;
     int64_t ret;
 
+    max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
+
     s->sector_num = hbitmap_iter_next(&s->hbi);
     if (s->sector_num < 0) {
         bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
@@ -244,7 +246,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
             trace_mirror_break_buf_busy(s, nb_chunks, s->in_flight);
             break;
         }
-        if (IOV_MAX < nb_chunks + added_chunks) {
+        if (max_iov < nb_chunks + added_chunks) {
             trace_mirror_break_iov_max(s, nb_chunks, added_chunks);
             break;
         }
-- 
2.4.3

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Fam Zheng]

On Thu, 07/09 10:56, Stefan Hajnoczi wrote:
> v2:
>  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> 
> This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".
> 
> IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> refuse to take more iovecs per call.  That's actually an implementation detail
> of raw-posix and we shouldn't spread that across the codebase.
> 
> This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
> and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().
> 
> By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing but
> follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
> one will be bothered by the reverse naming anymore.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>

Series:

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Kevin Wolf]

Am 09.07.2015 um 11:56 hat Stefan Hajnoczi geschrieben:
> v2:
>  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> 
> This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".
> 
> IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> refuse to take more iovecs per call.  That's actually an implementation detail
> of raw-posix and we shouldn't spread that across the codebase.
> 
> This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
> and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().
> 
> By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing but
> follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
> one will be bothered by the reverse naming anymore.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>

I was just looking for patches marked as for-2.5 and found this one...
Is there a reason it wasn't applied or did we just forget about it?

Kevin

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

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

On Thu, Jul 09, 2015 at 10:56:43AM +0100, Stefan Hajnoczi wrote:
> v2:
>  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> 
> This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".
> 
> IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> refuse to take more iovecs per call.  That's actually an implementation detail
> of raw-posix and we shouldn't spread that across the codebase.
> 
> This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
> and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().
> 
> By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing but
> follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
> one will be bothered by the reverse naming anymore.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> 
> Stefan Hajnoczi (4):
>   block: add BlockLimits.max_iov field
>   block-backend: add blk_get_max_iov()
>   block: replace IOV_MAX with BlockLimits.max_iov
>   block/mirror: replace IOV_MAX with blk_get_max_iov()
> 
>  block/block-backend.c          | 5 +++++
>  block/io.c                     | 8 +++++++-
>  block/mirror.c                 | 6 ++++--
>  hw/block/virtio-blk.c          | 2 +-
>  include/block/block_int.h      | 3 +++
>  include/sysemu/block-backend.h | 1 +
>  6 files changed, 21 insertions(+), 4 deletions(-)
> 
> -- 
> 2.4.3
> 

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

Stefan

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

Re: [Qemu-devel] [PATCH for-2.5 v2 0/4] block: replace IOV_MAX with BlockLimits.max_iov

[Stefan Hajnoczi]

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

On Wed, Nov 18, 2015 at 03:51:29PM +0100, Kevin Wolf wrote:
> Am 09.07.2015 um 11:56 hat Stefan Hajnoczi geschrieben:
> > v2:
> >  * Default to IOV_MAX instead of INT_MAX [Peter Lieven]
> > 
> > This series depends on "[PATCH] block/mirror: limit qiov to IOV_MAX elements".
> > 
> > IOV_MAX has been hardcoded in several places since preadv()/pwritev()/etc
> > refuse to take more iovecs per call.  That's actually an implementation detail
> > of raw-posix and we shouldn't spread that across the codebase.
> > 
> > This series adds BlockLimits.max_iov (similar to BlockLimits.max_transfer_len
> > and other fields).  Current IOV_MAX users are converted to blk_get_max_iov().
> > 
> > By the way, the IOV_MAX vs BlockLimits.max_iov naming is slightly confusing but
> > follows the BlockLimits field naming convention.  Once IOV_MAX is banished no
> > one will be bothered by the reverse naming anymore.
> > 
> > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> 
> I was just looking for patches marked as for-2.5 and found this one...
> Is there a reason it wasn't applied or did we just forget about it?

This series was forgotten.  I have applied it for QEMU 2.6 since it is
not a bug fix.

Stefan

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