On 28 Jul 2021 at 00:07, Darrick J. Wong wrote:
> On Tue, Jul 27, 2021 at 10:15:27AM +0530, Chandan Babu R wrote:
>> On 26 Jul 2021 at 22:49, Darrick J. Wong wrote:
>> > On Mon, Jul 26, 2021 at 12:13:13PM +0530, Chandan Babu R wrote:
>> >> _scratch_do_mkfs constructs a mkfs command line by concatenating the values of
>> >> 1. $mkfs_cmd
>> >> 2. $MKFS_OPTIONS
>> >> 3. $extra_mkfs_options
>> >>
>> >> The corresponding mkfs command line fails if $MKFS_OPTIONS enables either
>> >> reflink or rmapbt feature. The failure occurs because the test tries to create
>> >> a filesystem with realtime device enabled. In such a case, _scratch_do_mkfs()
>> >> will construct and invoke an mkfs command line without including the value of
>> >> $MKFS_OPTIONS.
>> >>
>> >> To prevent such silent failures, this commit causes the test to exit if it
>> >> detects either reflink or rmapbt feature being enabled.
>> >
>> > Er, what combinations of mkfs.xfs and MKFS_OPTIONS cause this result?
>> > What kind of fs configuration comes out of that?
>>
>> With MKFS_OPTIONS set as shown below,
>>
>> export MKFS_OPTIONS="-m reflink=1 -b size=1k"
>>
>> _scratch_do_mkfs() invokes mkfs.xfs with both realtime and reflink options
>> enabled. Such an invocation of mkfs.xfs fails causing _scratch_do_mkfs() to
>> ignore the contents of $MKFS_OPTIONS while constructing and invoking mkfs.xfs
>> once again.
>>
>> This time, the fs block size will however be set to 4k (the default block
>> size). At the beginning of the test we would have obtained the block size of
>> the filesystem as 1k and used it to compute the size of the realtime device
>> required to overflow realtime bitmap inode's max pseudo extent count.
>>
>> Invocation of xfs_growfs (made later in the test) ends up succeeding since a
>> 4k fs block can accommodate more bits than a 1k fs block.
>
> OK, now I think I've finally put all the pieces together. Both of these
> patches are fixing weirdness when MKFS_OPTIONS="-m reflink=1 -b size=1k".
>
> With current HEAD, we try to mkfs.xfs with double "-b size" arguments.
> That fails with 'option respecified', so fstests tries again without
> MKFS_OPTIONS, which means you don't get the filesystem that you want.
> If, say, MKFS_OPTIONS also contained bigtime=1, you won't get a bigtime
> filesystem.
>
> So the first patch removes the double -bsize arguments. But you still
> have the problem that the reflink=1 in MKFS_OPTIONS still causes
> mkfs.xfs to fail (because we don't do rt and reflink yet), so fstests
> again drops MKFS_OPTIONS, and now you're testing the fs without a block
> size option at all. The test still regresses because the special rt
> geometry depends on the blocksize, and we didn't get all the geometry
> elements that we need to trip the growfs failure.
>
> Does the following patch fix all that for you?
>
> --D
>
> diff --git a/tests/xfs/530 b/tests/xfs/530
> index 4d168ac5..9c6f44d7 100755
> --- a/tests/xfs/530
> +++ b/tests/xfs/530
> @@ -60,10 +60,22 @@ echo "Format and mount rt volume"
>
> export USE_EXTERNAL=yes
> export SCRATCH_RTDEV=$rtdev
> -_scratch_mkfs -d size=$((1024 * 1024 * 1024)) -b size=${dbsize} \
> +_scratch_mkfs -d size=$((1024 * 1024 * 1024)) \
> -r size=${rtextsz},extsize=${rtextsz} >> $seqres.full
> _try_scratch_mount || _notrun "Couldn't mount fs with synthetic rt volume"
>
> +# If we didn't get the desired realtime volume and the same blocksize as the
> +# first format (which we used to compute a specific rt geometry), skip the
> +# test. This can happen if the MKFS_OPTIONS conflict with the ones we passed
> +# to _scratch_mkfs or do not result in a valid rt fs geometry. In this case,
> +# _scratch_mkfs will try to "succeed" at formatting by dropping MKFS_OPTIONS,
> +# giving us the wrong geometry.
> +formatted_blksz="$(_get_block_size $SCRATCH_MNT)"
> +test "$formatted_blksz" -ne "$dbsize" && \
> + _notrun "Tried to format with $dbsize blocksize, got $formatted_blksz."
> +$XFS_INFO_PROG $SCRATCH_MNT | egrep -q 'realtime.*blocks=0' && \
> + _notrun "Filesystem should have a realtime volume"
> +
The above solution indeed fixes the problems that patch 2 and 3 were
attempting to solve. If you plan on posting it, you can add
Reviewed-by: Chandan Babu R <chandanrlinux@xxxxxxxxx>
--
chandan
