[Qemu-devel] [for-2.9 PATCH 0/3] 9pfs: fix 9p session reset

[Qemu-devel] [for-2.9 PATCH 0/3] 9pfs: fix 9p session reset

[Greg Kurz]

When resetting a 9p session we leak the migration blocker: this makes the
device unmigratable until the guest remounts/unmounts the 9p share again.
We also leak in-flight I/O whose completion will occur in the context of
the new session. This violates the 9p specification [*] and is likely to
confuse clients.

This series fixes both issues.

---

Greg Kurz (3):
      9pfs: clear migration blocker at session reset
      9pfs: cancel active PDUs in virtfs_reset()
      9pfs: drop useless loop in v9fs_reset()


 hw/9pfs/9p.c |   38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

--
Greg

[Qemu-devel] [for-2.9 PATCH 1/3] 9pfs: clear migration blocker at session reset

[Greg Kurz]

The migration blocker survives a device 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 migration blocker is supposed to be cleared when
we put the last reference on the root fid, but virtfs_reset() wrongly calls
free_fid() instead of put_fid().

This patch fixes virtfs_reset() so that it honor the way fids are supposed
to be manipulated: first get a reference and later put it back when you're
done.

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 48babce836b6..cc109367b030 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -539,14 +539,15 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
 
     /* Free all fids */
     while (s->fid_list) {
+        /* Get fid */
         fidp = s->fid_list;
+        fidp->ref++;
+
+        /* Clunk fid */
         s->fid_list = fidp->next;
+        fidp->clunked = 1;
 
-        if (fidp->ref) {
-            fidp->clunked = 1;
-        } else {
-            free_fid(pdu, fidp);
-        }
+        put_fid(pdu, fidp);
     }
 }
 

Re: [Qemu-devel] [for-2.9 PATCH 1/3] 9pfs: clear migration blocker at session reset

[李强]

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Friday, March 31, 2017 7:27 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 1/3] 9pfs: clear migration blocker at session reset
> 
> The migration blocker survives a device 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 migration blocker is supposed to be cleared when we
> put the last reference on the root fid, but virtfs_reset() wrongly calls
> free_fid() instead of put_fid().
> 
> This patch fixes virtfs_reset() so that it honor the way fids are supposed to be
> manipulated: first get a reference and later put it back when you're done.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---

Reviewed-by: Li Qiang <liqiang6-s@360.cn>

>  hw/9pfs/9p.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 48babce836b6..cc109367b030
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -539,14 +539,15 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> 
>      /* Free all fids */
>      while (s->fid_list) {
> +        /* Get fid */
>          fidp = s->fid_list;
> +        fidp->ref++;
> +
> +        /* Clunk fid */
>          s->fid_list = fidp->next;
> +        fidp->clunked = 1;
> 
> -        if (fidp->ref) {
> -            fidp->clunked = 1;
> -        } else {
> -            free_fid(pdu, fidp);
> -        }
> +        put_fid(pdu, fidp);
>      }
>  }
> 

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

[Greg Kurz]

According to the 9P spec [1], the version operation should abort any
outstanding I/O and clunk all fids, so that a new session may be started
in a clean state.

The current code tries to clunk and free fids, but it doesn't wait for
active PDUs to complete. This can cause an I/O to actually complete after
the new session has begun, and confuse the client.

This patch modifies virtfs_reset() so that it explicitely cancels and waits
for inflight requests to terminate. All fids should thus be unreferenced
and ready to be freed. Let's make it clear with a an assertion.

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

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

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index cc109367b030..86ed9065c4e2 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -536,9 +536,29 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
 {
     V9fsState *s = pdu->s;
     V9fsFidState *fidp;
+    bool done = false;
+
+    /* Drain any outstanding I/O */
+    while (!done) {
+        V9fsPDU *cancel_pdu;
+
+        done = true;
+        QLIST_FOREACH(cancel_pdu, &s->active_list, next) {
+            if (cancel_pdu != pdu) {
+                done = false;
+                cancel_pdu->cancelled = 1;
+                qemu_co_queue_wait(&cancel_pdu->complete, NULL);
+                cancel_pdu->cancelled = 0;
+                pdu_free(cancel_pdu);
+                break;
+            }
+        }
+    }
 
     /* Free all fids */
     while (s->fid_list) {
+        assert(!fidp->ref);
+
         /* Get fid */
         fidp = s->fid_list;
         fidp->ref++;
@@ -670,7 +690,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);
     }

Re: [Qemu-devel] [for-2.9 PATCH 2/3] 9pfs: cancel active PDUs in virtfs_reset()

[李强]

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Friday, March 31, 2017 7:27 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 2/3] 9pfs: cancel active PDUs in virtfs_reset()
> 
> According to the 9P spec [1], the version operation should abort any
> outstanding I/O and clunk all fids, so that a new session may be started in a
> clean state.
> 
> The current code tries to clunk and free fids, but it doesn't wait for active PDUs
> to complete. This can cause an I/O to actually complete after the new session
> has begun, and confuse the client.
> 
> This patch modifies virtfs_reset() so that it explicitely cancels and waits for
> inflight requests to terminate. All fids should thus be unreferenced and ready to
> be freed. Let's make it clear with a an assertion.
> 
> [1] http://man.cat-v.org/plan_9/5/version
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---

Reviewed-by: Li Qiang <liqiang6-s@360.cn>

>  hw/9pfs/9p.c |   22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index cc109367b030..86ed9065c4e2
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -536,9 +536,29 @@ static void coroutine_fn virtfs_reset(V9fsPDU *pdu)
> {
>      V9fsState *s = pdu->s;
>      V9fsFidState *fidp;
> +    bool done = false;
> +
> +    /* Drain any outstanding I/O */
> +    while (!done) {
> +        V9fsPDU *cancel_pdu;
> +
> +        done = true;
> +        QLIST_FOREACH(cancel_pdu, &s->active_list, next) {
> +            if (cancel_pdu != pdu) {
> +                done = false;
> +                cancel_pdu->cancelled = 1;
> +                qemu_co_queue_wait(&cancel_pdu->complete, NULL);
> +                cancel_pdu->cancelled = 0;
> +                pdu_free(cancel_pdu);
> +                break;
> +            }
> +        }
> +    }
> 
>      /* Free all fids */
>      while (s->fid_list) {
> +        assert(!fidp->ref);
> +
>          /* Get fid */
>          fidp = s->fid_list;
>          fidp->ref++;
> @@ -670,7 +690,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);
>      }

[Qemu-devel] [for-2.9 PATCH 3/3] 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 |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 86ed9065c4e2..16ef6bd5bd8c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3601,6 +3601,7 @@ static void coroutine_fn virtfs_co_reset(void *opaque)
     VirtfsCoResetData *data = opaque;
 
     virtfs_reset(&data->pdu);
+    assert(QLIST_EMPTY(&data->pdu.s->active_list));
     data->done = true;
 }
 
@@ -3609,10 +3610,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);
 

Re: [Qemu-devel] [for-2.9 PATCH 3/3] 9pfs: drop useless loop in v9fs_reset()

[李强]

> -----Original Message-----
> From: Greg Kurz [mailto:groug@kaod.org]
> Sent: Friday, March 31, 2017 7:28 PM
> To: qemu-devel@nongnu.org
> Cc: Eric Blake; 李强; Greg Kurz
> Subject: [for-2.9 PATCH 3/3] 9pfs: drop useless loop in v9fs_reset()
> 
> 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>
> ---

Reviewed-by: Li Qiang <liqiang6-s@360.cn>

>  hw/9pfs/9p.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 86ed9065c4e2..16ef6bd5bd8c
> 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3601,6 +3601,7 @@ static void coroutine_fn virtfs_co_reset(void
> *opaque)
>      VirtfsCoResetData *data = opaque;
> 
>      virtfs_reset(&data->pdu);
> +    assert(QLIST_EMPTY(&data->pdu.s->active_list));
>      data->done = true;
>  }
> 
> @@ -3609,10 +3610,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);
>