Message-ID: <8c13ca69-a08a-41c7-bbef-0a79a5f44f93@kernel.org>
Date: Fri, 9 Jan 2026 16:44:53 +0800
From: Chao Yu <chao@...nel.org>
To: Barry Song <21cnbao@...il.com>
Cc: chao@...nel.org, jaegeuk@...nel.org,
 linux-f2fs-devel@...ts.sourceforge.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] f2fs: fix to prevent clearing immutable for large folio
 supported inode

On 1/9/2026 11:57 AM, Barry Song wrote:
> On Fri, Jan 9, 2026 at 4:45 PM Chao Yu <chao@...nel.org> wrote:
>>
>> On 1/9/2026 11:05 AM, Barry Song wrote:
>>> On Fri, Jan 9, 2026 at 3:47 PM Chao Yu <chao@...nel.org> wrote:
>>>>
>>>> Below testcase can change large folio supported inode from immutable
>>>> to mutable, it's not as expected, let's add a new check condition in
>>>> f2fs_setflags_common() to detect and reject it.
>>>>
>>>> - dd if=/dev/zero of=/mnt/f2fs/test bs=32k count=4
>>>> - f2fs_io setflags immutable /mnt/f2fs/test
>>>> - echo 3 > /proc/sys/vm/drop_caches
>>>>    : to reload inode with large folio
>>>> - f2fs_io read 32 0 1 mmap 0 0 /mnt/f2fs/test
>>>> - f2fs_io clearflags immutable /mnt/f2fs/test
>>>>
>>>> Signed-off-by: Chao Yu <chao@...nel.org>
>>>> ---
>>>>    fs/f2fs/file.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>> index ce291f152bc3..f7f9da0b215f 100644
>>>> --- a/fs/f2fs/file.c
>>>> +++ b/fs/f2fs/file.c
>>>> @@ -2155,6 +2155,12 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
>>>>                   }
>>>>           }
>>>>
>>>> +       if ((iflags ^ masked_flags) & F2FS_IMMUTABLE_FL) {
>>>> +               if ((masked_flags & F2FS_IMMUTABLE_FL) &&
>>>> +                       mapping_large_folio_support(inode->i_mapping))
>>>> +                       return -EINVAL;
>>>
>>
>> Barry,
>>
>> I noticed that we are allowed to clear immutable if large folio are supported
>> in target inode, since we can prevent to open w/ write permission in ->open after
>> immutable is cleared, see details in f2fs doc below.
>>
>> So, anyway, I guess my patch should be ignored.
>>
>> Per-file Read-Only Large Folio Support
>> --------------------------------------
>>
>> F2FS implements large folio support on the read path to leverage high-order
>> page allocation for significant performance gains. To minimize code complexity,
>> this support is currently excluded from the write path, which requires handling
>> complex optimizations such as compression and block allocation modes.
>>
>> This optional feature is triggered only when a file's immutable bit is set.
>> Consequently, F2FS will return EOPNOTSUPP if a user attempts to open a cached
>> file with write permissions, even immediately after clearing the bit. Write
>> access is only restored once the cached inode is dropped. The usage flow is
>> demonstrated below:
>>
>> .. code-block::
>>
>>      # f2fs_io setflags immutable /data/testfile_read_seq
>>
>>      /* flush and reload the inode to enable the large folio */
>>      # sync && echo 3 > /proc/sys/vm/drop_caches
>>
>>      /* mmap(MAP_POPULATE) + mlock() */
>>      # f2fs_io read 128 0 1024 mmap 1 0 /data/testfile_read_seq
>>
>>      /* mmap() + fadvise(POSIX_FADV_WILLNEED) + mlock() */
>>      # f2fs_io read 128 0 1024 fadvise 1 0 /data/testfile_read_seq
>>
>>      /* mmap() + mlock2(MLOCK_ONFAULT) + madvise(MADV_POPULATE_READ) */
>>      # f2fs_io read 128 0 1024 madvise 1 0 /data/testfile_read_seq
>>
>>      # f2fs_io clearflags immutable /data/testfile_read_seq
>>
>>      # f2fs_io write 1 0 1 zero buffered /data/testfile_read_seq
>>      Failed to open /mnt/test/test: Operation not supported
>>
>>      /* flush and reload the inode to disable the large folio */
>>      # sync && echo 3 > /proc/sys/vm/drop_caches
> 
> Right, I see. The only concern is that this would require dropping
> the page cache for the entire system, rather than for a single inode.

Yes, maybe, one way to avoid "sync && echo 3" is introducing a new ioctl
interface to hold inode's parent lock, and call d_invalidate(inode_dentry)
to evict the inode which is not opened by anyone.

> 
> 
>>
>>      # f2fs_io write 1 0 1 zero buffered /data/testfile_read_seq
>>      Written 4096 bytes with pattern = zero, total_time = 29 us, max_latency = 28 us
>>
>>      # rm /data/testfile_read_seq
>>
>>> Hi Yu, I find it a bit odd to prevent unsetting immutable solely
>>> because large folios are in use. If unsetting immutable is considered
>>> unexpected behavior, it should be disallowed regardless of whether
>>> large folios are used, and apply equally in both cases.
>>
>> To confirm, you mean if clearing immutable is considered unexpected behavior,
>> we need to prevent clearing immutable for inode which doesn't enable large folio?
> 
> Right. It feels unfair to prevent clearing immutable solely because
> the file happens to have large folios.
> 
>>
>>>
>>> I'm not sure whether reverting the large folios setting is the
>>> better approach:
>>> truncate_pagecache(inode, inode->i_size);
>>> mapping_set_folio_order_range(inode->i_mapping, 0, 0);
>>
>> If we want to support reverting the large folios setting dynamically as you
>> proposed above, it need to consider more race case and corner case, so, a
>> little bit complicated.
> 
> Right. The idea is to truncate the page cache via
> truncate_pagecache(inode, 0) and set the maximum page-cache order
> to 0. That said, we still need to consider whether any related
> locks are held.
> 
>  From the comment, it seems that we may need to hold i_rwsem and
> invalidate_lock.

w/ above locks, it seems there is still a race condition as below:

f2fs_fileattr_set				read
  - f2fs_setflags_common
   - truncate_pagecache
						- f2fs_read_data_large_folio
						: read large folios
   - mapping_set_folio_order_range

Thanks,

> 
> /**
>   * truncate_inode_pages - truncate *all* the pages from an offset
>   * @mapping: mapping to truncate
>   * @lstart: offset from which to truncate
>   *
>   * Called under (and serialised by) inode->i_rwsem and
>   * mapping->invalidate_lock.
>   *
>   * ...
>   */
> void truncate_inode_pages(struct address_space *mapping, loff_t lstart)
> {
>          truncate_inode_pages_range(mapping, lstart, (loff_t)-1);
> }
> EXPORT_SYMBOL(truncate_inode_pages);
> 
> 
> If clearing immutable is indeed rare, we may leave this as is, since
> writes are not supported until the page cache is fully dropped.
> Eventually, we will support large folios on non-immutable files.
> 
> Thanks
> Barry

Powered by blists - more mailing lists