Re: [PATCH 1/4] xfs/270: Fix ro mount failure when nrext64 option is enabled

[Chandan Babu R <chandan.babu@oracle.com>]

On Wed, Jun 08, 2022 at 09:51:33 AM +1000, Dave Chinner wrote:
> On Mon, Jun 06, 2022 at 06:10:58PM +0530, Chandan Babu R wrote:
>> With nrext64 option enabled at run time, the read-only mount performed by the
>> test fails because,
>> 1. mkfs.xfs would have calculated log size based on reflink being enabled.
>> 2. Clearing the reflink ro compat bit causes log size calculations to yield a
>>    different value.
>> 3. In the case where nrext64 is enabled, this causes attr reservation to be
>>    the largest among all the transaction reservations.
>> 4. This ends up causing XFS to require a larger ondisk log size than that
>>    which is available.
>> 
>> This commit fixes the problem by setting features_ro_compat to the value
>> obtained by the bitwise-OR of features_ro_compat field with 2^31.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  tests/xfs/270     | 16 ++++++++++++++--
>>  tests/xfs/270.out |  1 -
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tests/xfs/270 b/tests/xfs/270
>> index 0ab0c7d8..f3796691 100755
>> --- a/tests/xfs/270
>> +++ b/tests/xfs/270
>> @@ -27,8 +27,20 @@ _scratch_mkfs_xfs >>$seqres.full 2>&1
>>  # set the highest bit of features_ro_compat, use it as an unknown
>>  # feature bit. If one day this bit become known feature, please
>>  # change this case.
>> -_scratch_xfs_set_metadata_field "features_ro_compat" "$((2**31))" "sb 0" | \
>> -	grep 'features_ro_compat'
>> +ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
>> +ro_compat=${ro_compat##0x}
>> +ro_compat="16#"${ro_compat}
>> +ro_compat=$(($ro_compat|16#80000000))
>> +ro_compat=$(_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" \
>> +					    "sb 0" | grep 'features_ro_compat')
>> +
>> +ro_compat=${ro_compat##features_ro_compat = 0x}
>> +ro_compat="16#"${ro_compat}
>> +ro_compat=$(($ro_compat&16#80000000))
>> +if (( $ro_compat != 16#80000000 )); then
>> +	echo "Unable to set most significant bit of features_ro_compat"
>> +fi
>
> Urk. Bash - the new line noise generator. :(
>
> This is basically just bit manipulation in hex format. db accepts
> hex format integers (i.e. 0x1234), and according to the bash man
> page, it understands the 0x1234 prefix as well. So AFAICT there's no
> need for this weird "16#" prefix for the bit operations.
>
> But regardless of that, just because you can do something in bash
> doesn't mean you should:
>
> wit://utcc.utoronto.ca/~cks/space/blog/programming/ShellScriptsBeClearFirst
>
> IMO, this reads much better as something like:
>
> # grab the current ro compat fields and add an invalid high bit.
> ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
> 		awk '/features_ro_compat/ {
> 			printf("0x%x\n", or(strtonum($3), 0x80000000)
> 		}')
>
> # write the new ro compat field to the superblock
> _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0"
>
> # read the newly set ro compat filed for verification
> new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
> 		awk '/features_ro_compat/ {
> 			printf("0x%x\n", $3)
> 		}')
>
> # 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
>
> Yes, it's more lines of code, but it's easy to read, easy to
> understand, and easy to modify in future.
>

Thanks for the review. I will ensure to resort to weird bashisms unless there
are no alternate options available.

-- 
chandan