Patchwork blkverify: Handle overlapping I/O vector buffers

login
register
mail settings
Submitter Stefan Hajnoczi
Date Sept. 20, 2010, 1:31 p.m.
Message ID <1284989493-13154-1-git-send-email-stefanha@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/65208/
State New
Headers show

Comments

Stefan Hajnoczi - Sept. 20, 2010, 1:31 p.m.
When blkverify clones an I/O vector in order to perform mirrored reads
and then compare their contents, it does not take into account the
layout of individual buffers.  It turns out this is important because
guests may issue requests with overlapping buffers and the results
Kevin Wolf - Sept. 21, 2010, 10:06 a.m.
Am 20.09.2010 15:31, schrieb Stefan Hajnoczi:
> When blkverify clones an I/O vector in order to perform mirrored reads
> and then compare their contents, it does not take into account the
> layout of individual buffers.  It turns out this is important because
> guests may issue requests with overlapping buffers and the results
> differ depending on how buffers are overlapped.
> 
> This patch introduces logic to honor overlap relationships when cloning
> I/O vectors.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Took me a while to review this. These buffer calculations always look so
harmless, but it's not trivial at all...

Anyway, looks good to me.

Kevin
Stefan Hajnoczi - Sept. 21, 2010, 10:43 a.m.
On Tue, Sep 21, 2010 at 11:06 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 20.09.2010 15:31, schrieb Stefan Hajnoczi:
>> When blkverify clones an I/O vector in order to perform mirrored reads
>> and then compare their contents, it does not take into account the
>> layout of individual buffers.  It turns out this is important because
>> guests may issue requests with overlapping buffers and the results
>> differ depending on how buffers are overlapped.
>>
>> This patch introduces logic to honor overlap relationships when cloning
>> I/O vectors.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>
> Took me a while to review this. These buffer calculations always look so
> harmless, but it's not trivial at all...

Thanks for the review!  I wasn't thrilled to add this logic either but
I don't see a way around it.

Will merge into blkverify so there is one unified patch including
fixes for your review comments.

Stefan

Patch

differ depending on how buffers are overlapped.

This patch introduces logic to honor overlap relationships when cloning
I/O vectors.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 block/blkverify.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/block/blkverify.c b/block/blkverify.c
index 97717a6..524b777 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -166,17 +166,75 @@  static ssize_t blkverify_iovec_compare(QEMUIOVector *a, QEMUIOVector *b)
     return -1;
 }
 
+typedef struct {
+    int src_index;
+    struct iovec *src_iov;
+    void *dest_base;
+} IOVectorSortElem;
+
+static int sortelem_cmp_src_base(const void *a, const void *b)
+{
+    const IOVectorSortElem *elem_a = a;
+    const IOVectorSortElem *elem_b = b;
+
+    /* Don't overflow */
+    if (elem_a->src_iov->iov_base < elem_b->src_iov->iov_base) {
+        return -1;
+    } else if (elem_a->src_iov->iov_base > elem_b->src_iov->iov_base) {
+        return 1;
+    } else {
+        return 0;
+    }
+}
+
+static int sortelem_cmp_src_index(const void *a, const void *b)
+{
+    const IOVectorSortElem *elem_a = a;
+    const IOVectorSortElem *elem_b = b;
+
+    return elem_a->src_index - elem_b->src_index;
+}
+
 /**
  * Copy contents of I/O vector
+ *
+ * The relative relationships of overlapping iovecs are preserved.  This is
+ * necessary to ensure identical semantics in the cloned I/O vector.
  */
 static void blkverify_iovec_clone(QEMUIOVector *dest, const QEMUIOVector *src,
                                   void *buf)
 {
+    IOVectorSortElem sortelems[src->niov];
+    void *last_end;
     int i;
 
+    /* Sort by source iovecs by base address */
+    for (i = 0; i < src->niov; i++) {
+        sortelems[i].src_index = i;
+        sortelems[i].src_iov = &src->iov[i];
+    }
+    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_base);
+
+    /* Allocate buffer space taking into account overlapping iovecs */
+    last_end = NULL;
+    for (i = 0; i < src->niov; i++) {
+        struct iovec *cur = sortelems[i].src_iov;
+        ptrdiff_t rewind = 0;
+
+        /* Detect overlap */
+        if (last_end && last_end > cur->iov_base) {
+            rewind = last_end - cur->iov_base;
+        }
+
+        sortelems[i].dest_base = buf - rewind;
+        buf += cur->iov_len - MIN(rewind, cur->iov_len);
+        last_end = MAX(cur->iov_base + cur->iov_len, last_end);
+    }
+
+    /* Sort by source iovec index and build destination iovec */
+    qsort(sortelems, src->niov, sizeof(sortelems[0]), sortelem_cmp_src_index);
     for (i = 0; i < src->niov; i++) {
-        qemu_iovec_add(dest, buf, src->iov[i].iov_len);
-        buf += src->iov[i].iov_len;
+        qemu_iovec_add(dest, sortelems[i].dest_base, src->iov[i].iov_len);
     }
 }