Re: [f2fs-dev] [PATCH v3 09/12] f2fs: cache: use compress cache

[Chao Yu]

On 8/28/26 01:20, Daeho Jeong wrote:
> On Wed, Aug 26, 2026 at 8:24 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>>
>> On 8/27/26 04:04, Daeho Jeong wrote:
>>>> -static void f2fs_truncate_cache(struct f2fs_cached_block *entry,
>>>> +void f2fs_truncate_cache(struct f2fs_cached_block *entry,
>>>>                                         bool drop_dirty)
>>>
>>> -> __f2fs_truncate_cache() without locking?
>> Or, I guess you mean we need a cleanup here?
>>
>> What about:
>>
>> void f2fs_truncate_cache(struct f2fs_cached_block *entry, bool drop_dirty)
>> {
>>         if (!entry->cache)
>>                 return;
>>         f2fs_do_truncate_cache(entry, drop_dirty);
>> }
>>
>> static void f2fs_truncate_cache_locked(struct f2fs_cached_block *entry, bool drop_dirty)
>> {
>>         f2fs_lock_cache(entry);
>>         f2fs_truncate_cache(entry, drop_dirty);
>>         f2fs_unlock_cache(entry);
>> }
>>
>> f2fs_truncate_cache() was exported for using in other .c file, in the meantime,
>> static f2fs_truncate_cache_locked() can be used inside cache.c.
> 
> Splitting them into two helpers (one with locking, one without) makes
> total sense.
> 
> However, in the Linux kernel convention, having `_locked` or `__`
> usually indicates that the caller is expected to ALREADY hold the lock
> (e.g. `__list_add()`, `__folio_mark_dirty()`).
> 
> If an exported function is named `f2fs_truncate_cache()` without `__`,
> callers would naturally assume it handles locking internally.
> 
> So I'd suggest following the standard kernel pattern:
> 
> /* Caller must hold f2fs_lock_cache(entry) */
> void __f2fs_truncate_cache(struct f2fs_cached_block *entry, bool drop_dirty)
> {
> if (!entry->cache)
> return;
> f2fs_do_truncate_cache(entry, drop_dirty);
> }
> 
> /* Safe wrapper that acquires entry lock internally */
> void f2fs_truncate_cache(struct f2fs_cached_block *entry, bool drop_dirty)
> {
> f2fs_lock_cache(entry);
> __f2fs_truncate_cache(entry, drop_dirty);
> f2fs_unlock_cache(entry);
> }
> 
> Then compress.c (which already holds the lock to check entry->ino) can
> call `__f2fs_truncate_cache()`, while other callers can safely use
> `f2fs_truncate_cache()`.

Okay, I see.

My concern is previously we don't export __f2fs functions in *.h, see f2fs.h,
but since it has "f2fs" keyword in prefix, so it won't pollute global name space,
so I guess it's fine, anyway, let me update.

Thanks,

> 
> What do you think?
> 
> Thanks,
> 
>>
>> Thanks,