[PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs

[PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs

[Chandan Babu R]

_scratch_do_mkfs constructs a mkfs command line by concatenating the values of
1. $mkfs_cmd
2. $MKFS_OPTIONS
3. $extra_mkfs_options

The block size argument passed by xfs/530 to _scratch_mkfs() will cause
mkfs.xfs to fail if $MKFS_OPTIONS also has a block size specified. 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 removes the block size option
that was being explicitly passed to _scratch_mkfs().

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 tests/xfs/530 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/xfs/530 b/tests/xfs/530
index 4d168ac5..16dc426c 100755
--- a/tests/xfs/530
+++ b/tests/xfs/530
@@ -60,7 +60,7 @@ 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"
 
-- 
2.30.2

[PATCH 2/3] common/xfs: Add helpers to obtain reflink/rmapbt status of a filesystem

[Chandan Babu R]

This commit adds helpers to obtain status of reflink and rmapbt features of a
filesystem. The status of these features are obtained by invoking
$XFS_INFO_PROG program.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 common/xfs | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/common/xfs b/common/xfs
index c5e39427..e9f84b56 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1099,6 +1099,40 @@ _xfs_mount_agcount()
 	$XFS_INFO_PROG "$1" | grep agcount= | sed -e 's/^.*agcount=\([0-9]*\),.*$/\1/g'
 }
 
+# Get reflink status of a filesystem
+_xfs_is_reflink_enabled()
+{
+	local status
+
+	$XFS_INFO_PROG "$1" | grep -q reflink >> $seqres.full
+	[[ $? != 0 ]] && return 1
+
+	status=$($XFS_INFO_PROG "$1" | grep reflink= | \
+			 sed -e 's/^.*reflink=\([0-1]\).*$/\1/g')
+	if [[ $status == 1 ]]; then
+		return 0
+	else
+		return 1
+	fi
+}
+
+# Get rmapbt status of a filesystem
+_xfs_is_rmapbt_enabled()
+{
+	local status
+
+	$XFS_INFO_PROG "$1" | grep -q rmapbt >> $seqres.full
+	[[ $? != 0 ]] && return 1
+
+	status=$($XFS_INFO_PROG "$1" | grep rmapbt= | \
+			 sed -e 's/^.*rmapbt=\([0-1]\).*$/\1/g')
+	if [[ $status == 1 ]]; then
+		return 0
+	else
+		return 1
+	fi
+}
+
 # Wipe the superblock of each XFS AGs
 _try_wipe_scratch_xfs()
 {
-- 
2.30.2

[PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Chandan Babu R]

_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.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 tests/xfs/530 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/xfs/530 b/tests/xfs/530
index 16dc426c..669b061d 100755
--- a/tests/xfs/530
+++ b/tests/xfs/530
@@ -39,6 +39,12 @@ _require_scratch_nocheck
 echo "* Test extending rt inodes"
 
 _scratch_mkfs | _filter_mkfs >> $seqres.full 2> $tmp.mkfs
+
+_xfs_is_reflink_enabled $SCRATCH_DEV && \
+	_notrun "Realtime device cannot be used when reflink feature is enabled"
+_xfs_is_rmapbt_enabled $SCRATCH_DEV && \
+	_notrun "Realtime device cannot be used when rmapbt feature is enabled"
+
 . $tmp.mkfs
 
 echo "Create fake rt volume"
-- 
2.30.2

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Darrick J. Wong]

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?

Eventually, the plan is to support rmap[1] and reflink[2] on the
realtime device, at which point this will have to be torn out and a
better solution found.

--D

[1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-rmap
[2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-reflink

> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  tests/xfs/530 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/xfs/530 b/tests/xfs/530
> index 16dc426c..669b061d 100755
> --- a/tests/xfs/530
> +++ b/tests/xfs/530
> @@ -39,6 +39,12 @@ _require_scratch_nocheck
>  echo "* Test extending rt inodes"
>  
>  _scratch_mkfs | _filter_mkfs >> $seqres.full 2> $tmp.mkfs
> +
> +_xfs_is_reflink_enabled $SCRATCH_DEV && \
> +	_notrun "Realtime device cannot be used when reflink feature is enabled"
> +_xfs_is_rmapbt_enabled $SCRATCH_DEV && \
> +	_notrun "Realtime device cannot be used when rmapbt feature is enabled"
> +
>  . $tmp.mkfs
>  
>  echo "Create fake rt volume"
> -- 
> 2.30.2
> 

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Chandan Babu R]

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.

>
> Eventually, the plan is to support rmap[1] and reflink[2] on the
> realtime device, at which point this will have to be torn out and a
> better solution found.
>
> --D
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-rmap
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-reflink
>

--
chandan

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Darrick J. Wong]

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"
+
 echo "Consume free space"
 fillerdir=$SCRATCH_MNT/fillerdir
 nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)

> >
> > Eventually, the plan is to support rmap[1] and reflink[2] on the
> > realtime device, at which point this will have to be torn out and a
> > better solution found.
> >
> > --D
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-rmap
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-reflink
> >
> 
> --
> chandan

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Chandan Babu R]

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@gmail.com>

-- 
chandan

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Eryu Guan]

On Tue, Jul 27, 2021 at 11:37:34AM -0700, 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?

Do you have plan to post formal patch? I think both problems could be
fixed in one patch like you did. I'll leave patch 1 for now.

Thanks,
Eryu

> 
> --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"
> +
>  echo "Consume free space"
>  fillerdir=$SCRATCH_MNT/fillerdir
>  nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> 
> > >
> > > Eventually, the plan is to support rmap[1] and reflink[2] on the
> > > realtime device, at which point this will have to be torn out and a
> > > better solution found.
> > >
> > > --D
> > >
> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-rmap
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=realtime-reflink
> > >
> > 
> > --
> > chandan

Re: [PATCH 3/3] xfs/530: Bail out if either of reflink or rmapbt is enabled

[Eryu Guan]

On Sun, Aug 01, 2021 at 07:41:09PM +0800, Eryu Guan wrote:
> On Tue, Jul 27, 2021 at 11:37:34AM -0700, 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?
> 
> Do you have plan to post formal patch? I think both problems could be
> fixed in one patch like you did. I'll leave patch 1 for now.

Ah, I saw the patch now, thanks!

Eryu

Re: [PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs

[Darrick J. Wong]

On Mon, Jul 26, 2021 at 12:13:11PM +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 block size argument passed by xfs/530 to _scratch_mkfs() will cause
> mkfs.xfs to fail if $MKFS_OPTIONS also has a block size specified. 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 removes the block size option
> that was being explicitly passed to _scratch_mkfs().

Yes, that makes sense.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  tests/xfs/530 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/xfs/530 b/tests/xfs/530
> index 4d168ac5..16dc426c 100755
> --- a/tests/xfs/530
> +++ b/tests/xfs/530
> @@ -60,7 +60,7 @@ 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"
>  
> -- 
> 2.30.2
> 

Re: [PATCH 1/3] xfs/530: Do not pass block size argument to _scratch_mkfs

[Eryu Guan]

On Mon, Jul 26, 2021 at 10:08:27AM -0700, Darrick J. Wong wrote:
> On Mon, Jul 26, 2021 at 12:13:11PM +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 block size argument passed by xfs/530 to _scratch_mkfs() will cause
> > mkfs.xfs to fail if $MKFS_OPTIONS also has a block size specified. 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 removes the block size option
> > that was being explicitly passed to _scratch_mkfs().

Patch looks fine to me, and queued for update.

> 
> Yes, that makes sense.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

I'm wondering if mkfs.xfs could restore the behavior that allows
re-specified options, and the last specified wins, e.g.

mkfs -t xfs -b 1k -b 2k -f /dev/sda2

and mkfs.xfs didn't report failure but created xfs with 2k blocksize.
This may cause less problems like this patch resolved.

Thanks,
Eryu

> 
> --D
> 
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  tests/xfs/530 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/xfs/530 b/tests/xfs/530
> > index 4d168ac5..16dc426c 100755
> > --- a/tests/xfs/530
> > +++ b/tests/xfs/530
> > @@ -60,7 +60,7 @@ 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"
> >  
> > -- 
> > 2.30.2
> >