[PATCH] f2fs: do f2fs_zero_post_eof_page for all the time

[PATCH] f2fs: do f2fs_zero_post_eof_page for all the time

[Jaegeuk Kim]

This fixes xfstests/generic/794, since the issue is not related to the
posix=strict mode only.

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);
 }
 
-- 
2.56.0.307.g5fe7bd680c-goog

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

[Chao Yu]

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
    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.

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);
>  }
>  

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

[Jaegeuk Kim]

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

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