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

Commit Message

Christian Schoenebeck March 4, 2025, 3:16 p.m. UTC

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 <[e-mail]>
---
 hw/9pfs/9p.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Greg Kurz March 6, 2025, 9:07 a.m. UTC | #1

On Tue, 4 Mar 2025 16:16:05 +0100
Christian Schoenebeck <[e-mail]> 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 <[e-mail]>
> ---
>  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,

Christian Schoenebeck March 6, 2025, 3:11 p.m. UTC | #2

On Thursday, March 6, 2025 10:07:56 AM CET Greg Kurz wrote:
> On Tue, 4 Mar 2025 16:16:05 +0100
> Christian Schoenebeck <[e-mail]> 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 <[e-mail]>
> > ---
> >  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

Patch

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.