[PATCH] generic/392: stop checking st_blocks

[PATCH] generic/392: stop checking st_blocks

[Christoph Hellwig]

st_blocks is a rather vaguely defined field.  To quote the Linux stat(2)
man page:

    Use of the st_blocks and st_blksize fields may be less portable.
    (They were introduced in BSD.  The interpretation differs between
    systems, and possibly on a single system when NFS mounts are
    involved.)

or the FreeBSD one:

    st_blocks   Actual number of blocks allocated for the file in
		512-byte units.  As short symbolic links are stored in
		the inode, this number may be zero.

and at least for XFS they include speculative preallocations and
in-flight COW fork allocations, and the numbers can change when the way
how data is stored is reorganized.  Because of that it doesn't make sense
to require st_blocks to not change after a crash even when fsync or
fdatasync was involved.

Remove the st_blocks checks and the now superfluous XFS always_cow
workaround.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/generic/392 | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/tests/generic/392 b/tests/generic/392
index c4bb3f4b9..0c9efb6df 100755
--- a/tests/generic/392
+++ b/tests/generic/392
@@ -6,7 +6,7 @@
 #
 # Test inode's metadata after fsync or fdatasync calls.
 # In the case of fsync, filesystem should recover all the inode metadata, while
-# recovering i_blocks and i_size at least for fdatasync.
+# recovering for fdatasync it should at least recovery i_size.
 #
 . ./common/preamble
 _begin_fstest shutdown auto quick metadata punch
@@ -28,16 +28,6 @@ _scratch_mkfs >/dev/null 2>&1
 _require_metadata_journaling $SCRATCH_DEV
 _scratch_mount
 
-# This test requires that i_blocks remains unchanged from the start of the
-# check_inode_metadata call until after recovery is complete.  fpunch calls
-# turn into pagecache writes if the arguments are not aligned to the fs
-# blocksize.  If the range being punched is already mapped to a written extent
-# and alwayscow is enabled, i_blocks will increase by the size of the COW
-# staging extent.  This causes stat to report different numbers for %b, which
-# results in a test failure.  Hence do not run this test if XFS is in alwayscow
-# mode.
-test "$FSTYP" = "xfs" && _require_no_xfs_always_cow
-
 testfile=$SCRATCH_MNT/testfile
 
 # check inode metadata after shutdown
@@ -47,9 +37,9 @@ check_inode_metadata()
 
 	# fsync or fdatasync
 	if [ $sync_mode = "fsync" ]; then
-		stat_opt='-c "b: %b s: %s a: %x m: %y c: %z"'
+		stat_opt='-c "s: %s a: %x m: %y c: %z"'
 	else
-		stat_opt='-c "b: %b s: %s"'
+		stat_opt='-c "s: %s"'
 	fi
 
 	before=`stat "$stat_opt" $testfile`
-- 
2.39.2

Re: [PATCH] generic/392: stop checking st_blocks

[Darrick J. Wong]

On Mon, Feb 26, 2024 at 11:03:19AM +0100, Christoph Hellwig wrote:
> st_blocks is a rather vaguely defined field.  To quote the Linux stat(2)
> man page:
> 
>     Use of the st_blocks and st_blksize fields may be less portable.
>     (They were introduced in BSD.  The interpretation differs between
>     systems, and possibly on a single system when NFS mounts are
>     involved.)
> 
> or the FreeBSD one:
> 
>     st_blocks   Actual number of blocks allocated for the file in
> 		512-byte units.  As short symbolic links are stored in
> 		the inode, this number may be zero.
> 
> and at least for XFS they include speculative preallocations and
> in-flight COW fork allocations, and the numbers can change when the way
> how data is stored is reorganized.  Because of that it doesn't make sense
> to require st_blocks to not change after a crash even when fsync or
> fdatasync was involved.
> 
> Remove the st_blocks checks and the now superfluous XFS always_cow
> workaround.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I've long found the st_blocks checking suspect, so

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/generic/392 | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/generic/392 b/tests/generic/392
> index c4bb3f4b9..0c9efb6df 100755
> --- a/tests/generic/392
> +++ b/tests/generic/392
> @@ -6,7 +6,7 @@
>  #
>  # Test inode's metadata after fsync or fdatasync calls.
>  # In the case of fsync, filesystem should recover all the inode metadata, while
> -# recovering i_blocks and i_size at least for fdatasync.
> +# recovering for fdatasync it should at least recovery i_size.
>  #
>  . ./common/preamble
>  _begin_fstest shutdown auto quick metadata punch
> @@ -28,16 +28,6 @@ _scratch_mkfs >/dev/null 2>&1
>  _require_metadata_journaling $SCRATCH_DEV
>  _scratch_mount
>  
> -# This test requires that i_blocks remains unchanged from the start of the
> -# check_inode_metadata call until after recovery is complete.  fpunch calls
> -# turn into pagecache writes if the arguments are not aligned to the fs
> -# blocksize.  If the range being punched is already mapped to a written extent
> -# and alwayscow is enabled, i_blocks will increase by the size of the COW
> -# staging extent.  This causes stat to report different numbers for %b, which
> -# results in a test failure.  Hence do not run this test if XFS is in alwayscow
> -# mode.
> -test "$FSTYP" = "xfs" && _require_no_xfs_always_cow
> -
>  testfile=$SCRATCH_MNT/testfile
>  
>  # check inode metadata after shutdown
> @@ -47,9 +37,9 @@ check_inode_metadata()
>  
>  	# fsync or fdatasync
>  	if [ $sync_mode = "fsync" ]; then
> -		stat_opt='-c "b: %b s: %s a: %x m: %y c: %z"'
> +		stat_opt='-c "s: %s a: %x m: %y c: %z"'
>  	else
> -		stat_opt='-c "b: %b s: %s"'
> +		stat_opt='-c "s: %s"'
>  	fi
>  
>  	before=`stat "$stat_opt" $testfile`
> -- 
> 2.39.2
> 
> 

Re: [PATCH] generic/392: stop checking st_blocks

[Christoph Hellwig]

Zorro, I noticed this didn't make it to the pending queue, can you
pick it up?

Re: [PATCH] generic/392: stop checking st_blocks

[Zorro Lang]

On Fri, Mar 08, 2024 at 04:43:16PM +0100, Christoph Hellwig wrote:
> Zorro, I noticed this didn't make it to the pending queue, can you
> pick it up?

Sure Christoph, sorry I did some rebase work locally, some patches got
changed, will re-create/push it later.

Thanks,
Zorro

>