Re: [PATCH 4/6] nfsd: filecache: introduce NFSD_FILE_RECENT

[Chuck Lever <chuck.lever@oracle.com>]

On 2/9/25 6:23 PM, NeilBrown wrote:
> 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@suse.de>
>>> ---
>>>  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.

Fair enough.


> 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.

If I may observe, you haven't offered a compelling explanation of why an
imperfect fit between list_lru and the filecache adds more technical
debt than does the introduction of a bespoke LRU mechanism.

I'm open to that argument, but I need stronger rationale (or performance
data) to back it up. So far I can agree that the defect rate in this
area is somewhat abnormal, but that seems to be because we don't
understand how to use the list_lru API to its best advantage.


-- 
Chuck Lever