xfs/278: mkfs with v4 format

Commit Message

Eric Sandeen March 22, 2018, 8:09 p.m. UTC

This test had silently stopped working when mkfs.xfs switched
to v5 supers by default, and changed inode sizes:

> field u not found
> parsing error
> field u not found
> parsing error

...

Switch back to the original behavior by turning off crcs,
and catch such failures if they crop up again by looking
for xfs_db errors in $seqres.full.

Signed-off-by: Eric Sandeen <[e-mail]>
---

(is it horrific to grep $seqres.full?  xfs_db doesn't exit
with failure in this case :( )

and now this causes repair to fail on a verifier error, but
I just sent an xfsprogs patch to fix that as well.


--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Darrick J. Wong March 22, 2018, 8:26 p.m. UTC | #1

> > parsing error
> > field u not found
> > parsing error
> 
> ...
> 
> Switch back to the original behavior by turning off crcs,
> and catch such failures if they crop up again by looking
> for xfs_db errors in $seqres.full.
> 
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> (is it horrific to grep $seqres.full?  xfs_db doesn't exit
> with failure in this case :( )
> 
> and now this causes repair to fail on a verifier error, but
> I just sent an xfsprogs patch to fix that as well.
> 
> diff --git a/tests/xfs/278 b/tests/xfs/278
> index b94ee9c..c0e09c7 100755
> --- a/tests/xfs/278
> +++ b/tests/xfs/278
> @@ -48,7 +48,7 @@ _supported_os Linux
>  _require_scratch
>  
>  rm -f $seqres.full
> -_scratch_mkfs >$seqres.full 2>&1
> +_scratch_mkfs -m crc=0 -n ftype=0 >$seqres.full 2>&1

This is still a problem on v5 filesystems, isn't it?

>  _scratch_mount
>  
>  mkdir -p $SCRATCH_MNT/dir/subdir
> @@ -73,6 +73,8 @@ _scratch_xfs_db -x -c "inode $DIR_INO" -c "write core.nlinkv2 0"  >> $seqres.ful
>  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write u.sfdir2.hdr.parent.i4 0"  >> $seqres.full

So I think the problem here is that between v4 and v5 the field names
changed slightly?

u.sfdir2.hdr.parent.i4 vs. u3.sfdir3.hdr.parent.i4?

So we ought to be able to _scratch_xfs_get_metadata_field to see if the
v4 field name exists, then _scratch_xfs_set_metadata_field with the
appropriate name to set the field?

ino_loc="inode $SUBDIR_INO"
if [ -n "$(_scratch_xfs_get_metadata_field "u.sfdir2.hdr.parent.i4" "$ino_loc" ]; then
	_scratch_xfs_set_metadata_field "u.sfdir2.hdr.parent.i4" 0 "$ino_loc"
else
	_scratch_xfs_set_metadata_field "u3.sfdir3.hdr.parent.i4" 0 "$ino_loc"
fi

Or something like that.

--D

>  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write core.nlinkv2 0"  >> $seqres.full
>  
> +grep -q "not found\|parsing error" $seqres.full && _fail "xfs_db commands failed"
> +
>  echo "===== BEGIN of xfs_repair =====" >> $seqres.full
>  echo "" >>$seqres.full
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eric Sandeen March 22, 2018, 8:37 p.m. UTC | #2

On 3/22/18 3:26 PM, Darrick J. Wong wrote:
> 
>>> parsing error
>>> field u not found
>>> parsing error
>>
>> ...
>>
>> Switch back to the original behavior by turning off crcs,
>> and catch such failures if they crop up again by looking
>> for xfs_db errors in $seqres.full.
>>
>> Signed-off-by: Eric Sandeen <[e-mail]>
>> ---
>>
>> (is it horrific to grep $seqres.full?  xfs_db doesn't exit
>> with failure in this case :( )
>>
>> and now this causes repair to fail on a verifier error, but
>> I just sent an xfsprogs patch to fix that as well.
>>
>> diff --git a/tests/xfs/278 b/tests/xfs/278
>> index b94ee9c..c0e09c7 100755
>> --- a/tests/xfs/278
>> +++ b/tests/xfs/278
>> @@ -48,7 +48,7 @@ _supported_os Linux
>>  _require_scratch
>>  
>>  rm -f $seqres.full
>> -_scratch_mkfs >$seqres.full 2>&1
>> +_scratch_mkfs -m crc=0 -n ftype=0 >$seqres.full 2>&1
> 
> This is still a problem on v5 filesystems, isn't it?

... but the above makes it V4, right?

> 
>>  _scratch_mount
>>  
>>  mkdir -p $SCRATCH_MNT/dir/subdir
>> @@ -73,6 +73,8 @@ _scratch_xfs_db -x -c "inode $DIR_INO" -c "write core.nlinkv2 0"  >> $seqres.ful
>>  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write u.sfdir2.hdr.parent.i4 0"  >> $seqres.full
> 
> So I think the problem here is that between v4 and v5 the field names
> changed slightly?
> 
> u.sfdir2.hdr.parent.i4 vs. u3.sfdir3.hdr.parent.i4?
> 
> So we ought to be able to _scratch_xfs_get_metadata_field to see if the
> v4 field name exists, then _scratch_xfs_set_metadata_field with the
> appropriate name to set the field?
> 
> ino_loc="inode $SUBDIR_INO"
> if [ -n "$(_scratch_xfs_get_metadata_field "u.sfdir2.hdr.parent.i4" "$ino_loc" ]; then
> 	_scratch_xfs_set_metadata_field "u.sfdir2.hdr.parent.i4" 0 "$ino_loc"
> else
> 	_scratch_xfs_set_metadata_field "u3.sfdir3.hdr.parent.i4" 0 "$ino_loc"
> fi
> 
> Or something like that.
> 

yeah, could do, is it worth it?  Trying to exercise repair's lost+found behavior,
I'm not sure how valuable it is to add a bunch of code to handle every possible
format to get to the same endpoint...

My original plan was to just constrain the test to a known quantity to test
the target functionality that way ...

-Eric
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eryu Guan March 23, 2018, 2:16 a.m. UTC | #3

On Thu, Mar 22, 2018 at 03:09:53PM -0500, Eric Sandeen wrote:
> This test had silently stopped working when mkfs.xfs switched
> to v5 supers by default, and changed inode sizes:
> 
> > field u not found
> > parsing error
> > field u not found
> > parsing error
> 
> ...
> 
> Switch back to the original behavior by turning off crcs,
> and catch such failures if they crop up again by looking
> for xfs_db errors in $seqres.full.
> 
> Signed-off-by: Eric Sandeen <[e-mail]>
> ---
> 
> (is it horrific to grep $seqres.full?  xfs_db doesn't exit
> with failure in this case :( )

I think that's fine, at least xfs/083 and ext4/006 do grep it :)

Or we can redirect xfs_db output to a $tmp.xfs_db file and grep it, and
append the file content to $seqres.full if necessary, so that we just
grep what we care about, not the full $seqres.full file.

> 
> and now this causes repair to fail on a verifier error, but
> I just sent an xfsprogs patch to fix that as well.
> 
> diff --git a/tests/xfs/278 b/tests/xfs/278
> index b94ee9c..c0e09c7 100755
> --- a/tests/xfs/278
> +++ b/tests/xfs/278
> @@ -48,7 +48,7 @@ _supported_os Linux
>  _require_scratch
>  
>  rm -f $seqres.full
> -_scratch_mkfs >$seqres.full 2>&1
> +_scratch_mkfs -m crc=0 -n ftype=0 >$seqres.full 2>&1

At first I thought this might lose test coverage with v5 filesystem, but
after looking at the xfsprogs commit that fixed the original bug (commit
198b747f2553 ("repair: properly mark lost+found inode as used")), it
seems like that both v4 and v5 go to the same code path, and we won't
lose test coverage.

If this is the case, I think it's worth mentioning it in commit log too.
Otherwise looks fine to me.

Thanks,
Eryu

>  _scratch_mount
>  
>  mkdir -p $SCRATCH_MNT/dir/subdir
> @@ -73,6 +73,8 @@ _scratch_xfs_db -x -c "inode $DIR_INO" -c "write core.nlinkv2 0"  >> $seqres.ful
>  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write u.sfdir2.hdr.parent.i4 0"  >> $seqres.full
>  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write core.nlinkv2 0"  >> $seqres.full
>  
> +grep -q "not found\|parsing error" $seqres.full && _fail "xfs_db commands failed"
> +
>  echo "===== BEGIN of xfs_repair =====" >> $seqres.full
>  echo "" >>$seqres.full
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong March 23, 2018, 4:49 p.m. UTC | #4

On Fri, Mar 23, 2018 at 10:16:00AM +0800, Eryu Guan wrote:
> On Thu, Mar 22, 2018 at 03:09:53PM -0500, Eric Sandeen wrote:
> > This test had silently stopped working when mkfs.xfs switched
> > to v5 supers by default, and changed inode sizes:
> > 
> > > field u not found
> > > parsing error
> > > field u not found
> > > parsing error
> > 
> > ...
> > 
> > Switch back to the original behavior by turning off crcs,
> > and catch such failures if they crop up again by looking
> > for xfs_db errors in $seqres.full.
> > 
> > Signed-off-by: Eric Sandeen <[e-mail]>
> > ---
> > 
> > (is it horrific to grep $seqres.full?  xfs_db doesn't exit
> > with failure in this case :( )
> 
> I think that's fine, at least xfs/083 and ext4/006 do grep it :)
> 
> Or we can redirect xfs_db output to a $tmp.xfs_db file and grep it, and
> append the file content to $seqres.full if necessary, so that we just
> grep what we care about, not the full $seqres.full file.
> 
> > 
> > and now this causes repair to fail on a verifier error, but
> > I just sent an xfsprogs patch to fix that as well.
> > 
> > diff --git a/tests/xfs/278 b/tests/xfs/278
> > index b94ee9c..c0e09c7 100755
> > --- a/tests/xfs/278
> > +++ b/tests/xfs/278
> > @@ -48,7 +48,7 @@ _supported_os Linux
> >  _require_scratch
> >  
> >  rm -f $seqres.full
> > -_scratch_mkfs >$seqres.full 2>&1
> > +_scratch_mkfs -m crc=0 -n ftype=0 >$seqres.full 2>&1
> 
> At first I thought this might lose test coverage with v5 filesystem, but
> after looking at the xfsprogs commit that fixed the original bug (commit
> 198b747f2553 ("repair: properly mark lost+found inode as used")), it
> seems like that both v4 and v5 go to the same code path, and we won't
> lose test coverage.

I don't like the idea of losing test coverage on v5 filesystems just to
avoid having to rewrite this test to detect the xfs_db inode field name
prefix.  At some point in the near(ish) future we're going to start
landing the parent pointer code at which point the v5 code paths in
repair /will/ start to diverge.

Anyway, I have my autodetect patch ready so I'll send that shortly.

--D

> If this is the case, I think it's worth mentioning it in commit log too.
> Otherwise looks fine to me.
> 
> Thanks,
> Eryu
> 
> >  _scratch_mount
> >  
> >  mkdir -p $SCRATCH_MNT/dir/subdir
> > @@ -73,6 +73,8 @@ _scratch_xfs_db -x -c "inode $DIR_INO" -c "write core.nlinkv2 0"  >> $seqres.ful
> >  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write u.sfdir2.hdr.parent.i4 0"  >> $seqres.full
> >  _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write core.nlinkv2 0"  >> $seqres.full
> >  
> > +grep -q "not found\|parsing error" $seqres.full && _fail "xfs_db commands failed"
> > +
> >  echo "===== BEGIN of xfs_repair =====" >> $seqres.full
> >  echo "" >>$seqres.full
> >  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to [e-mail]
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/tests/xfs/278 b/tests/xfs/278
index b94ee9c..c0e09c7 100755
--- a/tests/xfs/278
+++ b/tests/xfs/278
@@ -48,7 +48,7 @@  _supported_os Linux
 _require_scratch
 
 rm -f $seqres.full
-_scratch_mkfs >$seqres.full 2>&1
+_scratch_mkfs -m crc=0 -n ftype=0 >$seqres.full 2>&1
 _scratch_mount
 
 mkdir -p $SCRATCH_MNT/dir/subdir
@@ -73,6 +73,8 @@  _scratch_xfs_db -x -c "inode $DIR_INO" -c "write core.nlinkv2 0"  >> $seqres.ful
 _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write u.sfdir2.hdr.parent.i4 0"  >> $seqres.full
 _scratch_xfs_db -x -c "inode $SUBDIR_INO" -c "write core.nlinkv2 0"  >> $seqres.full
 
+grep -q "not found\|parsing error" $seqres.full && _fail "xfs_db commands failed"
+
 echo "===== BEGIN of xfs_repair =====" >> $seqres.full
 echo "" >>$seqres.full