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

[Greg Kurz <groug@kaod.org>]

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