[PATCH 0/2] 9pfs: v9fs_reclaim_fd() fixes

[PATCH 0/2] 9pfs: v9fs_reclaim_fd() fixes

[Christian Schoenebeck]

Two fixes for 9p server's v9fs_reclaim_fd() function:

* Patch 1 fixes a concurrency issue.

* Patch 2 optimizes overall latency. On a test machine with ~800,000 FIDs, this
  reduced execution duration of v9fs_reclaim_fd() from 30ms to 1ms.

Christian Schoenebeck (2):
  9pfs: fix concurrent v9fs_reclaim_fd() calls
  9pfs: reduce latency of v9fs_reclaim_fd()

 hw/9pfs/9p.c | 36 +++++++++++++++++++++++++++---------
 hw/9pfs/9p.h |  1 +
 2 files changed, 28 insertions(+), 9 deletions(-)

-- 
2.39.5

[PATCH 1/2] 9pfs: fix concurrent v9fs_reclaim_fd() calls

[Christian Schoenebeck]

Even though this function is serialized to be always called from main
thread, v9fs_reclaim_fd() is dispatching the coroutine to a worker thread
in between via its v9fs_co_*() calls, hence leading to the situation where
v9fs_reclaim_fd() is effectively executed multiple times simultaniously,
which renders its LRU algorithm useless and causes high latency.

Fix this by adding a simple boolean variable to ensure this function is
only called once at a time. No synchronization needed for this boolean
variable as this function is only entered and returned on main thread.

Fixes: 7a46274529c ('hw/9pfs: Add file descriptor reclaim support')
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 10 ++++++++++
 hw/9pfs/9p.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 7cad2bce62..4f9c2dde9c 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -435,6 +435,12 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
     GHashTableIter iter;
     gpointer fid;
 
+    /* prevent multiple coroutines running this function simultaniously */
+    if (s->reclaiming) {
+        return;
+    }
+    s->reclaiming = true;
+
     g_hash_table_iter_init(&iter, s->fids);
 
     QSLIST_HEAD(, V9fsFidState) reclaim_list =
@@ -510,6 +516,8 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
          */
         put_fid(pdu, f);
     }
+
+    s->reclaiming = false;
 }
 
 /*
@@ -4324,6 +4332,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
     s->ctx.fst = &fse->fst;
     fsdev_throttle_init(s->ctx.fst);
 
+    s->reclaiming = false;
+
     rc = 0;
 out:
     if (rc) {
diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
index 5e041e1f60..259ad32ed1 100644
--- a/hw/9pfs/9p.h
+++ b/hw/9pfs/9p.h
@@ -362,6 +362,7 @@ struct V9fsState {
     uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
     uint16_t qp_affix_next;
     uint64_t qp_fullpath_next;
+    bool reclaiming;
 };
 
 /* 9p2000.L open flags */
-- 
2.39.5

Re: [PATCH 1/2] 9pfs: fix concurrent v9fs_reclaim_fd() calls

[Greg Kurz]

Hi Christian !

On Tue, 4 Mar 2025 16:15:57 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> Even though this function is serialized to be always called from main
> thread, v9fs_reclaim_fd() is dispatching the coroutine to a worker thread
> in between via its v9fs_co_*() calls, hence leading to the situation where
> v9fs_reclaim_fd() is effectively executed multiple times simultaniously,
> which renders its LRU algorithm useless and causes high latency.
> 
> Fix this by adding a simple boolean variable to ensure this function is
> only called once at a time. No synchronization needed for this boolean
> variable as this function is only entered and returned on main thread.
> 
> Fixes: 7a46274529c ('hw/9pfs: Add file descriptor reclaim support')
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---

Another long long standing bug bites the dust ! Good catch !

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/9pfs/9p.c | 10 ++++++++++
>  hw/9pfs/9p.h |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 7cad2bce62..4f9c2dde9c 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -435,6 +435,12 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>      GHashTableIter iter;
>      gpointer fid;
>  
> +    /* prevent multiple coroutines running this function simultaniously */
> +    if (s->reclaiming) {
> +        return;
> +    }
> +    s->reclaiming = true;
> +
>      g_hash_table_iter_init(&iter, s->fids);
>  
>      QSLIST_HEAD(, V9fsFidState) reclaim_list =
> @@ -510,6 +516,8 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>           */
>          put_fid(pdu, f);
>      }
> +
> +    s->reclaiming = false;
>  }
>  
>  /*
> @@ -4324,6 +4332,8 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>      s->ctx.fst = &fse->fst;
>      fsdev_throttle_init(s->ctx.fst);
>  
> +    s->reclaiming = false;
> +
>      rc = 0;
>  out:
>      if (rc) {
> diff --git a/hw/9pfs/9p.h b/hw/9pfs/9p.h
> index 5e041e1f60..259ad32ed1 100644
> --- a/hw/9pfs/9p.h
> +++ b/hw/9pfs/9p.h
> @@ -362,6 +362,7 @@ struct V9fsState {
>      uint64_t qp_ndevices; /* Amount of entries in qpd_table. */
>      uint16_t qp_affix_next;
>      uint64_t qp_fullpath_next;
> +    bool reclaiming;
>  };
>  
>  /* 9p2000.L open flags */



-- 
Greg

[PATCH 2/2] 9pfs: reduce latency of v9fs_reclaim_fd()

[Christian Schoenebeck]

This function calls v9fs_co_close() and v9fs_co_closedir() in a loop. Each
one of the calls adds two thread hops (between main thread and a fs driver
background thread). Each thread hop adds latency, which sums up in
function's loop to a significant duration.

Reduce overall latency by open coding what v9fs_co_close() and
v9fs_co_closedir() do, executing those and the loop itself altogether in
only one background thread block, hence reducing the total amount of
thread hops to only two.

Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 hw/9pfs/9p.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 4f9c2dde9c..4b52b1a22a 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -434,6 +434,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
     V9fsFidState *f;
     GHashTableIter iter;
     gpointer fid;
+    int err;
 
     /* prevent multiple coroutines running this function simultaniously */
     if (s->reclaiming) {
@@ -446,10 +447,10 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
     QSLIST_HEAD(, V9fsFidState) reclaim_list =
         QSLIST_HEAD_INITIALIZER(reclaim_list);
 
+    /* Pick FIDs to be closed, collect them on reclaim_list. */
     while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &f)) {
         /*
-         * Unlink fids cannot be reclaimed. Check
-         * for them and skip them. Also skip fids
+         * Unlinked fids cannot be reclaimed, skip those, and also skip fids
          * currently being operated on.
          */
         if (f->ref || f->flags & FID_NON_RECLAIMABLE) {
@@ -499,17 +500,24 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
         }
     }
     /*
-     * Now close the fid in reclaim list. Free them if they
-     * are already clunked.
+     * Close the picked FIDs altogether on a background I/O driver thread. Do
+     * this all at once to keep latency (i.e. amount of thread hops between main
+     * thread <-> fs driver background thread) as low as possible.
      */
+    v9fs_co_run_in_worker({
+        QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
+            err = (f->fid_type == P9_FID_DIR) ?
+                s->ops->closedir(&s->ctx, &f->fs_reclaim) :
+                s->ops->close(&s->ctx, &f->fs_reclaim);
+            if (!err) {
+                total_open_fd--;
+            }
+        }
+    });
+    /* Free the closed FIDs. */
     while (!QSLIST_EMPTY(&reclaim_list)) {
         f = QSLIST_FIRST(&reclaim_list);
         QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
-        if (f->fid_type == P9_FID_FILE) {
-            v9fs_co_close(pdu, &f->fs_reclaim);
-        } else if (f->fid_type == P9_FID_DIR) {
-            v9fs_co_closedir(pdu, &f->fs_reclaim);
-        }
         /*
          * Now drop the fid reference, free it
          * if clunked.
-- 
2.39.5

Re: [PATCH 2/2] 9pfs: reduce latency of v9fs_reclaim_fd()

[Greg Kurz]

On Tue, 4 Mar 2025 16:16:05 +0100
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This function calls v9fs_co_close() and v9fs_co_closedir() in a loop. Each
> one of the calls adds two thread hops (between main thread and a fs driver
> background thread). Each thread hop adds latency, which sums up in
> function's loop to a significant duration.
> 
> Reduce overall latency by open coding what v9fs_co_close() and
> v9fs_co_closedir() do, executing those and the loop itself altogether in
> only one background thread block, hence reducing the total amount of
> thread hops to only two.
> 

I'm afraid it is a bit trickier but I might have found a subtle bug
in the current code :-)

> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  hw/9pfs/9p.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index 4f9c2dde9c..4b52b1a22a 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -434,6 +434,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>      V9fsFidState *f;
>      GHashTableIter iter;
>      gpointer fid;
> +    int err;
>  
>      /* prevent multiple coroutines running this function simultaniously */
>      if (s->reclaiming) {
> @@ -446,10 +447,10 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>      QSLIST_HEAD(, V9fsFidState) reclaim_list =
>          QSLIST_HEAD_INITIALIZER(reclaim_list);
>  
> +    /* Pick FIDs to be closed, collect them on reclaim_list. */
>      while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &f)) {
>          /*
> -         * Unlink fids cannot be reclaimed. Check
> -         * for them and skip them. Also skip fids
> +         * Unlinked fids cannot be reclaimed, skip those, and also skip fids
>           * currently being operated on.
>           */
>          if (f->ref || f->flags & FID_NON_RECLAIMABLE) {
> @@ -499,17 +500,24 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
>          }
>      }
>      /*
> -     * Now close the fid in reclaim list. Free them if they
> -     * are already clunked.
> +     * Close the picked FIDs altogether on a background I/O driver thread. Do
> +     * this all at once to keep latency (i.e. amount of thread hops between main
> +     * thread <-> fs driver background thread) as low as possible.
>       */
> +    v9fs_co_run_in_worker({
> +        QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
> +            err = (f->fid_type == P9_FID_DIR) ?
> +                s->ops->closedir(&s->ctx, &f->fs_reclaim) :
> +                s->ops->close(&s->ctx, &f->fs_reclaim);
> +            if (!err) {
> +                total_open_fd--;

Hmm... total_open_fd is supposed to be accessed by the main thread only.
Maybe turn total_open_fd into an atomic ? Or have this block to return
the count of closed fds and propagate that to total_open_fd in the main
thread ?

> +            }
> +        }
> +    });
> +    /* Free the closed FIDs. */
>      while (!QSLIST_EMPTY(&reclaim_list)) {
>          f = QSLIST_FIRST(&reclaim_list);
>          QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
> -        if (f->fid_type == P9_FID_FILE) {
> -            v9fs_co_close(pdu, &f->fs_reclaim);
> -        } else if (f->fid_type == P9_FID_DIR) {
> -            v9fs_co_closedir(pdu, &f->fs_reclaim);
> -        }

An now for the subtle bug in the existing code :

v9fs_co_close() and v9fs_co_closedir() honor TFLUSH with the following
check that skips the actual closing :

    if (v9fs_request_cancelled(pdu)) {
        return -EINTR;
    }

If the client cancels the request that needs fd reclaim, some
fds will be cleared from the fids without being closed. This
causes fd leaks and probably ruins the overall reclaim logic.

This is kinda fixed by the current patch since all the fds
in the reclaim list are closed altogether atomically in the
same v9fs_co_run_in_worker().

>          /*
>           * Now drop the fid reference, free it
>           * if clunked.

Cheers,

-- 
Greg

Re: [PATCH 2/2] 9pfs: reduce latency of v9fs_reclaim_fd()

[Christian Schoenebeck]

On Thursday, March 6, 2025 10:07:56 AM CET Greg Kurz wrote:
> On Tue, 4 Mar 2025 16:16:05 +0100
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > This function calls v9fs_co_close() and v9fs_co_closedir() in a loop. Each
> > one of the calls adds two thread hops (between main thread and a fs driver
> > background thread). Each thread hop adds latency, which sums up in
> > function's loop to a significant duration.
> > 
> > Reduce overall latency by open coding what v9fs_co_close() and
> > v9fs_co_closedir() do, executing those and the loop itself altogether in
> > only one background thread block, hence reducing the total amount of
> > thread hops to only two.
> > 
> 
> I'm afraid it is a bit trickier but I might have found a subtle bug
> in the current code :-)
> 
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> >  hw/9pfs/9p.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 4f9c2dde9c..4b52b1a22a 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -434,6 +434,7 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >      V9fsFidState *f;
> >      GHashTableIter iter;
> >      gpointer fid;
> > +    int err;
> >  
> >      /* prevent multiple coroutines running this function simultaniously */
> >      if (s->reclaiming) {
> > @@ -446,10 +447,10 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >      QSLIST_HEAD(, V9fsFidState) reclaim_list =
> >          QSLIST_HEAD_INITIALIZER(reclaim_list);
> >  
> > +    /* Pick FIDs to be closed, collect them on reclaim_list. */
> >      while (g_hash_table_iter_next(&iter, &fid, (gpointer *) &f)) {
> >          /*
> > -         * Unlink fids cannot be reclaimed. Check
> > -         * for them and skip them. Also skip fids
> > +         * Unlinked fids cannot be reclaimed, skip those, and also skip fids
> >           * currently being operated on.
> >           */
> >          if (f->ref || f->flags & FID_NON_RECLAIMABLE) {
> > @@ -499,17 +500,24 @@ void coroutine_fn v9fs_reclaim_fd(V9fsPDU *pdu)
> >          }
> >      }
> >      /*
> > -     * Now close the fid in reclaim list. Free them if they
> > -     * are already clunked.
> > +     * Close the picked FIDs altogether on a background I/O driver thread. Do
> > +     * this all at once to keep latency (i.e. amount of thread hops between main
> > +     * thread <-> fs driver background thread) as low as possible.
> >       */
> > +    v9fs_co_run_in_worker({
> > +        QSLIST_FOREACH(f, &reclaim_list, reclaim_next) {
> > +            err = (f->fid_type == P9_FID_DIR) ?
> > +                s->ops->closedir(&s->ctx, &f->fs_reclaim) :
> > +                s->ops->close(&s->ctx, &f->fs_reclaim);
> > +            if (!err) {
> > +                total_open_fd--;
> 
> Hmm... total_open_fd is supposed to be accessed by the main thread only.
> Maybe turn total_open_fd into an atomic ? Or have this block to return
> the count of closed fds and propagate that to total_open_fd in the main
> thread ?

Right! I'll change that to the latter, as it involves far less changes.

> > +            }
> > +        }
> > +    });
> > +    /* Free the closed FIDs. */
> >      while (!QSLIST_EMPTY(&reclaim_list)) {
> >          f = QSLIST_FIRST(&reclaim_list);
> >          QSLIST_REMOVE(&reclaim_list, f, V9fsFidState, reclaim_next);
> > -        if (f->fid_type == P9_FID_FILE) {
> > -            v9fs_co_close(pdu, &f->fs_reclaim);
> > -        } else if (f->fid_type == P9_FID_DIR) {
> > -            v9fs_co_closedir(pdu, &f->fs_reclaim);
> > -        }
> 
> An now for the subtle bug in the existing code :
> 
> v9fs_co_close() and v9fs_co_closedir() honor TFLUSH with the following
> check that skips the actual closing :
> 
>     if (v9fs_request_cancelled(pdu)) {
>         return -EINTR;
>     }
> 
> If the client cancels the request that needs fd reclaim, some
> fds will be cleared from the fids without being closed. This
> causes fd leaks and probably ruins the overall reclaim logic.
> 
> This is kinda fixed by the current patch since all the fds
> in the reclaim list are closed altogether atomically in the
> same v9fs_co_run_in_worker().

Hu, that makes this 2nd patch indeed more than just an optimization fix.

I'll send a v2 and CC qemu-stable on the 2nd patch as well.

Good catches, thanks Greg!

/Christian