diff mbox series

[1/2] xfs/270: only check new_ro_compat value when mkfs.xfs supports nrext64 feature

Message ID 1656340203-2322-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [1/2] xfs/270: only check new_ro_compat value when mkfs.xfs supports nrext64 feature | expand

Commit Message

xuyang2018.jy@fujitsu.com June 27, 2022, 2:30 p.m. UTC
Currently, this case fails on old xfsprogs as below:
+/var/lib/xfstests/tests/xfs/270: line 51: [: !=: unary operator expected

Getting ro_compat value will report the following error after setting new ro_compat
value:
+cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
+cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
+cache_zero_check: refcount is 1, not zero (node=0x56033fdf9110

Old xfsprogs miss a bugfix
f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()").

Here we skip the get step of new ro_comap value when nrext64 feature is supported.
Also will add a new test to cover this xfsprog bug.

Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
---
 tests/xfs/270 | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

xuyang2018.jy@fujitsu.com July 4, 2022, 9:43 a.m. UTC | #1
Hi Zorro

What do you think this patch or should I just add a 'x' string when
comparing get_value/new_value, then just remind user they may hit a old
xfsprogs bug

f4afdcb0a ("xfs_db: clean up the salvage read callsites in set_cur()").

Best Regards
Yang Xu

> Currently, this case fails on old xfsprogs as below:
> +/var/lib/xfstests/tests/xfs/270: line 51: [: !=: unary operator expected
> 
> Getting ro_compat value will report the following error after setting new ro_compat
> value:
> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
> +cache_zero_check: refcount is 1, not zero (node=0x56033fdf9110
> 
> Old xfsprogs miss a bugfix
> f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()").
> 
> Here we skip the get step of new ro_comap value when nrext64 feature is supported.
> Also will add a new test to cover this xfsprog bug.
> 
> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> ---
>   tests/xfs/270 | 23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/xfs/270 b/tests/xfs/270
> index b740c379..5ff83ead 100755
> --- a/tests/xfs/270
> +++ b/tests/xfs/270
> @@ -22,6 +22,10 @@ _require_scratch_nocheck
>   # Only V5 XFS disallow rw mount/remount with unknown ro-compat features
>   _require_scratch_xfs_crc
> 
> +nrext64_supported=0
> +_scratch_mkfs_xfs_supported -m crc=1 -i nrext64>  /dev/null 2>&1&&  \
> +	nrext64_supported=1
> +
>   _scratch_mkfs_xfs>>$seqres.full 2>&1
> 
>   # set the highest bit of features_ro_compat, use it as an unknown
> @@ -43,13 +47,18 @@ ro_compat=$(echo $ro_compat | \
>   _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
>   				>  $seqres.full 2>&1
> 
> -# read the newly set ro compat filed for verification
> -new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
> -						2>/dev/null)
> -
> -# verify the new ro_compat field is correct.
> -if [ $new_ro_compat != $ro_compat ]; then
> -	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
> +# Indeed, xfsprogs has a bug here and fixed by commit f4afdcb
> +# ("xfs_db: clean up the salvage read callsites in set_cur()")
> +# Here, we use nrext64 feature as a proxy.
> +if [ $nrext64_supported -eq 1 ]; then
> +	# read the newly set ro compat filed for verification
> +	new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" \
> +					"sb 0" 2>/dev/null)
> +	# verify the new ro_compat field is correct.
> +	if [ $new_ro_compat != $ro_compat ]; then
> +		echo "Unable to set new features_ro_compat. Wanted $ro_compat, \
> +			got $new_ro_compat"
> +	fi
>   fi
> 
>   # rw mount with unknown ro-compat feature should fail
Zorro Lang July 4, 2022, 3:42 p.m. UTC | #2
On Mon, Jun 27, 2022 at 10:30:02PM +0800, Yang Xu wrote:
> Currently, this case fails on old xfsprogs as below:
> +/var/lib/xfstests/tests/xfs/270: line 51: [: !=: unary operator expected
> 
> Getting ro_compat value will report the following error after setting new ro_compat
> value:
> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
> +cache_zero_check: refcount is 1, not zero (node=0x56033fdf9110
> 
> Old xfsprogs miss a bugfix
> f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()").
> 
> Here we skip the get step of new ro_comap value when nrext64 feature is supported.
> Also will add a new test to cover this xfsprog bug.
> 
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  tests/xfs/270 | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/xfs/270 b/tests/xfs/270
> index b740c379..5ff83ead 100755
> --- a/tests/xfs/270
> +++ b/tests/xfs/270
> @@ -22,6 +22,10 @@ _require_scratch_nocheck
>  # Only V5 XFS disallow rw mount/remount with unknown ro-compat features
>  _require_scratch_xfs_crc
>  
> +nrext64_supported=0
> +_scratch_mkfs_xfs_supported -m crc=1 -i nrext64 > /dev/null 2>&1 && \
> +	nrext64_supported=1
> +
>  _scratch_mkfs_xfs >>$seqres.full 2>&1
>  
>  # set the highest bit of features_ro_compat, use it as an unknown
> @@ -43,13 +47,18 @@ ro_compat=$(echo $ro_compat | \
>  _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
>  				> $seqres.full 2>&1
>  
> -# read the newly set ro compat filed for verification
> -new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
> -						2>/dev/null)
> -
> -# verify the new ro_compat field is correct.
> -if [ $new_ro_compat != $ro_compat ]; then

My personal opinion is -- change above line as:
   if [ "$new_ro_compat" != "$ro_compat" ]
to avoid the bash syntax error. Then the failure (on old xfsprogs) correspond
to a known xfsprogs bug. That's good enough.

Thanks,
Zorro

> -	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
> +# Indeed, xfsprogs has a bug here and fixed by commit f4afdcb
> +# ("xfs_db: clean up the salvage read callsites in set_cur()")
> +# Here, we use nrext64 feature as a proxy.
> +if [ $nrext64_supported -eq 1 ]; then
> +	# read the newly set ro compat filed for verification
> +	new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" \
> +					"sb 0" 2>/dev/null)
> +	# verify the new ro_compat field is correct.
> +	if [ $new_ro_compat != $ro_compat ]; then
> +		echo "Unable to set new features_ro_compat. Wanted $ro_compat, \
> +			got $new_ro_compat"
> +	fi
>  fi
>  
>  # rw mount with unknown ro-compat feature should fail
> -- 
> 2.27.0
>
xuyang2018.jy@fujitsu.com July 5, 2022, 5:46 a.m. UTC | #3
on 2022/07/04 23:42, Zorro Lang wrote:
> On Mon, Jun 27, 2022 at 10:30:02PM +0800, Yang Xu wrote:
>> Currently, this case fails on old xfsprogs as below:
>> +/var/lib/xfstests/tests/xfs/270: line 51: [: !=: unary operator expected
>>
>> Getting ro_compat value will report the following error after setting new ro_compat
>> value:
>> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
>> +cache_purge: shake on cache 0x56033fde4920 left 1 nodes!?
>> +cache_zero_check: refcount is 1, not zero (node=0x56033fdf9110
>>
>> Old xfsprogs miss a bugfix
>> f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()").
>>
>> Here we skip the get step of new ro_comap value when nrext64 feature is supported.
>> Also will add a new test to cover this xfsprog bug.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   tests/xfs/270 | 23 ++++++++++++++++-------
>>   1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/xfs/270 b/tests/xfs/270
>> index b740c379..5ff83ead 100755
>> --- a/tests/xfs/270
>> +++ b/tests/xfs/270
>> @@ -22,6 +22,10 @@ _require_scratch_nocheck
>>   # Only V5 XFS disallow rw mount/remount with unknown ro-compat features
>>   _require_scratch_xfs_crc
>>
>> +nrext64_supported=0
>> +_scratch_mkfs_xfs_supported -m crc=1 -i nrext64>  /dev/null 2>&1&&  \
>> +	nrext64_supported=1
>> +
>>   _scratch_mkfs_xfs>>$seqres.full 2>&1
>>
>>   # set the highest bit of features_ro_compat, use it as an unknown
>> @@ -43,13 +47,18 @@ ro_compat=$(echo $ro_compat | \
>>   _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
>>   				>  $seqres.full 2>&1
>>
>> -# read the newly set ro compat filed for verification
>> -new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
>> -						2>/dev/null)
>> -
>> -# verify the new ro_compat field is correct.
>> -if [ $new_ro_compat != $ro_compat ]; then
>
> My personal opinion is -- change above line as:
>     if [ "$new_ro_compat" != "$ro_compat" ]
> to avoid the bash syntax error. Then the failure (on old xfsprogs) correspond
> to a known xfsprogs bug. That's good enough.

Ok, will do it on v2. Thanks.

Best Regards
Yang Xu
>
> Thanks,
> Zorro
>
>> -	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
>> +# Indeed, xfsprogs has a bug here and fixed by commit f4afdcb
>> +# ("xfs_db: clean up the salvage read callsites in set_cur()")
>> +# Here, we use nrext64 feature as a proxy.
>> +if [ $nrext64_supported -eq 1 ]; then
>> +	# read the newly set ro compat filed for verification
>> +	new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" \
>> +					"sb 0" 2>/dev/null)
>> +	# verify the new ro_compat field is correct.
>> +	if [ $new_ro_compat != $ro_compat ]; then
>> +		echo "Unable to set new features_ro_compat. Wanted $ro_compat, \
>> +			got $new_ro_compat"
>> +	fi
>>   fi
>>
>>   # rw mount with unknown ro-compat feature should fail
>> --
>> 2.27.0
>>
>
diff mbox series

Patch

diff --git a/tests/xfs/270 b/tests/xfs/270
index b740c379..5ff83ead 100755
--- a/tests/xfs/270
+++ b/tests/xfs/270
@@ -22,6 +22,10 @@  _require_scratch_nocheck
 # Only V5 XFS disallow rw mount/remount with unknown ro-compat features
 _require_scratch_xfs_crc
 
+nrext64_supported=0
+_scratch_mkfs_xfs_supported -m crc=1 -i nrext64 > /dev/null 2>&1 && \
+	nrext64_supported=1
+
 _scratch_mkfs_xfs >>$seqres.full 2>&1
 
 # set the highest bit of features_ro_compat, use it as an unknown
@@ -43,13 +47,18 @@  ro_compat=$(echo $ro_compat | \
 _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
 				> $seqres.full 2>&1
 
-# read the newly set ro compat filed for verification
-new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
-						2>/dev/null)
-
-# verify the new ro_compat field is correct.
-if [ $new_ro_compat != $ro_compat ]; then
-	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
+# Indeed, xfsprogs has a bug here and fixed by commit f4afdcb
+# ("xfs_db: clean up the salvage read callsites in set_cur()")
+# Here, we use nrext64 feature as a proxy.
+if [ $nrext64_supported -eq 1 ]; then
+	# read the newly set ro compat filed for verification
+	new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" \
+					"sb 0" 2>/dev/null)
+	# verify the new ro_compat field is correct.
+	if [ $new_ro_compat != $ro_compat ]; then
+		echo "Unable to set new features_ro_compat. Wanted $ro_compat, \
+			got $new_ro_compat"
+	fi
 fi
 
 # rw mount with unknown ro-compat feature should fail