| Message ID | 20220606124101.263872-2-chandan.babu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Large extent counters tests | expand |
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)) I guess I just learned a ^^^^^ new bashism today. > +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 Thanks for the fix, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + > > # rw mount with unknown ro-compat feature should fail > echo "rw mount test" > diff --git a/tests/xfs/270.out b/tests/xfs/270.out > index 0a8b3851..edf4c254 100644 > --- a/tests/xfs/270.out > +++ b/tests/xfs/270.out > @@ -1,5 +1,4 @@ > QA output created by 270 > -features_ro_compat = 0x80000000 > rw mount test > ro mount test > rw remount test > -- > 2.35.1 >
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. Cheers, Dave.
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.
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 + # rw mount with unknown ro-compat feature should fail echo "rw mount test" diff --git a/tests/xfs/270.out b/tests/xfs/270.out index 0a8b3851..edf4c254 100644 --- a/tests/xfs/270.out +++ b/tests/xfs/270.out @@ -1,5 +1,4 @@ QA output created by 270 -features_ro_compat = 0x80000000 rw mount test ro mount test rw remount test
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(-)