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

[Chao Yu]

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@xxxxxxxxxx>

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@xxxxxxxxxx
> > > Fixes: 5eced87b7d19 ("f2fs: fix to zero post-EOF data when extending file size")
> > > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel