| Message ID | 20210726064313.19153-3-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [1/3] xfs/530: Do not pass block size argument to _scratch_mkfs | expand |
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 >
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
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
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>
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
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
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"
_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(+)