On Sat, 08 Feb 2025, Chuck Lever wrote:
> On 2/7/25 12:15 AM, NeilBrown wrote:
> > The filecache lru is walked in 2 circumstances for 2 different reasons.
> >
> > 1/ When called from the shrinker we want to discard the first few
> > entries on the list, ignoring any with NFSD_FILE_REFERENCED set
> > because they should really be at the end of the LRU as they have been
> > referenced recently. So those ones are ROTATED.
> >
> > 2/ When called from the nfsd_file_gc() timer function we want to discard
> > anything that hasn't been used since before the previous call, and
> > mark everything else as unused at this point in time.
> >
> > Using the same flag for both of these can result in some unexpected
> > outcomes. If the shrinker callback clears NFSD_FILE_REFERENCED then the
> > nfsd_file_gc() will think the file hasn't been used in a while, while
> > really it has.
> >
> > I think it is easier to reason about the behaviour if we instead have
> > two flags.
> >
> > NFSD_FILE_REFERENCED means "this should be at the end of the LRU, please
> > put it there when convenient"
> > NFSD_FILE_RECENT means "this has been used recently - since the last
> > run of nfsd_file_gc()
> >
> > When either caller finds an NFSD_FILE_REFERENCED entry, that entry
> > should be moved to the end of the LRU and the flag cleared. This can
> > safely happen at any time. The actual order on the lru might not be
> > strictly least-recently-used, but that is normal for linux lrus.
> >
> > The shrinker callback can ignore the "recent" flag. If it ends up
> > freeing something that is "recent" that simply means that memory
> > pressure is sufficient to limit the acceptable cache age to less than
> > the nfsd_file_gc frequency.
> >
> > The gc caller should primarily focus on NFSD_FILE_RECENT. It should
> > free everything that doesn't have this flag set, and should clear the
> > flag on everything else. When it clears the flag it is convenient to
> > clear the "REFERENCED" flag and move to the end of the LRU too.
> >
> > With this, calls from the shrinker do not prematurely age files. It
> > will focus only on freeing those that are least recently used.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > ---
> > fs/nfsd/filecache.c | 21 +++++++++++++++++++--
> > fs/nfsd/filecache.h | 1 +
> > fs/nfsd/trace.h | 3 +++
> > 3 files changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index 04588c03bdfe..9faf469354a5 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -318,10 +318,10 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
> > mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
> > }
> >
> > -
> > static bool nfsd_file_lru_add(struct nfsd_file *nf)
> > {
> > set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > + set_bit(NFSD_FILE_RECENT, &nf->nf_flags);
> > if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
> > trace_nfsd_file_lru_add(nf);
> > return true;
> > @@ -528,6 +528,23 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
> > return LRU_REMOVED;
> > }
> >
> > +static enum lru_status
> > +nfsd_file_gc_cb(struct list_head *item, struct list_lru_one *lru,
> > + void *arg)
> > +{
> > + struct nfsd_file *nf = list_entry(item, struct nfsd_file, nf_lru);
> > +
> > + if (test_and_clear_bit(NFSD_FILE_RECENT, &nf->nf_flags)) {
> > + /* "REFERENCED" really means "should be at the end of the LRU.
> > + * As we are putting it there we can clear the flag
> > + */
> > + clear_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > + trace_nfsd_file_gc_aged(nf);
> > + return LRU_ROTATE;
> > + }
> > + return nfsd_file_lru_cb(item, lru, arg);
> > +}
> > +
> > static void
> > nfsd_file_gc(void)
> > {
> > @@ -537,7 +554,7 @@ nfsd_file_gc(void)
> >
> > 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,
> > + ret += list_lru_walk_node(&nfsd_file_lru, nid, nfsd_file_gc_cb,
> > &dispose, &nr);
> > }
> > trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
> > diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> > index d5db6b34ba30..de5b8aa7fcb0 100644
> > --- a/fs/nfsd/filecache.h
> > +++ b/fs/nfsd/filecache.h
> > @@ -38,6 +38,7 @@ struct nfsd_file {
> > #define NFSD_FILE_PENDING (1)
> > #define NFSD_FILE_REFERENCED (2)
> > #define NFSD_FILE_GC (3)
> > +#define NFSD_FILE_RECENT (4)
> > unsigned long nf_flags;
> > refcount_t nf_ref;
> > unsigned char nf_may;
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index ad2c0c432d08..9af723eeb2b0 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -1039,6 +1039,7 @@ DEFINE_CLID_EVENT(confirmed_r);
> > { 1 << NFSD_FILE_HASHED, "HASHED" }, \
> > { 1 << NFSD_FILE_PENDING, "PENDING" }, \
> > { 1 << NFSD_FILE_REFERENCED, "REFERENCED" }, \
> > + { 1 << NFSD_FILE_RECENT, "RECENT" }, \
> > { 1 << NFSD_FILE_GC, "GC" })
> >
> > DECLARE_EVENT_CLASS(nfsd_file_class,
> > @@ -1317,6 +1318,7 @@ DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del_disposed);
> > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_in_use);
> > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_writeback);
> > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_referenced);
> > +DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_aged);
> > DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_disposed);
> >
> > DECLARE_EVENT_CLASS(nfsd_file_lruwalk_class,
> > @@ -1346,6 +1348,7 @@ DEFINE_EVENT(nfsd_file_lruwalk_class, name, \
> > TP_ARGS(removed, remaining))
> >
> > DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
> > +DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_recent);
> > DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
> >
> > TRACE_EVENT(nfsd_file_close,
>
> The other patches in this series look like solid improvements. This one
> could be as well, but it will take me some time to understand it.
>
> I am generally in favor of replacing the logic that removes and adds
> these items with a single atomic bitop, and I'm happy to see NFSD stick
> with the use of an existing LRU facility while documenting its unique
> requirements ("nfsd_file_gc_aged" and so on).
>
> I would still prefer the backport to be lighter -- looks like the key
> changes are 3/6 and 6/6. Is there any chance the series can be
> reorganized to facilitate backporting? I have to ask, and the answer
> might be "no", I realize.
I'm going with "no".
To be honest, I was hoping that the complexity displayed here needed
to work around the assumptions of list_lru what don't match our needs
would be sufficient to convince you that list_lru isn't worth pursuing.
I see that didn't work.
So I am no longer invested in this patch set. You are welcome to use it
if you wish and to make any changes that you think are suitable, but I
don't think it is a good direction to go and will not be offering any
more code changes to support the use of list_lru here.
Thanks,
NeilBrown
