[Qemu-devel] [PULL 0/5] 9p patches 20170125

[Qemu-devel] [PULL 0/5] 9p patches 20170125

[Greg Kurz]

The following changes since commit a9e404600a9bd1e6a26431fc89e5069092e67f14:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170124' into staging (2017-01-24 17:26:26 +0000)

are available in the git repository at:

  https://github.com/gkurz/qemu.git tags/for-upstream

for you to fetch changes up to fa0eb5c512d17a223d9f9bac45da48d78d12f584:

  9pfs: fix offset error in v9fs_xattr_read() (2017-01-25 09:34:35 +0100)

----------------------------------------------------------------
This pull request fixes a 2.9 regression and a long standing bug that can
cause 9p clients to hang. Other patches are minor enhancements.

----------------------------------------------------------------
Greg Kurz (5):
      9pfs: add missing coroutine_fn annotations
      tests: virtio-9p: improve error reporting
      9pfs: fix off-by-one error in PDU free list
      9pfs: local: trivial cosmetic fix in pwritev op
      9pfs: fix offset error in v9fs_xattr_read()

 hw/9pfs/9p-local.c     |  3 +--
 hw/9pfs/9p.c           | 12 ++++++------
 tests/virtio-9p-test.c | 24 +++++++++++++++++++-----
 3 files changed, 26 insertions(+), 13 deletions(-)
-- 
2.7.4

[Qemu-devel] [PULL 1/5] 9pfs: add missing coroutine_fn annotations

[Greg Kurz]

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index fa58877570f6..58310ca8d5a5 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1571,7 +1571,7 @@ out_nofid:
     v9fs_string_free(&name);
 }
 
-static void v9fs_fsync(void *opaque)
+static void coroutine_fn v9fs_fsync(void *opaque)
 {
     int err;
     int32_t fid;
@@ -2337,7 +2337,7 @@ out_nofid:
     v9fs_string_free(&symname);
 }
 
-static void v9fs_flush(void *opaque)
+static void coroutine_fn v9fs_flush(void *opaque)
 {
     ssize_t err;
     int16_t tag;
-- 
2.7.4

[Qemu-devel] [PULL 2/5] tests: virtio-9p: improve error reporting

[Greg Kurz]

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 tests/virtio-9p-test.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 060407b20e39..9556291567a4 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -236,6 +236,16 @@ static void v9fs_req_send(P9Req *req)
     req->t_off = 0;
 }
 
+static const char *rmessage_name(uint8_t id)
+{
+    return
+        id == P9_RLERROR ? "RLERROR" :
+        id == P9_RVERSION ? "RVERSION" :
+        id == P9_RATTACH ? "RATTACH" :
+        id == P9_RWALK ? "RWALK" :
+        "<unknown>";
+}
+
 static void v9fs_req_recv(P9Req *req, uint8_t id)
 {
     QVirtIO9P *v9p = req->v9p;
@@ -258,11 +268,15 @@ static void v9fs_req_recv(P9Req *req, uint8_t id)
     g_assert_cmpint(hdr.size, <=, P9_MAX_SIZE);
     g_assert_cmpint(hdr.tag, ==, req->tag);
 
-    if (hdr.id != id && hdr.id == P9_RLERROR) {
-        uint32_t err;
-        v9fs_uint32_read(req, &err);
-        g_printerr("Received Rlerror (%d) instead of Response %d\n", err, id);
-        g_assert_not_reached();
+    if (hdr.id != id) {
+        g_printerr("Received response %d (%s) instead of %d (%s)\n",
+                   hdr.id, rmessage_name(hdr.id), id, rmessage_name(id));
+
+        if (hdr.id == P9_RLERROR) {
+            uint32_t err;
+            v9fs_uint32_read(req, &err);
+            g_printerr("Rlerror has errno %d (%s)\n", err, strerror(err));
+        }
     }
     g_assert_cmpint(hdr.id, ==, id);
 }
-- 
2.7.4

[Qemu-devel] [PULL 3/5] 9pfs: fix off-by-one error in PDU free list

[Greg Kurz]

The server can handle MAX_REQ - 1 PDUs at a time and the virtio-9p
device has a MAX_REQ sized virtqueue. If the client manages to fill
up the virtqueue, pdu_alloc() will fail and the request won't be
processed without any notice to the client (it actually causes the
linux 9p client to hang).

This has been there since the beginning (commit 9f10751365b2 "virtio-9p:
Add a virtio 9p device to qemu"), but it needs an agressive workload to
run in the guest to show up.

We actually allocate MAX_REQ PDUs and I see no reason not to link them
all into the free list, so let's fix the init loop.

Reported-by: Tuomas Tynkkynen <tuomas@tuxera.com>
Suggested-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 58310ca8d5a5..d2d028828294 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3454,7 +3454,7 @@ int v9fs_device_realize_common(V9fsState *s, Error **errp)
     /* initialize pdu allocator */
     QLIST_INIT(&s->free_list);
     QLIST_INIT(&s->active_list);
-    for (i = 0; i < (MAX_REQ - 1); i++) {
+    for (i = 0; i < MAX_REQ; i++) {
         QLIST_INSERT_HEAD(&s->free_list, &s->pdus[i], next);
         s->pdus[i].s = s;
         s->pdus[i].idx = i;
-- 
2.7.4

[Qemu-devel] [PULL 4/5] 9pfs: local: trivial cosmetic fix in pwritev op

[Greg Kurz]

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p-local.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 845675e7a1bb..7de07e1ba67f 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -436,8 +436,7 @@ static ssize_t local_pwritev(FsContext *ctx, V9fsFidOpenState *fs,
                              const struct iovec *iov,
                              int iovcnt, off_t offset)
 {
-    ssize_t ret
-;
+    ssize_t ret;
 #ifdef CONFIG_PREADV
     ret = pwritev(fs->fd, iov, iovcnt, offset);
 #else
-- 
2.7.4

[Qemu-devel] [PULL 5/5] 9pfs: fix offset error in v9fs_xattr_read()

[Greg Kurz]

The current code tries to copy `read_count' bytes starting at offset
`offset' from a `read_count`-sized iovec. This causes v9fs_pack() to
fail with ENOBUFS.

Since the PDU iovec is already partially filled with `offset' bytes,
let's skip them when creating `qiov_full' and have v9fs_pack() to
copy the whole of it. Moreover, this is consistent with the other
places where v9fs_init_qiov_from_pdu() is called.

This fixes commit "bcb8998fac16 9pfs: call v9fs_init_qiov_from_pdu
before v9fs_pack".

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/9pfs/9p.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index d2d028828294..138d8e825d28 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -1655,7 +1655,7 @@ static void v9fs_init_qiov_from_pdu(QEMUIOVector *qiov, V9fsPDU *pdu,
     if (is_write) {
         pdu->s->transport->init_out_iov_from_pdu(pdu, &iov, &niov);
     } else {
-        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size);
+        pdu->s->transport->init_in_iov_from_pdu(pdu, &iov, &niov, size + skip);
     }
 
     qemu_iovec_init_external(&elem, iov, niov);
@@ -1685,8 +1685,8 @@ static int v9fs_xattr_read(V9fsState *s, V9fsPDU *pdu, V9fsFidState *fidp,
     }
     offset += err;
 
-    v9fs_init_qiov_from_pdu(&qiov_full, pdu, 0, read_count, false);
-    err = v9fs_pack(qiov_full.iov, qiov_full.niov, offset,
+    v9fs_init_qiov_from_pdu(&qiov_full, pdu, offset, read_count, false);
+    err = v9fs_pack(qiov_full.iov, qiov_full.niov, 0,
                     ((char *)fidp->fs.xattr.value) + off,
                     read_count);
     qemu_iovec_destroy(&qiov_full);
-- 
2.7.4

Re: [Qemu-devel] [PULL 0/5] 9p patches 20170125

[Peter Maydell]

On 25 January 2017 at 13:47, Greg Kurz <groug@kaod.org> wrote:
> The following changes since commit a9e404600a9bd1e6a26431fc89e5069092e67f14:
>
>   Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20170124' into staging (2017-01-24 17:26:26 +0000)
>
> are available in the git repository at:
>
>   https://github.com/gkurz/qemu.git tags/for-upstream
>
> for you to fetch changes up to fa0eb5c512d17a223d9f9bac45da48d78d12f584:
>
>   9pfs: fix offset error in v9fs_xattr_read() (2017-01-25 09:34:35 +0100)
>
> ----------------------------------------------------------------
> This pull request fixes a 2.9 regression and a long standing bug that can
> cause 9p clients to hang. Other patches are minor enhancements.
>
> ----------------------------------------------------------------
> Greg Kurz (5):
>       9pfs: add missing coroutine_fn annotations
>       tests: virtio-9p: improve error reporting
>       9pfs: fix off-by-one error in PDU free list
>       9pfs: local: trivial cosmetic fix in pwritev op
>       9pfs: fix offset error in v9fs_xattr_read()

Applied, thanks.

-- PMM