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@xxxxxxxxxx>
>> ---
>> 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
