| Message ID | 3429da65ff753b47654b7ae26607417c571a7cb1.1741101468.git.qemu_oss@crudebyte.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | 9pfs: v9fs_reclaim_fd() fixes | expand |
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 */
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 */
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(+)