Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

Chuck Lever
2025-01-27T13:36:02Z

On 1/26/25 5:33 PM, NeilBrown wrote:
> On Fri, 24 Jan 2025, Chuck Lever wrote:
>> On 1/22/25 5:10 PM, NeilBrown wrote:
>>> On Thu, 23 Jan 2025, Chuck Lever wrote:
>>>> On 1/21/25 10:54 PM, NeilBrown wrote:
>>>>> The final freeing of nfsd files is done by per-net nfsd threads (which
>>>>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
>>>>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
>>>>>
>>>>> This is a step towards replacing the list_lru with simple lists which
>>>>> each share the per-net lock in nfsd_fcache_disposal and will require
>>>>> less list walking.
>>>>>
>>>>> As the net is always shutdown before there is any chance that the rest
>>>>> of the filecache is shut down we can removed the tests on
>>>>> NFSD_FILE_CACHE_UP.
>>>>>
>>>>> For the filecache stats file, which assumes a global lru, we keep a
>>>>> separate counter which includes all files in all netns lrus.
>>>>
>>>> Hey Neil -
>>>>
>>>> One of the issues with the current code is that the contention for
>>>> the LRU lock has been effectively buried. It would be nice to have a
>>>> way to expose that contention in the new code.
>>>>
>>>> Can this patch or this series add some lock class infrastructure to
>>>> help account for the time spent contending for these dynamically
>>>> allocated spin locks?
>>>
>>> Unfortunately I don't know anything about accounting for lock contention
>>> time.
>>>
>>> I know about lock classes for the purpose of lockdep checking but not
>>> how they relate to contention tracking.
>>> Could you give me some pointers?
>>
>> Sticking these locks into a class is all you need to do. When lockstat
>> is enabled, it automatically accumulates the statistics for all locks
>> in a class, and treats that as a single lock in /proc/lock_stat.
>>
>
> Ahh thanks. So I don't need to do anything as this lock has it's own
> spin_lock_init() so it already has it's own class.
> However.... the init call is :
>
> spin_lock_init(&l->lock);
>
> so that appear in /proc/lock_stat as
>
> &l->lock:
> or maybe
> &l->lock/1:
> or even
> &l->lock/2:

Well, that's the problem, as I see it. They might all appear separately,
but we want lock_stat to group them together so we can see the aggregate
wait time and contention metrics.

Have a look at svc_reclassify_socket().

> Maybe I should change the "l" to something more unique. "nfd" ??
> Or I could actually define a lock_class_key and call __spin_lock_init:
>
> __skin_lock_init(&l->lock, "nfsd_fcache_disposal->lock", &key,
> false);
>
> There is no precedent for that though.
>
> NeilBrown
>

--
Chuck Lever