| Message ID | 165705853976.2820493.11634341636419465537.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: random fixes | expand |
On Tue, Jul 05, 2022 at 03:02:19PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > This test needs to fragment the free space on the data device so that > each block added to the attr fork gets its own mapping. If the test > configuration sets up a rt device and rtinherit=1 on the root dir, the > test will erroneously fragment space on the *realtime* volume. When > this happens, attr fork allocations are contiguous and get merged into > fewer than 10 extents and the test fails. > > Fix this test to force all allocations to be on the data device, and fix > incorrect variable usage in the error messages. > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > tests/xfs/547 | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > > diff --git a/tests/xfs/547 b/tests/xfs/547 > index 9d4216ca..60121eb9 100755 > --- a/tests/xfs/547 > +++ b/tests/xfs/547 > @@ -33,6 +33,10 @@ for nrext64 in 0 1; do > >> $seqres.full > _scratch_mount >> $seqres.full > > + # Force data device extents so that we can fragment the free space > + # and force attr fork allocations to be non-contiguous > + _xfs_force_bdev data $SCRATCH_MNT > + > bsize=$(_get_file_block_size $SCRATCH_MNT) > > testfile=$SCRATCH_MNT/testfile > @@ -76,13 +80,15 @@ for nrext64 in 0 1; do > acnt=$(_scratch_xfs_get_metadata_field core.naextents \ > "path /$(basename $testfile)") > > - if (( $dcnt != 10 )); then > - echo "Invalid data fork extent count: $dextcnt" > + echo "nrext64: $nrext64 dcnt: $dcnt acnt: $acnt" >> $seqres.full > + > + if [ -z "$dcnt" ] || (( $dcnt != 10 )); then I'm wondering why we need to use bash ((...)) operator at here, is $dcnt an expression? Can [ "$dcnt" != "10" ] help that? Thanks, Zorro > + echo "Invalid data fork extent count: $dcnt" > exit 1 > fi > > - if (( $acnt < 10 )); then > - echo "Invalid attr fork extent count: $aextcnt" > + if [ -z "$acnt" ] || (( $acnt < 10 )); then > + echo "Invalid attr fork extent count: $acnt" > exit 1 > fi > done >
On Thu, Jul 07, 2022 at 09:15:27PM +0800, Zorro Lang wrote: > On Tue, Jul 05, 2022 at 03:02:19PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > This test needs to fragment the free space on the data device so that > > each block added to the attr fork gets its own mapping. If the test > > configuration sets up a rt device and rtinherit=1 on the root dir, the > > test will erroneously fragment space on the *realtime* volume. When > > this happens, attr fork allocations are contiguous and get merged into > > fewer than 10 extents and the test fails. > > > > Fix this test to force all allocations to be on the data device, and fix > > incorrect variable usage in the error messages. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > tests/xfs/547 | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/tests/xfs/547 b/tests/xfs/547 > > index 9d4216ca..60121eb9 100755 > > --- a/tests/xfs/547 > > +++ b/tests/xfs/547 > > @@ -33,6 +33,10 @@ for nrext64 in 0 1; do > > >> $seqres.full > > _scratch_mount >> $seqres.full > > > > + # Force data device extents so that we can fragment the free space > > + # and force attr fork allocations to be non-contiguous > > + _xfs_force_bdev data $SCRATCH_MNT > > + > > bsize=$(_get_file_block_size $SCRATCH_MNT) > > > > testfile=$SCRATCH_MNT/testfile > > @@ -76,13 +80,15 @@ for nrext64 in 0 1; do > > acnt=$(_scratch_xfs_get_metadata_field core.naextents \ > > "path /$(basename $testfile)") > > > > - if (( $dcnt != 10 )); then > > - echo "Invalid data fork extent count: $dextcnt" > > + echo "nrext64: $nrext64 dcnt: $dcnt acnt: $acnt" >> $seqres.full > > + > > + if [ -z "$dcnt" ] || (( $dcnt != 10 )); then > > I'm wondering why we need to use bash ((...)) operator at here, is $dcnt > an expression? Can [ "$dcnt" != "10" ] help that? dcnt should be a decimal number, or the empty string if the xfs_db totally failed. The fancy comparison protects against xfs_db someday returning results in hexadecimal or a non-number string, but I don't think it'll ever do that. I think your suggestion would work for this case. I don't think it works so well for the second case, since the fancy comparison "(( $acnt < 10 ))" apparently returns false even if acnt is non-numeric, whereas "[ $acnt -lt 10 ]" would error out. --D > Thanks, > Zorro > > > + echo "Invalid data fork extent count: $dcnt" > > exit 1 > > fi > > > > - if (( $acnt < 10 )); then > > - echo "Invalid attr fork extent count: $aextcnt" > > + if [ -z "$acnt" ] || (( $acnt < 10 )); then > > + echo "Invalid attr fork extent count: $acnt" > > exit 1 > > fi > > done > > >
On Thu, Jul 07, 2022 at 11:05:25AM -0700, Darrick J. Wong wrote: > On Thu, Jul 07, 2022 at 09:15:27PM +0800, Zorro Lang wrote: > > On Tue, Jul 05, 2022 at 03:02:19PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <djwong@kernel.org> > > > > > > This test needs to fragment the free space on the data device so that > > > each block added to the attr fork gets its own mapping. If the test > > > configuration sets up a rt device and rtinherit=1 on the root dir, the > > > test will erroneously fragment space on the *realtime* volume. When > > > this happens, attr fork allocations are contiguous and get merged into > > > fewer than 10 extents and the test fails. > > > > > > Fix this test to force all allocations to be on the data device, and fix > > > incorrect variable usage in the error messages. > > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > > --- > > > tests/xfs/547 | 14 ++++++++++---- > > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > > > > > > diff --git a/tests/xfs/547 b/tests/xfs/547 > > > index 9d4216ca..60121eb9 100755 > > > --- a/tests/xfs/547 > > > +++ b/tests/xfs/547 > > > @@ -33,6 +33,10 @@ for nrext64 in 0 1; do > > > >> $seqres.full > > > _scratch_mount >> $seqres.full > > > > > > + # Force data device extents so that we can fragment the free space > > > + # and force attr fork allocations to be non-contiguous > > > + _xfs_force_bdev data $SCRATCH_MNT > > > + > > > bsize=$(_get_file_block_size $SCRATCH_MNT) > > > > > > testfile=$SCRATCH_MNT/testfile > > > @@ -76,13 +80,15 @@ for nrext64 in 0 1; do > > > acnt=$(_scratch_xfs_get_metadata_field core.naextents \ > > > "path /$(basename $testfile)") > > > > > > - if (( $dcnt != 10 )); then > > > - echo "Invalid data fork extent count: $dextcnt" > > > + echo "nrext64: $nrext64 dcnt: $dcnt acnt: $acnt" >> $seqres.full > > > + > > > + if [ -z "$dcnt" ] || (( $dcnt != 10 )); then > > > > I'm wondering why we need to use bash ((...)) operator at here, is $dcnt > > an expression? Can [ "$dcnt" != "10" ] help that? > > dcnt should be a decimal number, or the empty string if the xfs_db > totally failed. The fancy comparison protects against xfs_db someday > returning results in hexadecimal or a non-number string, but I don't Oh, it might be hexadecimal. OK, I'd like to respect the history reason. So this looks good to me. Reviewed-by: Zorro Lang <zlang@redhat.com> > think it'll ever do that. I think your suggestion would work for this > case. > > I don't think it works so well for the second case, since the fancy > comparison "(( $acnt < 10 ))" apparently returns false even if acnt is > non-numeric, whereas "[ $acnt -lt 10 ]" would error out. > > --D > > > Thanks, > > Zorro > > > > > + echo "Invalid data fork extent count: $dcnt" > > > exit 1 > > > fi > > > > > > - if (( $acnt < 10 )); then > > > - echo "Invalid attr fork extent count: $aextcnt" > > > + if [ -z "$acnt" ] || (( $acnt < 10 )); then > > > + echo "Invalid attr fork extent count: $acnt" > > > exit 1 > > > fi > > > done > > > > > >
diff --git a/tests/xfs/547 b/tests/xfs/547 index 9d4216ca..60121eb9 100755 --- a/tests/xfs/547 +++ b/tests/xfs/547 @@ -33,6 +33,10 @@ for nrext64 in 0 1; do >> $seqres.full _scratch_mount >> $seqres.full + # Force data device extents so that we can fragment the free space + # and force attr fork allocations to be non-contiguous + _xfs_force_bdev data $SCRATCH_MNT + bsize=$(_get_file_block_size $SCRATCH_MNT) testfile=$SCRATCH_MNT/testfile @@ -76,13 +80,15 @@ for nrext64 in 0 1; do acnt=$(_scratch_xfs_get_metadata_field core.naextents \ "path /$(basename $testfile)") - if (( $dcnt != 10 )); then - echo "Invalid data fork extent count: $dextcnt" + echo "nrext64: $nrext64 dcnt: $dcnt acnt: $acnt" >> $seqres.full + + if [ -z "$dcnt" ] || (( $dcnt != 10 )); then + echo "Invalid data fork extent count: $dcnt" exit 1 fi - if (( $acnt < 10 )); then - echo "Invalid attr fork extent count: $aextcnt" + if [ -z "$acnt" ] || (( $acnt < 10 )); then + echo "Invalid attr fork extent count: $acnt" exit 1 fi done