[Qemu-devel] [PATCH] blkverify: Handle overlapping I/O vector buffers

[Qemu-devel] [PATCH] blkverify: Handle overlapping I/O vector buffers

[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>
---
 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);
     }
 }
 
-- 
1.7.1

[Qemu-devel] Re: [PATCH] blkverify: Handle overlapping I/O vector buffers

[Kevin Wolf]

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

Re: [Qemu-devel] Re: [PATCH] blkverify: Handle overlapping I/O vector buffers

[Stefan Hajnoczi]

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