Re: [f2fs-dev] [PATCH v3 03/12] f2fs: cache: introduce shrinker

[Chao Yu <chao@kernel.org>]

On 8/28/26 01:00, Daeho Jeong wrote:
> On Wed, Aug 26, 2026 at 6:19 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 8/27/26 03:19, Daeho Jeong wrote:
>>> On Tue, Aug 25, 2026 at 6:03 AM Chao Yu via Linux-f2fs-devel
>>> <linux-f2fs-devel@lists.sourceforge.net> wrote:
>>>>
>>>> This patch integrates the metadata cache into the F2FS memory shrinker
>>>> subsystem to reclaim clean, unreferenced cached blocks under memory
>>>> pressure.
>>>>
>>>> It implements f2fs_shrink_cache() using a 3-phase cache reclamin method:
>>>> 1. isolate clean entries from lru list
>>>> 2. truncate from radix tree under lock
>>>> 3. splice un-reclaimed entries back
>>>>
>>>> And hooks the new interface into f2fs_shrink_count() and f2fs_shrink_scan().
>>>>
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>>  fs/f2fs/cache.c    | 81 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  fs/f2fs/cache.h    |  3 ++
>>>>  fs/f2fs/shrinker.c | 12 +++++++
>>>>  3 files changed, 96 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/cache.c b/fs/f2fs/cache.c
>>>> index 8f09492c402f..3cee33c69880 100644
>>>> --- a/fs/f2fs/cache.c
>>>> +++ b/fs/f2fs/cache.c
>>>> @@ -534,3 +534,84 @@ void f2fs_destroy_cache(struct f2fs_cached_block_list *cache)
>>>>         f2fs_put_cache(entry, true);
>>>>         goto next;
>>>>  }
>>>> +
>>>> +static unsigned long f2fs_do_shrink_cache(struct f2fs_cached_block_list *cache,
>>>> +                                               unsigned long nr_to_scan)
>>>> +{
>>>> +       struct f2fs_cached_block *entry, *next;
>>>> +       LIST_HEAD(dispose_list);
>>>> +       LIST_HEAD(keep_list);
>>>> +       unsigned long freed = 0;
>>>> +       unsigned long isolated = 0;
>>>> +
>>>> +       /* Phase 1: Isolate candidate entries from LRU list into dispose_list */
>>>> +       spin_lock(&cache->list_lock);
>>>> +       list_for_each_entry_safe(entry, next, &cache->lru_list, list) {
>>>> +               if (isolated++ >= nr_to_scan)
>>>
>>> scanned?
>>
>> Okay, will clean.
>>
>>>
>>>> +                       break;
>>>> +
>>>> +               if (f2fs_cache_test_dirty(entry) ||
>>>> +                   f2fs_cache_test_writeback(entry) ||
>>>> +                   f2fs_cache_test_locked(entry))
>>>> +                       continue;
>>>> +
>>>> +               if (f2fs_cache_refcount(entry) != 1)
>>>> +                       continue;
>>>> +
>>>> +               list_move_tail(&entry->list, &dispose_list);
>>>> +       }
>>>> +       spin_unlock(&cache->list_lock);
>>>> +
>>>> +       /* Phase 2: Process isolated candidates one by one */
>>>> +       while (1) {
>>>> +               spin_lock(&cache->list_lock);
>>>
>>> Why do we need this lock to protect local lists?
>>
>> I think we need to protect the entry from being relocated in f2fs_find_cache()?
> 
> Ah, I see your point. In the current implementation, concurrent
> f2fs_find_cache() can indeed touch entry->list while it is on
> dispose_list.
> 
> However, if we adopt the referenced bit approach we discussed in Patch 01:
> - f2fs_find_cache() will only set the referenced bit and will NOT call
> list_move_tail() at all.
> - As a result, entry->list will never be touched or relocated during
> lookup, and this list_lock in Phase 2 can be safely eliminated as
> well.
> 
> So moving to the referenced bit design neatly solves both problems at once.

Hmm, however, f2fs_truncate_cache() will race w/ shrinker,

- f2fs_truncate_cache				- f2fs_do_shrink_cache
 - f2fs_do_truncate_cache
  - list_del_init(&entry->list) w/ lock		 - list_move_tail w/o lock

So we can not simply drop the list_lock in phase 2.

Or we can relocate list_del_init(&entry->list) from f2fs_truncate_cache()
to f2fs_free_cache(), but it will cause the butterfly effect:

At that time entry->cache will be set to NULL in f2fs_truncate_cache() because
now we treat entry->cache == NULL as the entry was truncated (something like
folio->mapping = NULL), so it can not access entry->cache->list_lock before
deleting item from list, then we need to update the truncation definition from
entry->cache == NULL to another state e.g. F2FS_CACHE_TRUNCATED.

I suffer a lots of bugs caused by shrinker and truncation, needs to handle it
carefully here.

I think we can set it as a base and improve it once it get merged, how do you
think?

Thanks,

> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>> +               entry = list_first_entry_or_null(&dispose_list,
>>>> +                                               struct f2fs_cached_block, list);
>>>> +               if (!entry) {
>>>> +                       spin_unlock(&cache->list_lock);
>>>> +                       break;
>>>> +               }
>>>> +               f2fs_cache_get(entry);
>>>> +               list_move_tail(&entry->list, &keep_list);
>>>> +               spin_unlock(&cache->list_lock);
>>>> +
>>>> +               if (!f2fs_trylock_cache(entry)) {
>>>> +                       f2fs_put_cache(entry, false);
>>>> +                       continue;
>>>> +               }
>>>> +
>>>> +               /* the entry has been truncated */
>>>> +               if (!entry->cache) {
>>>> +                       f2fs_put_cache(entry, true);
>>>> +                       continue;
>>>> +               }
>>>> +               /*
>>>> +                * at least there are shrinker, radix tree and another user
>>>> +                * has referenced the entry.
>>>> +                */
>>>> +               if (f2fs_cache_refcount(entry) >= 3) {
>>>> +                       f2fs_put_cache(entry, true);
>>>> +                       continue;
>>>> +               }
>>>> +
>>>> +               f2fs_do_truncate_cache(entry, false);
>>>> +
>>>> +               if (f2fs_put_cache(entry, true))
>>>> +                       freed++;
>>>> +       }
>>>> +
>>>> +       /* Phase 3: Splice un-reclaimed entries back onto cache->lru_list */
>>>> +       if (!list_empty(&keep_list)) {
>>>> +               spin_lock(&cache->list_lock);
>>>> +               list_splice_tail(&keep_list, &cache->lru_list);
>>>> +               spin_unlock(&cache->list_lock);
>>>> +       }
>>>> +
>>>> +       return freed;
>>>> +}
>>>> +
>>>> +unsigned long f2fs_shrink_cache(struct f2fs_sb_info *sbi,
>>>> +                                       unsigned long nr_to_scan)
>>>> +{
>>>> +       return f2fs_do_shrink_cache(META_CACHE(sbi), nr_to_scan);
>>>> +}
>>>> diff --git a/fs/f2fs/cache.h b/fs/f2fs/cache.h
>>>> index 7ee98d276938..618b377590da 100644
>>>> --- a/fs/f2fs/cache.h
>>>> +++ b/fs/f2fs/cache.h
>>>> @@ -184,4 +184,7 @@ void f2fs_stop_cache_wb_thread(struct f2fs_sb_info *sbi);
>>>>  #define f2fs_truncate_meta_caches(sbi, start, len)     \
>>>>         f2fs_drop_cache_range(META_CACHE(sbi), start, len, true)
>>>>
>>>> +unsigned long f2fs_shrink_cache(struct f2fs_sb_info *sbi,
>>>> +                               unsigned long nr_to_scan);
>>>> +
>>>>  #endif /* _LINUX_F2FS_CACHE_H */
>>>> diff --git a/fs/f2fs/shrinker.c b/fs/f2fs/shrinker.c
>>>> index 4f6bf5926de4..1755c85849e4 100644
>>>> --- a/fs/f2fs/shrinker.c
>>>> +++ b/fs/f2fs/shrinker.c
>>>> @@ -37,6 +37,11 @@ static unsigned long __count_extent_cache(struct f2fs_sb_info *sbi,
>>>>                                 atomic_read(&eti->total_ext_node);
>>>>  }
>>>>
>>>> +static unsigned long __count_cache(struct f2fs_sb_info *sbi)
>>>> +{
>>>> +       return sbi->meta_blocks.num_entries;
>>>> +}
>>>> +
>>>>  unsigned long f2fs_shrink_count(struct shrinker *shrink,
>>>>                                 struct shrink_control *sc)
>>>>  {
>>>> @@ -68,6 +73,9 @@ unsigned long f2fs_shrink_count(struct shrinker *shrink,
>>>>                 /* count free nids cache entries */
>>>>                 count += __count_free_nids(sbi);
>>>>
>>>> +               /* count generic cache entries */
>>>> +               count += __count_cache(sbi);
>>>> +
>>>>                 spin_lock(&f2fs_list_lock);
>>>>                 p = p->next;
>>>>                 mutex_unlock(&sbi->umount_mutex);
>>>> @@ -120,6 +128,10 @@ unsigned long f2fs_shrink_scan(struct shrinker *shrink,
>>>>                 if (freed < nr)
>>>>                         freed += f2fs_try_to_free_nids(sbi, nr - freed);
>>>>
>>>> +               /* shrink generic cache entries */
>>>> +               if (freed < nr)
>>>> +                       freed += f2fs_shrink_cache(sbi, nr - freed);
>>>> +
>>>>                 spin_lock(&f2fs_list_lock);
>>>>                 p = p->next;
>>>>                 list_move_tail(&sbi->s_list, &f2fs_list);
>>>> --
>>>> 2.49.0
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> Linux-f2fs-devel mailing list
>>>> Linux-f2fs-devel@lists.sourceforge.net
>>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>>