Re: [f2fs-dev] [PATCH] f2fs: do f2fs_zero_post_eof_page for all the time

[Chao Yu <chao@kernel.org>]

On 10/28/26 23:26, Jaegeuk Kim wrote:
> On 10/28, Chao Yu via Linux-f2fs-devel wrote:
>> On 10/28/26 13:06, Jaegeuk Kim via Linux-f2fs-devel wrote:
>>> This fixes xfstests/generic/794, since the issue is not related to the
>>> posix=strict mode only.
>>
>> Jaegeuk,
>>
>> Actually, I fixed this only in fsync_mode=strict because I don't see such
>> scenario (described in original patch) in Android:
>>
>>      Steps of generic/794:
>>      1. write 4096 bytes to file w/ 0x5a
>>      2. use fiemap to get PBA of first block in file
>>      3. truncate file to 4080
>>      4. umount; write 4096 bytes to file w/ 0x5a directly via PBA; mount

I don't remember there is any case writing via PBA across EOF in Android, let
me know if I missed something.

Ah, it seems a common issue, I can reproduce reading stale data w/ below testcase
which didn't write via PBA:

1. pwrite -S 0x5a 0 4096; fsync; sync
2. truncate 4080
3. Checkpoint without writing back data, then power cut. Two ways to cut power:
  - sysrq-b: f2fs_io checkpoint then echo b > /proc/sysrq-trigger, a real VM reset.
  - shutdown: f2fs_io shutdown 1, which checkpoints and drops dirty data.
4. After recovery, grow the file with truncate 8192, falloc 4096 4096, or pwrite @4096, then f2fs_io checkpoint, umount and mount.
5. Read [4080,4096).

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

>>      5. extend filesize via
>>         a) append 4096 from offset 4096, or
>>         b) truncate 8192, or
>>         c) fallocate 4096 from offset 4096
>>      6. verify the gap is zeroed in memory [4080,4096)
>>      7. sync range 4096 from offset 4096; shutdown -f (flush meta before shutdown)
>>      8. umount; mount; verify [4080,4096) is zeroed or not.
> 
> I saw your comment and I think we should not connect posix=strict to who's
> using that. It was supposed to handle the myth of the recovery on rename().
> This sequence looks general which seems we should guarantee.
> 
>>
>> Maybe we should only enable fsync_mode=strict for 794 to let it pass?
>>
>> Thanks,
>>
>>>
>>> Cc: stable@kernel.org
>>> Fixes: 5eced87b7d19 ("f2fs: fix to zero post-EOF data when extending file size")
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>   fs/f2fs/file.c | 14 ++------------
>>>   1 file changed, 2 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 8b4a7a9d7dbe..823df06ae0f2 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -74,15 +74,10 @@ static int f2fs_zero_post_eof_page(struct inode *inode,
>>>   					loff_t new_size, bool lock, bool writeback)
>>>   {
>>>   	loff_t old_size = i_size_read(inode);
>>> -	bool strict =
>>> -		F2FS_OPTION(F2FS_I_SB(inode)).fsync_mode == FSYNC_MODE_STRICT;
>>>   
>>>   	if (old_size >= new_size)
>>>   		return 0;
>>>   
>>> -	if (!strict && mapping_empty(inode->i_mapping))
>>> -		return 0;
>>> -
>>>   	if (lock)
>>>   		filemap_invalidate_lock(inode->i_mapping);
>>>   	/* zero or drop pages only in range of [old_size, new_size] */
>>> @@ -90,14 +85,9 @@ static int f2fs_zero_post_eof_page(struct inode *inode,
>>>   	if (lock)
>>>   		filemap_invalidate_unlock(inode->i_mapping);
>>>   
>>> -	if (!writeback || !strict)
>>> +	if (!writeback)
>>>   		return 0;
>>> -	/*
>>> -	 * In fsync_mode=strict, when we expand an unaligned EOF size, we
>>> -	 * should zero post EOF data and writeback the data immediately,
>>> -	 * so that it can avoid exposing stale data after metadata flush
>>> -	 * and POR.
>>> -	 */
>>> +
>>>   	return do_zero_post_eof_page(inode, new_size);
>>>   }
>>>   
>>
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel