Hi Christian !
On Tue, 4 Mar 2025 16:15:57 +0100
Christian Schoenebeck <[email protected]> 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 <[email protected]>
> ---
Another long long standing bug bites the dust ! Good catch !
Reviewed-by: Greg Kurz <[email protected]>
> 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
