diff mbox

blkverify: Handle overlapping I/O vector buffers

Message ID 1284989493-13154-1-git-send-email-stefanha@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Hajnoczi Sept. 20, 2010, 1:31 p.m. UTC 
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

Comments

Kevin Wolf Sept. 21, 2010, 10:06 a.m. UTC | #1 
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. UTC | #2 
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
diff mbox

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