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

[Daeho Jeong]

On Thu, Aug 27, 2026 at 7:56 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> On 8/28/26 01:00, Daeho Jeong wrote:
> > On Wed, Aug 26, 2026 at 6:19 PM Chao Yu <chao@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >>>> ---
> >>>>  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?

Hi Chao,

Thanks for the detailed explanation. You are completely right.

Given how tricky shrinker vs truncation lifecycle races can be,
keeping the current safe locking as a solid baseline and refactoring
it in a follow-up patch after merging makes total sense to me.

Let's go with your approach!

Thanks,

>
> 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@xxxxxxxxxxxxxxxxxxxxx
> >>>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> >>
>