[Qemu-devel] [PATCH 0/2] block/io: refactor padding

[Qemu-devel] [PATCH 0/2] block/io: refactor padding

[Vladimir Sementsov-Ogievskiy]

Hi all! Here is unifying of padding code in block/io.c, the main
patch is 02.

Vladimir Sementsov-Ogievskiy (2):
  util/iov: introduce qemu_iovec_init_extended
  block/io: refactor padding

 include/qemu/iov.h |   5 +
 block/io.c         | 344 +++++++++++++++++++++++----------------------
 util/iov.c         |  89 ++++++++++++
 3 files changed, 273 insertions(+), 165 deletions(-)

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended

[Vladimir Sementsov-Ogievskiy]

Introduce new initialization API, to create requests with padding. Will
be used in the following patch.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qemu/iov.h |  5 +++
 util/iov.c         | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/include/qemu/iov.h b/include/qemu/iov.h
index 48b45987b7..1c5be66102 100644
--- a/include/qemu/iov.h
+++ b/include/qemu/iov.h
@@ -199,6 +199,11 @@ static inline void *qemu_iovec_buf(QEMUIOVector *qiov)
 
 void qemu_iovec_init(QEMUIOVector *qiov, int alloc_hint);
 void qemu_iovec_init_external(QEMUIOVector *qiov, struct iovec *iov, int niov);
+void qemu_iovec_init_extended(
+        QEMUIOVector *qiov,
+        void *left, size_t left_len,
+        QEMUIOVector *middle, size_t middle_offset, size_t middle_len,
+        void *right, size_t right_len);
 void qemu_iovec_add(QEMUIOVector *qiov, void *base, size_t len);
 void qemu_iovec_concat(QEMUIOVector *dst,
                        QEMUIOVector *src, size_t soffset, size_t sbytes);
diff --git a/util/iov.c b/util/iov.c
index 74e6ca8ed7..6bfd609998 100644
--- a/util/iov.c
+++ b/util/iov.c
@@ -353,6 +353,95 @@ void qemu_iovec_concat(QEMUIOVector *dst,
     qemu_iovec_concat_iov(dst, src->iov, src->niov, soffset, sbytes);
 }
 
+/*
+ * qiov_find_iov
+ *
+ * Return iov, where byte at @offset (in @qiov) is.
+ * Update @offset to be offset inside that iov to the smae byte.
+ */
+static struct iovec *qiov_find_iov(QEMUIOVector *qiov, size_t *offset)
+{
+    struct iovec *iov = qiov->iov;
+
+    assert(*offset < qiov->size);
+
+    while (*offset >= iov->iov_len) {
+        *offset -= iov->iov_len;
+        iov++;
+    }
+
+    return iov;
+}
+
+/*
+ * qiov_slice
+ *
+ * Fund subarray of iovec's, containing requested range. @head would
+ * be offset in first iov (retruned by the function), @tail would be
+ * count of extra bytes in last iov (returned iov + @niov - 1).
+ */
+static struct iovec *qiov_slice(QEMUIOVector *qiov,
+                                size_t offset, size_t len,
+                                size_t *head, size_t *tail, int *niov)
+{
+    struct iovec *iov = qiov_find_iov(qiov, &offset), *end_iov;
+    size_t end_offset;
+
+    assert(offset + len <= qiov->size);
+
+    end_offset = iov->iov_len;
+    end_iov = iov + 1;
+
+    while (end_offset - offset < len) {
+        end_offset += end_iov->iov_len;
+        end_iov++;
+    }
+
+    *niov = end_iov - iov;
+    *head = offset;
+    *tail = (end_offset - offset) - len;
+
+    return iov;
+}
+
+/*
+ * Compile new iovec, combining @head_buf buffer, sub-qiov of @mid_qiov,
+ * and @tail_buf buffer into new qiov.
+ */
+void qemu_iovec_init_extended(
+        QEMUIOVector *qiov,
+        void *head_buf, size_t head_len,
+        QEMUIOVector *mid_qiov, size_t mid_offset, size_t mid_len,
+        void *tail_buf, size_t tail_len)
+{
+    size_t mid_head, mid_tail;
+    int niov;
+    struct iovec *p, *mid_iov = qiov_slice(mid_qiov, mid_offset, mid_len,
+                                           &mid_head, &mid_tail, &niov);
+
+    assert(niov);
+    qiov->niov = qiov->nalloc = niov + !!head_len + !!tail_len;
+    qiov->size = head_len + mid_len + tail_len;
+
+    p = qiov->iov = g_new(struct iovec, qiov->niov);
+    if (head_len) {
+        p->iov_base = head_buf;
+        p->iov_len = head_len;
+        p++;
+    }
+
+    memcpy(p, mid_iov, niov * sizeof(*p));
+    p[0].iov_base = (uint8_t *)p[0].iov_base + mid_head;
+    p[0].iov_len -= mid_head;
+    p[niov - 1].iov_len -= mid_tail;
+    p += niov;
+
+    if (tail_len) {
+        p->iov_base = tail_buf;
+        p->iov_len = tail_len;
+    }
+}
+
 /*
  * Check if the contents of the iovecs are all zero
  */
-- 
2.18.0

[Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Vladimir Sementsov-Ogievskiy]

We have similar padding code in bdrv_co_pwritev,
bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/io.c | 344 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 179 insertions(+), 165 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3134a60a48..840e276269 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1344,28 +1344,155 @@ out:
 }
 
 /*
- * Handle a read request in coroutine context
+ * Request padding
+ *
+ *  |<---- align ---->|                     |<---- align ---->|
+ *  |<- head ->|<------------ bytes ------------>|<-- tail -->|
+ *  |          |      |                     |    |            |
+ * -*----------$------*-------- ... --------*----$------------*---
+ *  |          |      |                     |    |            |
+ *  |          offset |                     |    end          |
+ *  ALIGN_UP(offset)  ALIGN_DOWN(offset)    ALIGN_DOWN(end)   ALIGN_UP(end)
+ *  [buf   ... )                            [tail_buf         )
+ *
+ * @buf is an aligned allocation needed to store @head and @tail paddings. @head
+ * is placed at the beginning of @buf and @tail at the @end.
+ *
+ * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk
+ * around tail, if tail exists.
+ *
+ * @merge_reads is true for small requests,
+ * if @buf_len == @head + bytes + @tail. In this case it is possible that both
+ * head and tail exist but @buf_len == align and @tail_buf == @buf.
  */
+typedef struct BdrvRequestPadding {
+    uint8_t *buf;
+    size_t buf_len;
+    uint8_t *tail_buf;
+    size_t head;
+    size_t tail;
+    bool merge_reads;
+    QEMUIOVector local_qiov;
+} BdrvRequestPadding;
+
+static bool bdrv_init_padding(BlockDriverState *bs,
+                              int64_t offset, int64_t bytes,
+                              BdrvRequestPadding *pad)
+{
+    uint64_t align = bs->bl.request_alignment;
+    size_t sum;
+
+    memset(pad, 0, sizeof(*pad));
+
+    pad->head = offset & (align - 1);
+    pad->tail = ((offset + bytes) & (align - 1));
+    if (pad->tail) {
+        pad->tail = align - pad->tail;
+    }
+
+    if ((!pad->head && !pad->tail) || !bytes) {
+        return false;
+    }
+
+    sum = pad->head + bytes + pad->tail;
+    pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align;
+    pad->buf = qemu_blockalign(bs, pad->buf_len);
+    pad->merge_reads = sum == pad->buf_len;
+    if (pad->tail) {
+        pad->tail_buf = pad->buf + pad->buf_len - align;
+    }
+
+    return true;
+}
+
+static int bdrv_padding_read(BdrvChild *child,
+                             BdrvTrackedRequest *req,
+                             BdrvRequestPadding *pad,
+                             bool zero_middle)
+{
+    QEMUIOVector local_qiov;
+    BlockDriverState *bs = child->bs;
+    uint64_t align = bs->bl.request_alignment;
+    int ret;
+
+    assert(req->serialising && pad->buf);
+
+    if (pad->head || pad->merge_reads) {
+        uint64_t bytes = pad->merge_reads ? pad->buf_len : align;
+
+        qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
+
+        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
+        ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
+                                  align, &local_qiov, 0);
+        if (ret < 0) {
+            return ret;
+        }
+        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
+
+        if (pad->merge_reads) {
+            goto zero_mem;
+        }
+    }
+
+    if (pad->tail) {
+        qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align);
+
+        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
+        ret = bdrv_aligned_preadv(
+                child, req,
+                req->overlap_offset + req->overlap_bytes - align,
+                align, align, &local_qiov, 0);
+        if (ret < 0) {
+            return ret;
+        }
+        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
+    }
+
+zero_mem:
+    if (zero_middle) {
+        memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail);
+    }
+
+    return 0;
+}
+
+static void bdrv_padding_destroy(BdrvRequestPadding *pad)
+{
+    if (pad->buf) {
+        qemu_vfree(pad->buf);
+        qemu_iovec_destroy(&pad->local_qiov);
+    }
+}
+
+static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov,
+                                      int64_t *offset, unsigned int *bytes,
+                                      BdrvRequestPadding *pad)
+{
+    bdrv_init_padding(bs, *offset, *bytes, pad);
+    if (!pad->buf) {
+        return qiov;
+    }
+
+    qemu_iovec_init_extended(&pad->local_qiov, pad->buf, pad->head,
+                             qiov, 0, *bytes,
+                             pad->buf + pad->buf_len - pad->tail, pad->tail);
+    *bytes += pad->head + pad->tail;
+    *offset -= pad->head;
+
+    return &pad->local_qiov;
+}
+
 int coroutine_fn bdrv_co_preadv(BdrvChild *child,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags)
 {
     BlockDriverState *bs = child->bs;
-    BlockDriver *drv = bs->drv;
     BdrvTrackedRequest req;
-
-    uint64_t align = bs->bl.request_alignment;
-    uint8_t *head_buf = NULL;
-    uint8_t *tail_buf = NULL;
-    QEMUIOVector local_qiov;
-    bool use_local_qiov = false;
+    BdrvRequestPadding pad;
     int ret;
 
-    trace_bdrv_co_preadv(child->bs, offset, bytes, flags);
-
-    if (!drv) {
-        return -ENOMEDIUM;
-    }
+    trace_bdrv_co_preadv(bs, offset, bytes, flags);
 
     ret = bdrv_check_byte_request(bs, offset, bytes);
     if (ret < 0) {
@@ -1379,43 +1506,16 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
         flags |= BDRV_REQ_COPY_ON_READ;
     }
 
-    /* Align read if necessary by padding qiov */
-    if (offset & (align - 1)) {
-        head_buf = qemu_blockalign(bs, align);
-        qemu_iovec_init(&local_qiov, qiov->niov + 2);
-        qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
-        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-        use_local_qiov = true;
-
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
-    }
-
-    if ((offset + bytes) & (align - 1)) {
-        if (!use_local_qiov) {
-            qemu_iovec_init(&local_qiov, qiov->niov + 1);
-            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-            use_local_qiov = true;
-        }
-        tail_buf = qemu_blockalign(bs, align);
-        qemu_iovec_add(&local_qiov, tail_buf,
-                       align - ((offset + bytes) & (align - 1)));
-
-        bytes = ROUND_UP(bytes, align);
-    }
+    qiov = bdrv_pad_request(bs, qiov, &offset, &bytes, &pad);
 
     tracked_request_begin(&req, bs, offset, bytes, BDRV_TRACKED_READ);
-    ret = bdrv_aligned_preadv(child, &req, offset, bytes, align,
-                              use_local_qiov ? &local_qiov : qiov,
-                              flags);
+    ret = bdrv_aligned_preadv(child, &req, offset, bytes,
+                              bs->bl.request_alignment,
+                              qiov, flags);
     tracked_request_end(&req);
     bdrv_dec_in_flight(bs);
 
-    if (use_local_qiov) {
-        qemu_iovec_destroy(&local_qiov);
-        qemu_vfree(head_buf);
-        qemu_vfree(tail_buf);
-    }
+    bdrv_padding_destroy(&pad);
 
     return ret;
 }
@@ -1711,44 +1811,34 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
                                                 BdrvTrackedRequest *req)
 {
     BlockDriverState *bs = child->bs;
-    uint8_t *buf = NULL;
     QEMUIOVector local_qiov;
     uint64_t align = bs->bl.request_alignment;
-    unsigned int head_padding_bytes, tail_padding_bytes;
     int ret = 0;
+    bool padding;
+    BdrvRequestPadding pad;
 
-    head_padding_bytes = offset & (align - 1);
-    tail_padding_bytes = (align - (offset + bytes)) & (align - 1);
-
-
-    assert(flags & BDRV_REQ_ZERO_WRITE);
-    if (head_padding_bytes || tail_padding_bytes) {
-        buf = qemu_blockalign(bs, align);
-        qemu_iovec_init_buf(&local_qiov, buf, align);
-    }
-    if (head_padding_bytes) {
-        uint64_t zero_bytes = MIN(bytes, align - head_padding_bytes);
-
-        /* RMW the unaligned part before head. */
+    padding = bdrv_init_padding(bs, offset, bytes, &pad);
+    if (padding) {
         mark_request_serialising(req, align);
         wait_serialising_requests(req);
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
-        ret = bdrv_aligned_preadv(child, req, offset & ~(align - 1), align,
-                                  align, &local_qiov, 0);
-        if (ret < 0) {
-            goto fail;
-        }
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
 
-        memset(buf + head_padding_bytes, 0, zero_bytes);
-        ret = bdrv_aligned_pwritev(child, req, offset & ~(align - 1), align,
-                                   align, &local_qiov,
-                                   flags & ~BDRV_REQ_ZERO_WRITE);
-        if (ret < 0) {
-            goto fail;
+        bdrv_padding_read(child, req, &pad, true);
+
+        if (pad.head || pad.merge_reads) {
+            int64_t aligned_offset = offset & ~(align - 1);
+            int64_t write_bytes = pad.merge_reads ? pad.buf_len : align;
+
+            qemu_iovec_init_buf(&local_qiov, pad.buf, write_bytes);
+            ret = bdrv_aligned_pwritev(child, req, aligned_offset, write_bytes,
+                                       align, &local_qiov,
+                                       flags & ~BDRV_REQ_ZERO_WRITE);
+            if (ret < 0 || pad.merge_reads) {
+                /* Error or all work is done */
+                goto out;
+            }
+            offset += write_bytes - pad.head;
+            bytes -= write_bytes - pad.head;
         }
-        offset += zero_bytes;
-        bytes -= zero_bytes;
     }
 
     assert(!bytes || (offset & (align - 1)) == 0);
@@ -1758,7 +1848,7 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
         ret = bdrv_aligned_pwritev(child, req, offset, aligned_bytes, align,
                                    NULL, flags);
         if (ret < 0) {
-            goto fail;
+            goto out;
         }
         bytes -= aligned_bytes;
         offset += aligned_bytes;
@@ -1766,26 +1856,17 @@ static int coroutine_fn bdrv_co_do_zero_pwritev(BdrvChild *child,
 
     assert(!bytes || (offset & (align - 1)) == 0);
     if (bytes) {
-        assert(align == tail_padding_bytes + bytes);
-        /* RMW the unaligned part after tail. */
-        mark_request_serialising(req, align);
-        wait_serialising_requests(req);
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
-        ret = bdrv_aligned_preadv(child, req, offset, align,
-                                  align, &local_qiov, 0);
-        if (ret < 0) {
-            goto fail;
-        }
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
+        assert(align == pad.tail + bytes);
 
-        memset(buf, 0, bytes);
+        qemu_iovec_init_buf(&local_qiov, pad.tail_buf, align);
         ret = bdrv_aligned_pwritev(child, req, offset, align, align,
                                    &local_qiov, flags & ~BDRV_REQ_ZERO_WRITE);
     }
-fail:
-    qemu_vfree(buf);
-    return ret;
 
+out:
+    bdrv_padding_destroy(&pad);
+
+    return ret;
 }
 
 /*
@@ -1798,10 +1879,7 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
     BlockDriverState *bs = child->bs;
     BdrvTrackedRequest req;
     uint64_t align = bs->bl.request_alignment;
-    uint8_t *head_buf = NULL;
-    uint8_t *tail_buf = NULL;
-    QEMUIOVector local_qiov;
-    bool use_local_qiov = false;
+    BdrvRequestPadding pad;
     int ret;
 
     trace_bdrv_co_pwritev(child->bs, offset, bytes, flags);
@@ -1828,86 +1906,22 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
         goto out;
     }
 
-    if (offset & (align - 1)) {
-        QEMUIOVector head_qiov;
-
+    qiov = bdrv_pad_request(bs, qiov, &offset, &bytes, &pad);
+    if (pad.head || pad.tail) {
         mark_request_serialising(&req, align);
         wait_serialising_requests(&req);
-
-        head_buf = qemu_blockalign(bs, align);
-        qemu_iovec_init_buf(&head_qiov, head_buf, align);
-
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
-        ret = bdrv_aligned_preadv(child, &req, offset & ~(align - 1), align,
-                                  align, &head_qiov, 0);
-        if (ret < 0) {
-            goto fail;
-        }
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
-
-        qemu_iovec_init(&local_qiov, qiov->niov + 2);
-        qemu_iovec_add(&local_qiov, head_buf, offset & (align - 1));
-        qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-        use_local_qiov = true;
-
-        bytes += offset & (align - 1);
-        offset = offset & ~(align - 1);
-
-        /* We have read the tail already if the request is smaller
-         * than one aligned block.
-         */
-        if (bytes < align) {
-            qemu_iovec_add(&local_qiov, head_buf + bytes, align - bytes);
-            bytes = align;
-        }
-    }
-
-    if ((offset + bytes) & (align - 1)) {
-        QEMUIOVector tail_qiov;
-        size_t tail_bytes;
-        bool waited;
-
-        mark_request_serialising(&req, align);
-        waited = wait_serialising_requests(&req);
-        assert(!waited || !use_local_qiov);
-
-        tail_buf = qemu_blockalign(bs, align);
-        qemu_iovec_init_buf(&tail_qiov, tail_buf, align);
-
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
-        ret = bdrv_aligned_preadv(child, &req, (offset + bytes) & ~(align - 1),
-                                  align, align, &tail_qiov, 0);
-        if (ret < 0) {
-            goto fail;
-        }
-        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
-
-        if (!use_local_qiov) {
-            qemu_iovec_init(&local_qiov, qiov->niov + 1);
-            qemu_iovec_concat(&local_qiov, qiov, 0, qiov->size);
-            use_local_qiov = true;
-        }
-
-        tail_bytes = (offset + bytes) & (align - 1);
-        qemu_iovec_add(&local_qiov, tail_buf + tail_bytes, align - tail_bytes);
-
-        bytes = ROUND_UP(bytes, align);
+        bdrv_padding_read(child, &req, &pad, false);
     }
 
     ret = bdrv_aligned_pwritev(child, &req, offset, bytes, align,
-                               use_local_qiov ? &local_qiov : qiov,
-                               flags);
+                               qiov, flags);
 
-fail:
+    bdrv_padding_destroy(&pad);
 
-    if (use_local_qiov) {
-        qemu_iovec_destroy(&local_qiov);
-    }
-    qemu_vfree(head_buf);
-    qemu_vfree(tail_buf);
 out:
     tracked_request_end(&req);
     bdrv_dec_in_flight(bs);
+
     return ret;
 }
 
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/2] util/iov: introduce qemu_iovec_init_extended

[Stefan Hajnoczi]

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

On Tue, May 28, 2019 at 11:45:43AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> diff --git a/util/iov.c b/util/iov.c
> index 74e6ca8ed7..6bfd609998 100644
> --- a/util/iov.c
> +++ b/util/iov.c
> @@ -353,6 +353,95 @@ void qemu_iovec_concat(QEMUIOVector *dst,
>      qemu_iovec_concat_iov(dst, src->iov, src->niov, soffset, sbytes);
>  }
>  
> +/*
> + * qiov_find_iov
> + *
> + * Return iov, where byte at @offset (in @qiov) is.
> + * Update @offset to be offset inside that iov to the smae byte.

s/smae/same/

> + */
> +static struct iovec *qiov_find_iov(QEMUIOVector *qiov, size_t *offset)
> +{
> +    struct iovec *iov = qiov->iov;
> +
> +    assert(*offset < qiov->size);
> +
> +    while (*offset >= iov->iov_len) {
> +        *offset -= iov->iov_len;
> +        iov++;
> +    }
> +
> +    return iov;
> +}
> +
> +/*
> + * qiov_slice
> + *
> + * Fund subarray of iovec's, containing requested range. @head would

s/Fund/Find/

> + * be offset in first iov (retruned by the function), @tail would be

s/retruned/returned/

> + * count of extra bytes in last iov (returned iov + @niov - 1).
> + */
> +static struct iovec *qiov_slice(QEMUIOVector *qiov,
> +                                size_t offset, size_t len,
> +                                size_t *head, size_t *tail, int *niov)
> +{
> +    struct iovec *iov = qiov_find_iov(qiov, &offset), *end_iov;
> +    size_t end_offset;
> +
> +    assert(offset + len <= qiov->size);

offset has already been modified by qiov_find_iov() in iov's
initializer so this comparison is meaningless.  Fix:

  struct iovec *iov;
  struct iovec *end_iov;
  size_t end_offset;

  assert(offset + len <= qiov->size);

  iov = qiov_find_iov(qiov, &offset);

Perhaps qiov_find_iov() shouldn't reuse the offset argument for two
different things (the offset from the beginning of qiov and the offset
from the beginning of the returned iovec).  This would eliminate this
class of bugs.

> +
> +    end_offset = iov->iov_len;
> +    end_iov = iov + 1;
> +
> +    while (end_offset - offset < len) {
> +        end_offset += end_iov->iov_len;
> +        end_iov++;
> +    }

Hmm...this looks like qiov_find_iov().  Can this function be implemented
in less code using two calls to qiov_find_iov() to find the first and
last iovecs?

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

Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Stefan Hajnoczi]

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

On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We have similar padding code in bdrv_co_pwritev,
> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
> it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/io.c | 344 ++++++++++++++++++++++++++++-------------------------

Hmmm...this adds more lines than it removes.  O_o

Merging a change like this can still be useful but there's a risk of
making the code harder to understand due to the additional layers of
abstraction.

>  1 file changed, 179 insertions(+), 165 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 3134a60a48..840e276269 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1344,28 +1344,155 @@ out:
>  }
>  
>  /*
> - * Handle a read request in coroutine context
> + * Request padding
> + *
> + *  |<---- align ---->|                     |<---- align ---->|
> + *  |<- head ->|<------------ bytes ------------>|<-- tail -->|
> + *  |          |      |                     |    |            |
> + * -*----------$------*-------- ... --------*----$------------*---
> + *  |          |      |                     |    |            |
> + *  |          offset |                     |    end          |
> + *  ALIGN_UP(offset)  ALIGN_DOWN(offset)    ALIGN_DOWN(end)   ALIGN_UP(end)

Are ALIGN_UP(offset) and ALIGN_DOWN(offset) in the wrong order?

> + *  [buf   ... )                            [tail_buf         )
> + *
> + * @buf is an aligned allocation needed to store @head and @tail paddings. @head
> + * is placed at the beginning of @buf and @tail at the @end.
> + *
> + * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk
> + * around tail, if tail exists.
> + *
> + * @merge_reads is true for small requests,
> + * if @buf_len == @head + bytes + @tail. In this case it is possible that both
> + * head and tail exist but @buf_len == align and @tail_buf == @buf.
>   */
> +typedef struct BdrvRequestPadding {
> +    uint8_t *buf;
> +    size_t buf_len;
> +    uint8_t *tail_buf;
> +    size_t head;
> +    size_t tail;
> +    bool merge_reads;
> +    QEMUIOVector local_qiov;
> +} BdrvRequestPadding;
> +
> +static bool bdrv_init_padding(BlockDriverState *bs,
> +                              int64_t offset, int64_t bytes,
> +                              BdrvRequestPadding *pad)
> +{
> +    uint64_t align = bs->bl.request_alignment;
> +    size_t sum;
> +
> +    memset(pad, 0, sizeof(*pad));
> +
> +    pad->head = offset & (align - 1);
> +    pad->tail = ((offset + bytes) & (align - 1));
> +    if (pad->tail) {
> +        pad->tail = align - pad->tail;
> +    }
> +
> +    if ((!pad->head && !pad->tail) || !bytes) {
> +        return false;
> +    }
> +
> +    sum = pad->head + bytes + pad->tail;
> +    pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align;
> +    pad->buf = qemu_blockalign(bs, pad->buf_len);
> +    pad->merge_reads = sum == pad->buf_len;
> +    if (pad->tail) {
> +        pad->tail_buf = pad->buf + pad->buf_len - align;
> +    }
> +
> +    return true;
> +}
> +
> +static int bdrv_padding_read(BdrvChild *child,
> +                             BdrvTrackedRequest *req,
> +                             BdrvRequestPadding *pad,
> +                             bool zero_middle)
> +{
> +    QEMUIOVector local_qiov;
> +    BlockDriverState *bs = child->bs;
> +    uint64_t align = bs->bl.request_alignment;
> +    int ret;
> +
> +    assert(req->serialising && pad->buf);
> +
> +    if (pad->head || pad->merge_reads) {
> +        uint64_t bytes = pad->merge_reads ? pad->buf_len : align;
> +
> +        qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
> +
> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);

PWRITEV?  That's unexpected for a function called bdrv_padding_read().
Please rename this to bdrv_padding_rmw_read() so it's clear this is part
of a read-modify-write operation, not a regular read.

> +        ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
> +                                  align, &local_qiov, 0);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
> +
> +        if (pad->merge_reads) {
> +            goto zero_mem;
> +        }
> +    }
> +
> +    if (pad->tail) {
> +        qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align);
> +
> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
> +        ret = bdrv_aligned_preadv(
> +                child, req,
> +                req->overlap_offset + req->overlap_bytes - align,
> +                align, align, &local_qiov, 0);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
> +    }
> +
> +zero_mem:
> +    if (zero_middle) {
> +        memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail);
> +    }
> +
> +    return 0;
> +}
> +
> +static void bdrv_padding_destroy(BdrvRequestPadding *pad)
> +{
> +    if (pad->buf) {
> +        qemu_vfree(pad->buf);
> +        qemu_iovec_destroy(&pad->local_qiov);
> +    }
> +}
> +
> +static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov,
> +                                      int64_t *offset, unsigned int *bytes,
> +                                      BdrvRequestPadding *pad)

Doc comment missing?

> +{
> +    bdrv_init_padding(bs, *offset, *bytes, pad);
> +    if (!pad->buf) {
> +        return qiov;
> +    }

I think there's no need to peek at pad->buf:

  if (!bdrv_init_padding(bs, *offset, *bytes, pad)) {
      return qiov;
  }

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

Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Vladimir Sementsov-Ogievskiy]

31.05.2019 13:51, Stefan Hajnoczi wrote:
> On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> We have similar padding code in bdrv_co_pwritev,
>> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
>> it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/io.c | 344 ++++++++++++++++++++++++++++-------------------------
> 
> Hmmm...this adds more lines than it removes.  O_o

It's near to be the same size, and keep in mind big comment.

> 
> Merging a change like this can still be useful but there's a risk of
> making the code harder to understand due to the additional layers of
> abstraction.

It's a preparation for adding qiov_offset parameter to read/write path. Seems
correct to unify similar things, which I'm going to change. And I really want
to make code more understandable than it was.. But my view is not general
of course.

> 
>>   1 file changed, 179 insertions(+), 165 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 3134a60a48..840e276269 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1344,28 +1344,155 @@ out:
>>   }
>>   
>>   /*
>> - * Handle a read request in coroutine context
>> + * Request padding
>> + *
>> + *  |<---- align ---->|                     |<---- align ---->|
>> + *  |<- head ->|<------------ bytes ------------>|<-- tail -->|
>> + *  |          |      |                     |    |            |
>> + * -*----------$------*-------- ... --------*----$------------*---
>> + *  |          |      |                     |    |            |
>> + *  |          offset |                     |    end          |
>> + *  ALIGN_UP(offset)  ALIGN_DOWN(offset)    ALIGN_DOWN(end)   ALIGN_UP(end)
> 
> Are ALIGN_UP(offset) and ALIGN_DOWN(offset) in the wrong order?

yes :(

> 
>> + *  [buf   ... )                            [tail_buf         )
>> + *
>> + * @buf is an aligned allocation needed to store @head and @tail paddings. @head
>> + * is placed at the beginning of @buf and @tail at the @end.
>> + *
>> + * @tail_buf is a pointer to sub-buffer, corresponding to align-sized chunk
>> + * around tail, if tail exists.
>> + *
>> + * @merge_reads is true for small requests,
>> + * if @buf_len == @head + bytes + @tail. In this case it is possible that both
>> + * head and tail exist but @buf_len == align and @tail_buf == @buf.
>>    */
>> +typedef struct BdrvRequestPadding {
>> +    uint8_t *buf;
>> +    size_t buf_len;
>> +    uint8_t *tail_buf;
>> +    size_t head;
>> +    size_t tail;
>> +    bool merge_reads;
>> +    QEMUIOVector local_qiov;
>> +} BdrvRequestPadding;
>> +
>> +static bool bdrv_init_padding(BlockDriverState *bs,
>> +                              int64_t offset, int64_t bytes,
>> +                              BdrvRequestPadding *pad)
>> +{
>> +    uint64_t align = bs->bl.request_alignment;
>> +    size_t sum;
>> +
>> +    memset(pad, 0, sizeof(*pad));
>> +
>> +    pad->head = offset & (align - 1);
>> +    pad->tail = ((offset + bytes) & (align - 1));
>> +    if (pad->tail) {
>> +        pad->tail = align - pad->tail;
>> +    }
>> +
>> +    if ((!pad->head && !pad->tail) || !bytes) {
>> +        return false;
>> +    }
>> +
>> +    sum = pad->head + bytes + pad->tail;
>> +    pad->buf_len = (sum > align && pad->head && pad->tail) ? 2 * align : align;
>> +    pad->buf = qemu_blockalign(bs, pad->buf_len);
>> +    pad->merge_reads = sum == pad->buf_len;
>> +    if (pad->tail) {
>> +        pad->tail_buf = pad->buf + pad->buf_len - align;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +static int bdrv_padding_read(BdrvChild *child,
>> +                             BdrvTrackedRequest *req,
>> +                             BdrvRequestPadding *pad,
>> +                             bool zero_middle)
>> +{
>> +    QEMUIOVector local_qiov;
>> +    BlockDriverState *bs = child->bs;
>> +    uint64_t align = bs->bl.request_alignment;
>> +    int ret;
>> +
>> +    assert(req->serialising && pad->buf);
>> +
>> +    if (pad->head || pad->merge_reads) {
>> +        uint64_t bytes = pad->merge_reads ? pad->buf_len : align;
>> +
>> +        qemu_iovec_init_buf(&local_qiov, pad->buf, bytes);
>> +
>> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_HEAD);
> 
> PWRITEV?  That's unexpected for a function called bdrv_padding_read().
> Please rename this to bdrv_padding_rmw_read() so it's clear this is part
> of a read-modify-write operation, not a regular read.
> 
>> +        ret = bdrv_aligned_preadv(child, req, req->overlap_offset, bytes,
>> +                                  align, &local_qiov, 0);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_HEAD);
>> +
>> +        if (pad->merge_reads) {
>> +            goto zero_mem;
>> +        }
>> +    }
>> +
>> +    if (pad->tail) {
>> +        qemu_iovec_init_buf(&local_qiov, pad->tail_buf, align);
>> +
>> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_TAIL);
>> +        ret = bdrv_aligned_preadv(
>> +                child, req,
>> +                req->overlap_offset + req->overlap_bytes - align,
>> +                align, align, &local_qiov, 0);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +        bdrv_debug_event(bs, BLKDBG_PWRITEV_RMW_AFTER_TAIL);
>> +    }
>> +
>> +zero_mem:
>> +    if (zero_middle) {
>> +        memset(pad->buf + pad->head, 0, pad->buf_len - pad->head - pad->tail);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void bdrv_padding_destroy(BdrvRequestPadding *pad)
>> +{
>> +    if (pad->buf) {
>> +        qemu_vfree(pad->buf);
>> +        qemu_iovec_destroy(&pad->local_qiov);
>> +    }
>> +}
>> +
>> +static QEMUIOVector *bdrv_pad_request(BlockDriverState *bs, QEMUIOVector *qiov,
>> +                                      int64_t *offset, unsigned int *bytes,
>> +                                      BdrvRequestPadding *pad)
> 
> Doc comment missing?
> 
>> +{
>> +    bdrv_init_padding(bs, *offset, *bytes, pad);
>> +    if (!pad->buf) {
>> +        return qiov;
>> +    }
> 
> I think there's no need to peek at pad->buf:
> 
>    if (!bdrv_init_padding(bs, *offset, *bytes, pad)) {
>        return qiov;
>    }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Kevin Wolf]

Am 31.05.2019 um 16:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 31.05.2019 13:51, Stefan Hajnoczi wrote:
> > On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> We have similar padding code in bdrv_co_pwritev,
> >> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
> >> it.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> ---
> >>   block/io.c | 344 ++++++++++++++++++++++++++++-------------------------
> > 
> > Hmmm...this adds more lines than it removes.  O_o
> 
> It's near to be the same size, and keep in mind big comment.

If you take the whole series into account, it looks even less
favourable, despite some comments:

3 files changed, 273 insertions(+), 165 deletions(-)

> > 
> > Merging a change like this can still be useful but there's a risk of
> > making the code harder to understand due to the additional layers of
> > abstraction.
> 
> It's a preparation for adding qiov_offset parameter to read/write path. Seems
> correct to unify similar things, which I'm going to change. And I really want
> to make code more understandable than it was.. But my view is not general
> of course.

Depending on the changes you're going to make (e.g. adding more users of
the same functionality, or making the duplicated code much larger), this
can be a good justification even if the code size increases.

I'd suggest to add the explanation (like 'This is in preparation for
...') to the commit message then.

Kevin

Re: [Qemu-devel] [PATCH 2/2] block/io: refactor padding

[Vladimir Sementsov-Ogievskiy]

31.05.2019 18:49, Kevin Wolf wrote:
> Am 31.05.2019 um 16:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 31.05.2019 13:51, Stefan Hajnoczi wrote:
>>> On Tue, May 28, 2019 at 11:45:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> We have similar padding code in bdrv_co_pwritev,
>>>> bdrv_co_do_pwrite_zeroes and bdrv_co_preadv. Let's combine and unify
>>>> it.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/io.c | 344 ++++++++++++++++++++++++++++-------------------------
>>>
>>> Hmmm...this adds more lines than it removes.  O_o
>>
>> It's near to be the same size, and keep in mind big comment.
> 
> If you take the whole series into account, it looks even less
> favourable, despite some comments:
> 
> 3 files changed, 273 insertions(+), 165 deletions(-)
> 
>>>
>>> Merging a change like this can still be useful but there's a risk of
>>> making the code harder to understand due to the additional layers of
>>> abstraction.
>>
>> It's a preparation for adding qiov_offset parameter to read/write path. Seems
>> correct to unify similar things, which I'm going to change. And I really want
>> to make code more understandable than it was.. But my view is not general
>> of course.
> 
> Depending on the changes you're going to make (e.g. adding more users of
> the same functionality, or making the duplicated code much larger), this
> can be a good justification even if the code size increases.
> 
> I'd suggest to add the explanation (like 'This is in preparation for
> ...') to the commit message then.
> 
> Kevin
> 

I think, I'll resend the whole series with this preparation next week.

-- 
Best regards,
Vladimir