[Qemu-devel] [PATCH for-2.8 0/5] 9pfs: fix session reset

[Qemu-devel] [PATCH for-2.8 0/5] 9pfs: fix session reset

[Greg Kurz]

This series addresses two issues we currently have:

1) the version operation only does a partial cleanup of a previously active
   session. It can leave unfinished PDUs and stale fids behind. This violates
   the 9p specification.

2) if a guest mounts a 9p share and is then resetted with system_reset, it
   remains unmigratable because the migration blocker isn't cleared.

The current implementation of the official 9p client in the linux kernel
cannot hit 1) because it only uses the version operation at mount time.
Anyway, this could speed-up reset since we wouldn't simply wait for pending
I/O to complete as we do know.

And it is very easy to hit 2).

Please review.

Stefan,

Both issues have always been around, but it is the first time we can fix
them since 2.8 introduces vdc->reset. Do you think this is ok for 2.8 or
would you prefer to postpone this to 2.9 ?

Cheers.

--
Greg

---

Greg Kurz (5):
      9pfs: add missing coroutine_fn annotations
      9pfs: cancel active PDUs in virtfs_reset()
      9pfs: always free fids in virtfs_reset()
      9pfs: drop useless loop in v9fs_reset()
      9pfs: clear migration blocker when resetting the device


 hw/9pfs/9p.c |   59 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 20 deletions(-)

[Qemu-devel] [PATCH for-2.8 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 aea7e9d39206..e4815a97922d 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;
@@ -2332,7 +2332,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;

[Qemu-devel] [PATCH for-2.8 2/5] 9pfs: cancel active PDUs in virtfs_reset()

[Greg Kurz]

According to the 9P spec [1], the version operation should abort any
outstanding I/O, so that a new session may be started in a clean state.
This also makes sense in case of reset: we don't want to keep stale state
around.

This patch modifies virtfs_reset() which is called in both cases, so that
it explicitely cancels and waits for inflight requests to terminate.

Of course this may have impact on the underlying filesystem, as some
operations may have partly modified its state. But it is out of the
scope of this patch to have each operation handle cancellation in a
cleaner way.

[1] http://man.cat-v.org/plan_9/5/version

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index e4815a97922d..3a48cdcdf975 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -532,10 +532,37 @@ static int coroutine_fn v9fs_mark_fids_unreclaim(V9fsPDU *pdu, V9fsPath *path)
     return 0;
 }
 
+static void coroutine_fn pdu_cancel(V9fsPDU *pdu)
+{
+    pdu->cancelled = 1;
+    /*
+     * Wait for pdu to complete.
+     */
+    qemu_co_queue_wait(&pdu->complete);
+    pdu->cancelled = 0;
+    if (!qemu_co_queue_next(&pdu->complete)) {
+        pdu_free(pdu);
+    }
+}
+
 static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
 {
     V9fsState *s = pdu->s;
     V9fsFidState *fidp;
+    bool done = false;
+
+    while (!done) {
+        V9fsPDU *cancel_pdu;
+
+        done = true;
+        QLIST_FOREACH(cancel_pdu, &s->active_list, next) {
+            if (cancel_pdu != pdu) {
+                done = false;
+                pdu_cancel(cancel_pdu);
+                break;
+            }
+        }
+    }
 
     /* Free all fids */
     while (s->fid_list) {
@@ -547,6 +574,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
         } else {
             free_fid(pdu, fidp);
         }
+        free_fid(pdu, fidp);
     }
 }
 
@@ -669,7 +697,7 @@ static void coroutine_fn pdu_complete(V9fsPDU *pdu, ssize_t len)
 
     pdu_push_and_notify(pdu);
 
-    /* Now wakeup anybody waiting in flush for this request */
+    /* Now wakeup anybody waiting in flush or reset for this request */
     if (!qemu_co_queue_next(&pdu->complete)) {
         pdu_free(pdu);
     }
@@ -2354,13 +2382,7 @@ static void coroutine_fn v9fs_flush(void *opaque)
         }
     }
     if (cancel_pdu) {
-        cancel_pdu->cancelled = 1;
-        /*
-         * Wait for pdu to complete.
-         */
-        qemu_co_queue_wait(&cancel_pdu->complete);
-        cancel_pdu->cancelled = 0;
-        pdu_free(cancel_pdu);
+        pdu_cancel(cancel_pdu);
     }
     pdu_complete(pdu, 7);
 }

[Qemu-devel] [PATCH for-2.8 3/5] 9pfs: always free fids in virtfs_reset()

[Greg Kurz]

All PDU that may hold a reference on a fid have been cancelled: we can
therefore free all the fids.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 3a48cdcdf975..75c6645de9ac 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -569,11 +569,7 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
         fidp = s->fid_list;
         s->fid_list = fidp->next;
 
-        if (fidp->ref) {
-            fidp->clunked = 1;
-        } else {
-            free_fid(pdu, fidp);
-        }
+        g_assert(!fidp->ref);
         free_fid(pdu, fidp);
     }
 }

[Qemu-devel] [PATCH for-2.8 4/5] 9pfs: drop useless loop in v9fs_reset()

[Greg Kurz]

We don't need to wait for the PDU active list to be empty: virtfs_reset()
already takes care of that.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 75c6645de9ac..6fea68866a5c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3560,8 +3560,11 @@ typedef struct VirtfsCoResetData {
 static void coroutine_fn virtfs_co_reset(void *opaque)
 {
     VirtfsCoResetData *data = opaque;
+    V9fsPDU *fake_pdu = &data->pdu;
+    V9fsState *s = fake_pdu->s;
 
-    virtfs_reset(&data->pdu);
+    virtfs_reset(fake_pdu);
+    g_assert(QLIST_EMPTY(&s->active_list));
     data->done = true;
 }
 
@@ -3570,10 +3573,6 @@ void v9fs_reset(V9fsState *s)
     VirtfsCoResetData data = { .pdu = { .s = s }, .done = false };
     Coroutine *co;
 
-    while (!QLIST_EMPTY(&s->active_list)) {
-        aio_poll(qemu_get_aio_context(), true);
-    }
-
     co = qemu_coroutine_create(virtfs_co_reset, &data);
     qemu_coroutine_enter(co);
 

[Qemu-devel] [PATCH for-2.8 5/5] 9pfs: clear migration blocker when resetting the device

[Greg Kurz]

The migration blocker survives a machine reset: if the guest mounts a
9p share and then gets rebooted with system_reset, it will be unmigratable
until it remounts and umounts the 9p share again.

This happens because the blocker gets freed in put_fid(), whereas
virtfs_reset() calls free_fid() directly.

The easiest fix is to have virtfs_reset() call put_fid() instead. We need
to mark the fid as clunked to be sure put_fid() actually calls free_fid().

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 6fea68866a5c..756d04f0a36a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -570,7 +570,9 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
         s->fid_list = fidp->next;
 
         g_assert(!fidp->ref);
-        free_fid(pdu, fidp);
+        fidp->clunked = 1;
+        fidp->ref++;
+        put_fid(pdu, fidp);
     }
 }