| Message ID | 20240226100319.280355-1-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | generic/392: stop checking st_blocks | expand |
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 > >
Zorro, I noticed this didn't make it to the pending queue, can you pick it up?
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 >
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`
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(-)