[Qemu-devel] [PATCH v2 0/3] dataplane: misaligned buffers support for Windows guests

[Qemu-devel] [PATCH v2 0/3] dataplane: misaligned buffers support for Windows guests

[Stefan Hajnoczi]

Windows guests do not work with x-data-plane=on because misaligned request
support is missing in hw/dataplane/virtio-blk.c.  This series adds a bounce
buffer when the request is misaligned.  Windows guests now work with
x-data-plane=on.

v2:
 * Use qemu_iovec_from_buffer() on read completion, not qemu_iovec_to_buffer() [Stefan]

Stefan Hajnoczi (3):
  block: make qiov_is_aligned() public
  dataplane: extract virtio-blk read/write processing into do_rdwr_cmd()  dataplane: handle misaligned virtio-blk requests

 block.c                   | 16 +++++++++++
 block/raw-posix.c         | 18 +------------
 hw/dataplane/virtio-blk.c | 67 +++++++++++++++++++++++++++++++++++++++--------
 include/block/block.h     |  1 +
 4 files changed, 74 insertions(+), 28 deletions(-)

-- 
1.8.0.2

[Qemu-devel] [PATCH v2 1/3] block: make qiov_is_aligned() public

[Stefan Hajnoczi]

The qiov_is_aligned() function checks whether a QEMUIOVector meets a
BlockDriverState's alignment requirements.  This is needed by
virtio-blk-data-plane so:

1. Move the function from block/raw-posix.c to block/block.c.
2. Make it public in block/block.h.
3. Rename to bdrv_qiov_is_aligned().
4. Change return type from int to bool.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c               | 16 ++++++++++++++++
 block/raw-posix.c     | 18 +-----------------
 include/block/block.h |  1 +
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 4e28c55..11ca661 100644
--- a/block.c
+++ b/block.c
@@ -4323,6 +4323,22 @@ void *qemu_blockalign(BlockDriverState *bs, size_t size)
     return qemu_memalign((bs && bs->buffer_alignment) ? bs->buffer_alignment : 512, size);
 }
 
+/*
+ * Check if all memory in this vector is sector aligned.
+ */
+bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
+{
+    int i;
+
+    for (i = 0; i < qiov->niov; i++) {
+        if ((uintptr_t) qiov->iov[i].iov_base % bs->buffer_alignment) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 void bdrv_set_dirty_tracking(BlockDriverState *bs, int enable)
 {
     int64_t bitmap_size;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 87d888e..02d2a4e 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -430,22 +430,6 @@ static void raw_reopen_abort(BDRVReopenState *state)
 #endif
 */
 
-/*
- * Check if all memory in this vector is sector aligned.
- */
-static int qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov)
-{
-    int i;
-
-    for (i = 0; i < qiov->niov; i++) {
-        if ((uintptr_t) qiov->iov[i].iov_base % bs->buffer_alignment) {
-            return 0;
-        }
-    }
-
-    return 1;
-}
-
 static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb)
 {
     int ret;
@@ -722,7 +706,7 @@ static BlockDriverAIOCB *raw_aio_submit(BlockDriverState *bs,
      * driver that it needs to copy the buffer.
      */
     if ((bs->open_flags & BDRV_O_NOCACHE)) {
-        if (!qiov_is_aligned(bs, qiov)) {
+        if (!bdrv_qiov_is_aligned(bs, qiov)) {
             type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
         } else if (s->use_aio) {
diff --git a/include/block/block.h b/include/block/block.h
index 0719339..ffd1936 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -349,6 +349,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
 
 void bdrv_set_buffer_alignment(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
+bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
 
 #define BDRV_SECTORS_PER_DIRTY_CHUNK 2048
 
-- 
1.8.0.2

[Qemu-devel] [PATCH v2 2/3] dataplane: extract virtio-blk read/write processing into do_rdwr_cmd()

[Stefan Hajnoczi]

Extract code for read/write command processing into do_rdwr_cmd().  This
brings together pieces that are spread across process_request().

The real motivation is to set the stage for handling misaligned
requests, which the next patch tackles.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/dataplane/virtio-blk.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/hw/dataplane/virtio-blk.c b/hw/dataplane/virtio-blk.c
index 4c4ad84..a6696b8 100644
--- a/hw/dataplane/virtio-blk.c
+++ b/hw/dataplane/virtio-blk.c
@@ -130,6 +130,22 @@ static void do_get_id_cmd(VirtIOBlockDataPlane *s,
     complete_request_early(s, head, inhdr, VIRTIO_BLK_S_OK);
 }
 
+static int do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
+                       struct iovec *iov, unsigned int iov_cnt,
+                       long long offset, unsigned int head,
+                       QEMUIOVector *inhdr)
+{
+    struct iocb *iocb;
+
+    iocb = ioq_rdwr(&s->ioqueue, read, iov, iov_cnt, offset);
+
+    /* Fill in virtio block metadata needed for completion */
+    VirtIOBlockRequest *req = container_of(iocb, VirtIOBlockRequest, iocb);
+    req->head = head;
+    req->inhdr = inhdr;
+    return 0;
+}
+
 static int process_request(IOQueue *ioq, struct iovec iov[],
                            unsigned int out_num, unsigned int in_num,
                            unsigned int head)
@@ -139,7 +155,6 @@ static int process_request(IOQueue *ioq, struct iovec iov[],
     struct virtio_blk_outhdr outhdr;
     QEMUIOVector *inhdr;
     size_t in_size;
-    struct iocb *iocb;
 
     /* Copy in outhdr */
     if (unlikely(iov_to_buf(iov, out_num, 0, &outhdr,
@@ -167,12 +182,12 @@ static int process_request(IOQueue *ioq, struct iovec iov[],
 
     switch (outhdr.type) {
     case VIRTIO_BLK_T_IN:
-        iocb = ioq_rdwr(ioq, true, in_iov, in_num, outhdr.sector * 512);
-        break;
+        do_rdwr_cmd(s, true, in_iov, in_num, outhdr.sector * 512, head, inhdr);
+        return 0;
 
     case VIRTIO_BLK_T_OUT:
-        iocb = ioq_rdwr(ioq, false, iov, out_num, outhdr.sector * 512);
-        break;
+        do_rdwr_cmd(s, false, iov, out_num, outhdr.sector * 512, head, inhdr);
+        return 0;
 
     case VIRTIO_BLK_T_SCSI_CMD:
         /* TODO support SCSI commands */
@@ -198,12 +213,6 @@ static int process_request(IOQueue *ioq, struct iovec iov[],
         g_slice_free(QEMUIOVector, inhdr);
         return -EFAULT;
     }
-
-    /* Fill in virtio block metadata needed for completion */
-    VirtIOBlockRequest *req = container_of(iocb, VirtIOBlockRequest, iocb);
-    req->head = head;
-    req->inhdr = inhdr;
-    return 0;
 }
 
 static void handle_notify(EventHandler *handler)
-- 
1.8.0.2

[Qemu-devel] [PATCH v2 3/3] dataplane: handle misaligned virtio-blk requests

[Stefan Hajnoczi]

O_DIRECT on Linux has alignment requirements on I/O buffers and
misaligned requests result in -EINVAL.  The Linux virtio_blk guest
driver usually submits aligned requests so I forgot to handle misaligned
requests.

It turns out that virtio-win guest drivers submit misaligned requests.
Handle them using a bounce buffer that meets alignment requirements.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/dataplane/virtio-blk.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/hw/dataplane/virtio-blk.c b/hw/dataplane/virtio-blk.c
index a6696b8..88300a6 100644
--- a/hw/dataplane/virtio-blk.c
+++ b/hw/dataplane/virtio-blk.c
@@ -34,6 +34,8 @@ typedef struct {
     struct iocb iocb;               /* Linux AIO control block */
     QEMUIOVector *inhdr;            /* iovecs for virtio_blk_inhdr */
     unsigned int head;              /* vring descriptor index */
+    void *bounce_buffer;            /* used if guest buffers are unaligned */
+    QEMUIOVector *read_qiov;        /* for read completion /w bounce buffer */
 } VirtIOBlockRequest;
 
 struct VirtIOBlockDataPlane {
@@ -89,6 +91,15 @@ static void complete_request(struct iocb *iocb, ssize_t ret, void *opaque)
 
     trace_virtio_blk_data_plane_complete_request(s, req->head, ret);
 
+    if (req->read_qiov) {
+        assert(req->bounce_buffer);
+        qemu_iovec_from_buf(req->read_qiov, 0, req->bounce_buffer, len);
+        qemu_iovec_destroy(req->read_qiov);
+        g_slice_free(QEMUIOVector, req->read_qiov);
+    }
+
+    qemu_vfree(req->bounce_buffer);
+
     qemu_iovec_from_buf(req->inhdr, 0, &hdr, sizeof(hdr));
     qemu_iovec_destroy(req->inhdr);
     g_slice_free(QEMUIOVector, req->inhdr);
@@ -136,6 +147,29 @@ static int do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
                        QEMUIOVector *inhdr)
 {
     struct iocb *iocb;
+    QEMUIOVector qiov;
+    struct iovec bounce_iov;
+    void *bounce_buffer = NULL;
+    QEMUIOVector *read_qiov = NULL;
+
+    qemu_iovec_init_external(&qiov, iov, iov_cnt);
+    if (!bdrv_qiov_is_aligned(s->blk->conf.bs, &qiov)) {
+        /* Redirect I/O to aligned bounce buffer */
+        bounce_buffer = qemu_blockalign(s->blk->conf.bs, qiov.size);
+        bounce_iov.iov_base = bounce_buffer;
+        bounce_iov.iov_len = qiov.size;
+        iov = &bounce_iov;
+        iov_cnt = 1;
+
+        if (read) {
+            /* Need to copy back from bounce buffer on completion */
+            read_qiov = g_slice_new(QEMUIOVector);
+            qemu_iovec_init(read_qiov, iov_cnt);
+            qemu_iovec_concat_iov(read_qiov, iov, iov_cnt, 0, qiov.size);
+        } else {
+            qemu_iovec_to_buf(&qiov, 0, bounce_buffer, qiov.size);
+        }
+    }
 
     iocb = ioq_rdwr(&s->ioqueue, read, iov, iov_cnt, offset);
 
@@ -143,6 +177,8 @@ static int do_rdwr_cmd(VirtIOBlockDataPlane *s, bool read,
     VirtIOBlockRequest *req = container_of(iocb, VirtIOBlockRequest, iocb);
     req->head = head;
     req->inhdr = inhdr;
+    req->bounce_buffer = bounce_buffer;
+    req->read_qiov = read_qiov;
     return 0;
 }
 
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH v2 3/3] dataplane: handle misaligned virtio-blk requests

[Paolo Bonzini]

Il 11/01/2013 13:34, Stefan Hajnoczi ha scritto:
> +        iov = &bounce_iov;
> +        iov_cnt = 1;
> +
> +        if (read) {
> +            /* Need to copy back from bounce buffer on completion */
> +            read_qiov = g_slice_new(QEMUIOVector);
> +            qemu_iovec_init(read_qiov, iov_cnt);
> +            qemu_iovec_concat_iov(read_qiov, iov, iov_cnt, 0, qiov.size);

This is still wrong, how did you test it?

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] dataplane: handle misaligned virtio-blk requests

[Stefan Hajnoczi]

On Fri, Jan 11, 2013 at 02:28:17PM +0100, Paolo Bonzini wrote:
> Il 11/01/2013 13:34, Stefan Hajnoczi ha scritto:
> > +        iov = &bounce_iov;
> > +        iov_cnt = 1;
> > +
> > +        if (read) {
> > +            /* Need to copy back from bounce buffer on completion */
> > +            read_qiov = g_slice_new(QEMUIOVector);
> > +            qemu_iovec_init(read_qiov, iov_cnt);
> > +            qemu_iovec_concat_iov(read_qiov, iov, iov_cnt, 0, qiov.size);
> 
> This is still wrong, how did you test it?

I sent it before reading the reviews from you and Kevin.  Bizarrely
Windows saw the files I copied to the disk - even across reboot (no
buffer cache involved?).  Or is Windows 8 doing tricks like "shutdown"
is actually "suspend to disk"?

Stefan

Re: [Qemu-devel] [PATCH v2 3/3] dataplane: handle misaligned virtio-blk requests

[Kevin Wolf]

Am 11.01.2013 15:40, schrieb Stefan Hajnoczi:
> On Fri, Jan 11, 2013 at 02:28:17PM +0100, Paolo Bonzini wrote:
>> Il 11/01/2013 13:34, Stefan Hajnoczi ha scritto:
>>> +        iov = &bounce_iov;
>>> +        iov_cnt = 1;
>>> +
>>> +        if (read) {
>>> +            /* Need to copy back from bounce buffer on completion */
>>> +            read_qiov = g_slice_new(QEMUIOVector);
>>> +            qemu_iovec_init(read_qiov, iov_cnt);
>>> +            qemu_iovec_concat_iov(read_qiov, iov, iov_cnt, 0, qiov.size);
>>
>> This is still wrong, how did you test it?
> 
> I sent it before reading the reviews from you and Kevin.  Bizarrely
> Windows saw the files I copied to the disk - even across reboot (no
> buffer cache involved?).  Or is Windows 8 doing tricks like "shutdown"
> is actually "suspend to disk"?

As far as I know this is exactly what it's doing. I've seen warnings
that you shouldn't mount file systems of a dual-boot Windows 8 if you
didn't change its options so that it really shuts down.

Kevin