On Sat, 08 Feb 2025, Chuck Lever wrote:
> On 2/7/25 12:15 AM, NeilBrown wrote:
> > list_lru_walk() is only useful when the aim is to remove all elements
> > from the list_lru.
>
> I think I get where this is going. Can the description cite some API
> documentation that comports with this claim?
That would require someone to write some documentation, because there
isn't any - not for list_lru_walk(). I wouldn't recommend writing any
either. It should be replaced with something with a name like
"list_lru_destroy()" which is given a list_lru_ and a callback (no
count). The callback can skip or isolate or if the disposal list gets
lock, drop the lock, dispose the lists, and return LRU_RETRY.
>
>
> > It will repeated visit rotated element of the first
> > per-node sublist before proceeding to subsrequent sublists.
>
> s/repeated/repeatedly, s/element/elements, and s/subsrequent/subsequent.
>
Yep.
>
> > This patch changes to use list_lru_walk_node() and list_lru_count_node()
> > on each individual node.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/nfsd/filecache.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 7dc20143c854..04588c03bdfe 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -532,10 +532,14 @@ static void
> > nfsd_file_gc(void)
> > {
> > LIST_HEAD(dispose);
> > - unsigned long ret;
> > + unsigned long ret = 0;
> > + int nid;
> >
> > - ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
> > - &dispose, list_lru_count(&nfsd_file_lru));
> > + for_each_node_state(nid, N_NORMAL_MEMORY) {
> > + unsigned long nr = list_lru_count_node(&nfsd_file_lru, nid);
> > + ret += list_lru_walk_node(&nfsd_file_lru, nid, nfsd_file_lru_cb,
> > + &dispose, &nr);
> > + }
> > trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
>
> Nit: Maybe this trace point should be placed in the loop and changed to
> record the nid as well?
Maybe.
>
>
> > nfsd_file_dispose_list_delayed(&dispose);
>
> Wondering if maybe the nfsd_file_dispose_list_delayed() call should also
> be moved inside the for loop to break up the disposal processing per
> node as well.
>
I don't think it would make much difference. The "disposal" here is
just moving the items onto different lists.
NeilBrown
>
> > }
>
>
> --
> Chuck Lever
>
