| Message ID | 20220209123305.253038-3-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: fix _scratch_mkfs_sized failure handling | expand |
On Wed, Feb 09, 2022 at 09:33:01PM +0900, Shin'ichiro Kawasaki wrote: > The test case generic/204 calls _scratch_mkfs to get data block size and > i-node size of the filesystem and obtained data block size is passed to > the following _scratch_mfks_sized call as an option. However, the > _scratch_mkfs call is unnecessary since the sizes can be obtained by > _scratch_mkfs_sized call without the data block size option. > > Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds > and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs > leaves valid working filesystem on scratch device then following mount > and IO operations can not detect the failure of _scratch_mkfs_sized. > This results in the test case run with unexpected test condition. > > Hence, remove the _scratch_mkfs call and the data block size option for > _scratch_mkfs_sized call. > > Suggested-by: Darrick J. Wong <djwong@kernel.org> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Looks ok, assuming you've verified that fstests with FSTYP=xfs doesn't regress... Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > tests/generic/204 | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/tests/generic/204 b/tests/generic/204 > index a3dabb71..a33a090f 100755 > --- a/tests/generic/204 > +++ b/tests/generic/204 > @@ -24,10 +24,6 @@ _supported_fs generic > > _require_scratch > > -# get the block size first > -_scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null > -. $tmp.mkfs > - > # For xfs, we need to handle the different default log sizes that different > # versions of mkfs create. All should be valid with a 16MB log, so use that. > # And v4/512 v5/1k xfs don't have enough free inodes, set imaxpct=50 at mkfs > @@ -35,7 +31,7 @@ _scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null > [ $FSTYP = "xfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -l size=16m -i maxpct=50" > > SIZE=`expr 115 \* 1024 \* 1024` > -_scratch_mkfs_sized $SIZE $dbsize 2> /dev/null > $tmp.mkfs.raw > +_scratch_mkfs_sized $SIZE 2> /dev/null > $tmp.mkfs.raw > cat $tmp.mkfs.raw | _filter_mkfs 2> $tmp.mkfs > /dev/null > _scratch_mount > > -- > 2.34.1 >
On Feb 09, 2022 / 14:31, Darrick J. Wong wrote: > On Wed, Feb 09, 2022 at 09:33:01PM +0900, Shin'ichiro Kawasaki wrote: > > The test case generic/204 calls _scratch_mkfs to get data block size and > > i-node size of the filesystem and obtained data block size is passed to > > the following _scratch_mfks_sized call as an option. However, the > > _scratch_mkfs call is unnecessary since the sizes can be obtained by > > _scratch_mkfs_sized call without the data block size option. > > > > Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds > > and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs > > leaves valid working filesystem on scratch device then following mount > > and IO operations can not detect the failure of _scratch_mkfs_sized. > > This results in the test case run with unexpected test condition. > > > > Hence, remove the _scratch_mkfs call and the data block size option for > > _scratch_mkfs_sized call. > > > > Suggested-by: Darrick J. Wong <djwong@kernel.org> > > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> > > Looks ok, assuming you've verified that fstests with FSTYP=xfs doesn't > regress... > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> Thanks for reviewing. I tested the test case with FSTYP=xfs on a few devices and 3 variety of MKFS_OPTIONS (no option, "-b size=1024 -i size=512" and "-b size=4096 -i size=2048") and all passed. Also I ran whole fstests with FSTYP=xfs, and confirmed that this change does not cause additional failure.
diff --git a/tests/generic/204 b/tests/generic/204 index a3dabb71..a33a090f 100755 --- a/tests/generic/204 +++ b/tests/generic/204 @@ -24,10 +24,6 @@ _supported_fs generic _require_scratch -# get the block size first -_scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null -. $tmp.mkfs - # For xfs, we need to handle the different default log sizes that different # versions of mkfs create. All should be valid with a 16MB log, so use that. # And v4/512 v5/1k xfs don't have enough free inodes, set imaxpct=50 at mkfs @@ -35,7 +31,7 @@ _scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null [ $FSTYP = "xfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -l size=16m -i maxpct=50" SIZE=`expr 115 \* 1024 \* 1024` -_scratch_mkfs_sized $SIZE $dbsize 2> /dev/null > $tmp.mkfs.raw +_scratch_mkfs_sized $SIZE 2> /dev/null > $tmp.mkfs.raw cat $tmp.mkfs.raw | _filter_mkfs 2> $tmp.mkfs > /dev/null _scratch_mount
The test case generic/204 calls _scratch_mkfs to get data block size and i-node size of the filesystem and obtained data block size is passed to the following _scratch_mfks_sized call as an option. However, the _scratch_mkfs call is unnecessary since the sizes can be obtained by _scratch_mkfs_sized call without the data block size option. Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs leaves valid working filesystem on scratch device then following mount and IO operations can not detect the failure of _scratch_mkfs_sized. This results in the test case run with unexpected test condition. Hence, remove the _scratch_mkfs call and the data block size option for _scratch_mkfs_sized call. Suggested-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- tests/generic/204 | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)