| Message ID | 20241031193552.1171855-1-zlang@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xfs/157: mkfs does not need a specific fssize | expand |
On Fri, Nov 01, 2024 at 03:35:52AM +0800, Zorro Lang wrote: > The xfs/157 doesn't need to do a "sized" mkfs, the image file is > 500MiB, don't need to do _scratch_mkfs_sized with a 500MiB fssize > argument, a general _scratch_mkfs is good enough. > > Besides that, if we do: > > MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size > > the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs fails > with incompatible $MKFS_OPTIONS options, likes this: > > ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 ** > ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 ** > > But if we do: > > _scratch_mkfs -L oldlabel > > the _scratch_mkfs trys to keep the "-L oldlabel", when mkfs fails > with incompatible $MKFS_OPTIONS options, likes this: > > ** mkfs failed with extra mkfs options added to "-m rmapbt=1" by test 157 ** > ** attempting to mkfs using only test 157 options: -L oldlabel ** > > that's actually what we need. > > Signed-off-by: Zorro Lang <zlang@kernel.org> > --- > > This test started to fail since 2f7e1b8a6f09 ("xfs/157,xfs/547,xfs/548: switch to > using _scratch_mkfs_sized") was merged. > > FSTYP -- xfs (non-debug) > PLATFORM -- Linux/x86_64 > MKFS_OPTIONS -- -f -m rmapbt=1 /dev/sda3 > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch > > xfs/157 7s ... - output mismatch (see /root/git/xfstests/results//xfs/157.out.bad) > --- tests/xfs/157.out 2024-11-01 01:05:03.664543576 +0800 > +++ /root/git/xfstests/results//xfs/157.out.bad 2024-11-01 02:56:47.994007900 +0800 > @@ -6,10 +6,10 @@ > label = "oldlabel" > label = "newlabel" > S3: Check that setting with rtdev works > -label = "oldlabel" > +label = "" > label = "newlabel" > S4: Check that setting with rtdev + logdev works > ... > (Run 'diff -u /root/git/xfstests/tests/xfs/157.out /root/git/xfstests/results//xfs/157.out.bad' to see the entire diff) > Ran: xfs/157 > Failures: xfs/157 > Failed 1 of 1 tests > > Before that change, the _scratch_mkfs can drop "rmapbt=1" option from $MKFS_OPTIONS, > only keep the "-L label" option. That's why this test never failed before. > > Now it fails on xfs, if MKFS_OPTIONS contains "-m rmapbt=1", the reason as I > explained above. > > Thanks, > Zorro > > tests/xfs/157 | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/tests/xfs/157 b/tests/xfs/157 > index 9b5badbae..459c6de7c 100755 > --- a/tests/xfs/157 > +++ b/tests/xfs/157 > @@ -66,8 +66,7 @@ scenario() { > } > > check_label() { > - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ > - >> $seqres.full > + _scratch_mkfs -L oldlabel >> $seqres.full 2>&1 Hans Holmberg discovered that this mkfs fails if the SCRATCH_RTDEV is very large and SCRATCH_DEV is set to the 500M fake_datafile because the rtbitmap is larger than the datadev. I wonder if there's a way to pass the -L argument through in the "attempting to mkfs using only" case? --D > _scratch_xfs_db -c label > _scratch_xfs_admin -L newlabel "$@" >> $seqres.full > _scratch_xfs_db -c label > -- > 2.45.2 > >
On Thu, Oct 31, 2024 at 03:08:21PM -0700, Darrick J. Wong wrote: > On Fri, Nov 01, 2024 at 03:35:52AM +0800, Zorro Lang wrote: > > The xfs/157 doesn't need to do a "sized" mkfs, the image file is > > 500MiB, don't need to do _scratch_mkfs_sized with a 500MiB fssize > > argument, a general _scratch_mkfs is good enough. > > > > Besides that, if we do: > > > > MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size > > > > the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs fails > > with incompatible $MKFS_OPTIONS options, likes this: > > > > ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 ** > > ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 ** > > > > But if we do: > > > > _scratch_mkfs -L oldlabel > > > > the _scratch_mkfs trys to keep the "-L oldlabel", when mkfs fails > > with incompatible $MKFS_OPTIONS options, likes this: > > > > ** mkfs failed with extra mkfs options added to "-m rmapbt=1" by test 157 ** > > ** attempting to mkfs using only test 157 options: -L oldlabel ** > > > > that's actually what we need. > > > > Signed-off-by: Zorro Lang <zlang@kernel.org> > > --- > > > > This test started to fail since 2f7e1b8a6f09 ("xfs/157,xfs/547,xfs/548: switch to > > using _scratch_mkfs_sized") was merged. > > > > FSTYP -- xfs (non-debug) > > PLATFORM -- Linux/x86_64 > > MKFS_OPTIONS -- -f -m rmapbt=1 /dev/sda3 > > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch > > > > xfs/157 7s ... - output mismatch (see /root/git/xfstests/results//xfs/157.out.bad) > > --- tests/xfs/157.out 2024-11-01 01:05:03.664543576 +0800 > > +++ /root/git/xfstests/results//xfs/157.out.bad 2024-11-01 02:56:47.994007900 +0800 > > @@ -6,10 +6,10 @@ > > label = "oldlabel" > > label = "newlabel" > > S3: Check that setting with rtdev works > > -label = "oldlabel" > > +label = "" > > label = "newlabel" > > S4: Check that setting with rtdev + logdev works > > ... > > (Run 'diff -u /root/git/xfstests/tests/xfs/157.out /root/git/xfstests/results//xfs/157.out.bad' to see the entire diff) > > Ran: xfs/157 > > Failures: xfs/157 > > Failed 1 of 1 tests > > > > Before that change, the _scratch_mkfs can drop "rmapbt=1" option from $MKFS_OPTIONS, > > only keep the "-L label" option. That's why this test never failed before. > > > > Now it fails on xfs, if MKFS_OPTIONS contains "-m rmapbt=1", the reason as I > > explained above. > > > > Thanks, > > Zorro > > > > tests/xfs/157 | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/tests/xfs/157 b/tests/xfs/157 > > index 9b5badbae..459c6de7c 100755 > > --- a/tests/xfs/157 > > +++ b/tests/xfs/157 > > @@ -66,8 +66,7 @@ scenario() { > > } > > > > check_label() { > > - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ > > - >> $seqres.full > > + _scratch_mkfs -L oldlabel >> $seqres.full 2>&1 > > Hans Holmberg discovered that this mkfs fails if the SCRATCH_RTDEV is > very large and SCRATCH_DEV is set to the 500M fake_datafile because the > rtbitmap is larger than the datadev. > > I wonder if there's a way to pass the -L argument through in the > "attempting to mkfs using only" case? As I know mkfs.xfs can disable rmapbt automatically if "-r rtdevt=xxx" is used. How about unset the MKFS_OPTIONS for this test? As it already tests rtdev and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? Any better idea? Thanks, Zorro > > --D > > > _scratch_xfs_db -c label > > _scratch_xfs_admin -L newlabel "$@" >> $seqres.full > > _scratch_xfs_db -c label > > -- > > 2.45.2 > > > > >
On Fri, Nov 01, 2024 at 01:48:10PM +0800, Zorro Lang wrote: > On Thu, Oct 31, 2024 at 03:08:21PM -0700, Darrick J. Wong wrote: > > On Fri, Nov 01, 2024 at 03:35:52AM +0800, Zorro Lang wrote: > > > The xfs/157 doesn't need to do a "sized" mkfs, the image file is > > > 500MiB, don't need to do _scratch_mkfs_sized with a 500MiB fssize > > > argument, a general _scratch_mkfs is good enough. > > > > > > Besides that, if we do: > > > > > > MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size > > > > > > the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs fails > > > with incompatible $MKFS_OPTIONS options, likes this: > > > > > > ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 ** > > > ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 ** > > > > > > But if we do: > > > > > > _scratch_mkfs -L oldlabel > > > > > > the _scratch_mkfs trys to keep the "-L oldlabel", when mkfs fails > > > with incompatible $MKFS_OPTIONS options, likes this: > > > > > > ** mkfs failed with extra mkfs options added to "-m rmapbt=1" by test 157 ** > > > ** attempting to mkfs using only test 157 options: -L oldlabel ** > > > > > > that's actually what we need. > > > > > > Signed-off-by: Zorro Lang <zlang@kernel.org> > > > --- > > > > > > This test started to fail since 2f7e1b8a6f09 ("xfs/157,xfs/547,xfs/548: switch to > > > using _scratch_mkfs_sized") was merged. > > > > > > FSTYP -- xfs (non-debug) > > > PLATFORM -- Linux/x86_64 > > > MKFS_OPTIONS -- -f -m rmapbt=1 /dev/sda3 > > > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch > > > > > > xfs/157 7s ... - output mismatch (see /root/git/xfstests/results//xfs/157.out.bad) > > > --- tests/xfs/157.out 2024-11-01 01:05:03.664543576 +0800 > > > +++ /root/git/xfstests/results//xfs/157.out.bad 2024-11-01 02:56:47.994007900 +0800 > > > @@ -6,10 +6,10 @@ > > > label = "oldlabel" > > > label = "newlabel" > > > S3: Check that setting with rtdev works > > > -label = "oldlabel" > > > +label = "" > > > label = "newlabel" > > > S4: Check that setting with rtdev + logdev works > > > ... > > > (Run 'diff -u /root/git/xfstests/tests/xfs/157.out /root/git/xfstests/results//xfs/157.out.bad' to see the entire diff) > > > Ran: xfs/157 > > > Failures: xfs/157 > > > Failed 1 of 1 tests > > > > > > Before that change, the _scratch_mkfs can drop "rmapbt=1" option from $MKFS_OPTIONS, > > > only keep the "-L label" option. That's why this test never failed before. > > > > > > Now it fails on xfs, if MKFS_OPTIONS contains "-m rmapbt=1", the reason as I > > > explained above. > > > > > > Thanks, > > > Zorro > > > > > > tests/xfs/157 | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/tests/xfs/157 b/tests/xfs/157 > > > index 9b5badbae..459c6de7c 100755 > > > --- a/tests/xfs/157 > > > +++ b/tests/xfs/157 > > > @@ -66,8 +66,7 @@ scenario() { > > > } > > > > > > check_label() { > > > - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ > > > - >> $seqres.full > > > + _scratch_mkfs -L oldlabel >> $seqres.full 2>&1 > > > > Hans Holmberg discovered that this mkfs fails if the SCRATCH_RTDEV is > > very large and SCRATCH_DEV is set to the 500M fake_datafile because the > > rtbitmap is larger than the datadev. > > > > I wonder if there's a way to pass the -L argument through in the > > "attempting to mkfs using only" case? > > As I know mkfs.xfs can disable rmapbt automatically if "-r rtdevt=xxx" is > used. That's not going to last forever, rmap support is coming for realtime, hopefully for 6.14. > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? That will exclude quite a few configurations. Also, how many people actually turn on rmapbt explicitly now? > Any better idea? I'm afraid not. Maybe I should restructure the test to force the rt device to be 500MB even when we're not using the fake rtdev? --D > Thanks, > Zorro > > > > > --D > > > > > _scratch_xfs_db -c label > > > _scratch_xfs_admin -L newlabel "$@" >> $seqres.full > > > _scratch_xfs_db -c label > > > -- > > > 2.45.2 > > > > > > > > >
On Fri, Nov 01, 2024 at 02:49:26PM -0700, Darrick J. Wong wrote: > > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev > > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? > > That will exclude quite a few configurations. Also, how many people > actually turn on rmapbt explicitly now? > > > Any better idea? > > I'm afraid not. Maybe I should restructure the test to force the rt > device to be 500MB even when we're not using the fake rtdev? All of this is really just bandaids or the fundamental problem that: - we try to abitrarily mix config and test provided options without checking that they are compatible in general, and with what the test is trying to specifically - some combination of options and devices (size, block size, sequential required zoned) fundamentally can't work I haven't really found an easy solution for them. In the long run I suspect we need to split tests between those that just take the options from the config and are supposed to work with all options (maybe a few notruns that fundamentally can't work). And those that want to test specific mkfs/mount options and hard code them but don't take options from the input.
On Sun, Nov 03, 2024 at 11:50:32PM -0800, Christoph Hellwig wrote: > On Fri, Nov 01, 2024 at 02:49:26PM -0700, Darrick J. Wong wrote: > > > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev > > > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? > > > > That will exclude quite a few configurations. Also, how many people > > actually turn on rmapbt explicitly now? > > > > > Any better idea? > > > > I'm afraid not. Maybe I should restructure the test to force the rt > > device to be 500MB even when we're not using the fake rtdev? > > All of this is really just bandaids or the fundamental problem that: > > - we try to abitrarily mix config and test provided options without > checking that they are compatible in general, and with what the test > is trying to specifically > - some combination of options and devices (size, block size, sequential > required zoned) fundamentally can't work > > I haven't really found an easy solution for them. In the long run I > suspect we need to split tests between those that just take the options > from the config and are supposed to work with all options (maybe a few > notruns that fundamentally can't work). And those that want to test > specific mkfs/mount options and hard code them but don't take options > from the input. So how about unset extra MKFS_OPTIONS in this case ? This test has its own mkfs options (-L label and logdev and rtdev and fssize). Thanks, Zorro > >
On Mon, Nov 04, 2024 at 09:04:37PM +0800, Zorro Lang wrote: > On Sun, Nov 03, 2024 at 11:50:32PM -0800, Christoph Hellwig wrote: > > On Fri, Nov 01, 2024 at 02:49:26PM -0700, Darrick J. Wong wrote: > > > > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev > > > > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? > > > > > > That will exclude quite a few configurations. Also, how many people > > > actually turn on rmapbt explicitly now? > > > > > > > Any better idea? > > > > > > I'm afraid not. Maybe I should restructure the test to force the rt > > > device to be 500MB even when we're not using the fake rtdev? > > > > All of this is really just bandaids or the fundamental problem that: > > > > - we try to abitrarily mix config and test provided options without > > checking that they are compatible in general, and with what the test > > is trying to specifically > > - some combination of options and devices (size, block size, sequential > > required zoned) fundamentally can't work > > > > I haven't really found an easy solution for them. In the long run I > > suspect we need to split tests between those that just take the options > > from the config and are supposed to work with all options (maybe a few > > notruns that fundamentally can't work). And those that want to test > > specific mkfs/mount options and hard code them but don't take options > > from the input. > > So how about unset extra MKFS_OPTIONS in this case ? This test has its own > mkfs options (-L label and logdev and rtdev and fssize). The trouble with clearing MKFS_OPTIONS is that you then have to adjust the other _scratch_* calls in check_label(), and then all you're doing is reducing fs configuration test coverage. If (say) there was a bug when changing the fs label on a rtgroups filesystem with a rt section, you'd never see it. Hang on ... is this a general complaint about _scratch_mkfs_xfs dropping MKFS_OPTIONS in favor of the specific arguments passed to it by the test? Or a specific complaint about xfs/157 barfing when the test runner puts "-L foo" in the MKFS_OPTIONS that it passes to ./check? If it's the second, then let's do this: extract_mkfs_label() { set -- $MKFS_OPTIONS local in_l for arg in "$@"; do if [ "$in_l" = "1" ]; then echo "$arg" return 0 elif [ "$arg" = "-L" ]; then in_l=1 fi done return 1 } check_label() { local existing_label local filter # Handle -L somelabel being set in MKFS_OPTIONS if existing_label="$(extract_mkfs_label)"; then filter=(sed -e "s|$existing_label|oldlabel|g") _scratch_mkfs_sized $fs_size >> $seqres.full else filter=(cat) MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" \ _scratch_mkfs_sized $fs_size >> $seqres.full fi _scratch_xfs_db -c label | ${filter[@]} _scratch_xfs_admin -L newlabel "$@" >> $seqres.full _scratch_xfs_db -c label _scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?" } --D > Thanks, > Zorro > > > > > > >
On Mon, Nov 04, 2024 at 03:34:26PM -0800, Darrick J. Wong wrote: > On Mon, Nov 04, 2024 at 09:04:37PM +0800, Zorro Lang wrote: > > On Sun, Nov 03, 2024 at 11:50:32PM -0800, Christoph Hellwig wrote: > > > On Fri, Nov 01, 2024 at 02:49:26PM -0700, Darrick J. Wong wrote: > > > > > How about unset the MKFS_OPTIONS for this test? As it already tests rtdev > > > > > and logdev by itself. Or call _notrun if MKFS_OPTIONS has "rmapbt=1"? > > > > > > > > That will exclude quite a few configurations. Also, how many people > > > > actually turn on rmapbt explicitly now? > > > > > > > > > Any better idea? > > > > > > > > I'm afraid not. Maybe I should restructure the test to force the rt > > > > device to be 500MB even when we're not using the fake rtdev? > > > > > > All of this is really just bandaids or the fundamental problem that: > > > > > > - we try to abitrarily mix config and test provided options without > > > checking that they are compatible in general, and with what the test > > > is trying to specifically > > > - some combination of options and devices (size, block size, sequential > > > required zoned) fundamentally can't work > > > > > > I haven't really found an easy solution for them. In the long run I > > > suspect we need to split tests between those that just take the options > > > from the config and are supposed to work with all options (maybe a few > > > notruns that fundamentally can't work). And those that want to test > > > specific mkfs/mount options and hard code them but don't take options > > > from the input. > > > > So how about unset extra MKFS_OPTIONS in this case ? This test has its own > > mkfs options (-L label and logdev and rtdev and fssize). > > The trouble with clearing MKFS_OPTIONS is that you then have to adjust > the other _scratch_* calls in check_label(), and then all you're doing > is reducing fs configuration test coverage. If (say) there was a bug > when changing the fs label on a rtgroups filesystem with a rt section, > you'd never see it. Nobody need to overload MKFS_OPTIONS or unset it. Local test configs are supposed to be passed as function parameters, whilst MKFS_OPTIONS defines the global defaults. When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS and uses only the local parameters so the filesystem is set up with the configuration the test expects. In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the local RTDEV/USE_EXTERNAL test setup. Because the test icurrently overloads the global MKFS_OPTIONS with local test options, the local test parameters are dropped along with the global paramters when there is a conflict. Hence the mkfs_scratch call fails to set the filesystem up the way the test expects. IOWs, Zorro's fix is correct and the right one to make - it fixes the failures here on all the config sections I have that have configs that aren't compatible with RT devices. -Dave.
On Fri, Nov 01, 2024 at 03:35:52AM +0800, Zorro Lang wrote: > The xfs/157 doesn't need to do a "sized" mkfs, the image file is > 500MiB, don't need to do _scratch_mkfs_sized with a 500MiB fssize > argument, a general _scratch_mkfs is good enough. ..... > diff --git a/tests/xfs/157 b/tests/xfs/157 > index 9b5badbae..459c6de7c 100755 > --- a/tests/xfs/157 > +++ b/tests/xfs/157 > @@ -66,8 +66,7 @@ scenario() { > } > > check_label() { > - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ > - >> $seqres.full > + _scratch_mkfs -L oldlabel >> $seqres.full 2>&1 > _scratch_xfs_db -c label > _scratch_xfs_admin -L newlabel "$@" >> $seqres.full > _scratch_xfs_db -c label Looks good, fixes the failure I'm getting here. Reviewed-by: Dave Chinner <dchinner@redhat.com> -Dave.
On Tue, Nov 05, 2024 at 05:58:03PM +1100, Dave Chinner wrote: > When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS > and uses only the local parameters so the filesystem is set up with > the configuration the test expects. > > In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the > local RTDEV/USE_EXTERNAL test setup. Because the test icurrently > overloads the global MKFS_OPTIONS with local test options, the local > test parameters are dropped along with the global paramters when > there is a conflict. Hence the mkfs_scratch call fails to set the > filesystem up the way the test expects. But the rmapbt can be default on, in which case it does not get removed. And then without the _sized we'll run into the problem that Hans' patches fixed once again.
On Tue, Nov 05, 2024 at 07:02:26AM -0800, Christoph Hellwig wrote: > On Tue, Nov 05, 2024 at 05:58:03PM +1100, Dave Chinner wrote: > > When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS > > and uses only the local parameters so the filesystem is set up with > > the configuration the test expects. > > > > In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the > > local RTDEV/USE_EXTERNAL test setup. Because the test icurrently > > overloads the global MKFS_OPTIONS with local test options, the local > > test parameters are dropped along with the global paramters when > > there is a conflict. Hence the mkfs_scratch call fails to set the > > filesystem up the way the test expects. > > But the rmapbt can be default on, in which case it does not get > removed. And then without the _sized we'll run into the problem that > Hans' patches fixed once again. Well we /could/ make _scratch_mkfs_sized pass options through to the underlying _scratch_mkfs. --D
On Tue, Nov 05, 2024 at 07:47:12AM -0800, Darrick J. Wong wrote: > On Tue, Nov 05, 2024 at 07:02:26AM -0800, Christoph Hellwig wrote: > > On Tue, Nov 05, 2024 at 05:58:03PM +1100, Dave Chinner wrote: > > > When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS > > > and uses only the local parameters so the filesystem is set up with > > > the configuration the test expects. > > > > > > In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the > > > local RTDEV/USE_EXTERNAL test setup. Because the test icurrently > > > overloads the global MKFS_OPTIONS with local test options, the local > > > test parameters are dropped along with the global paramters when > > > there is a conflict. Hence the mkfs_scratch call fails to set the > > > filesystem up the way the test expects. > > > > But the rmapbt can be default on, in which case it does not get > > removed. And then without the _sized we'll run into the problem that > > Hans' patches fixed once again. > > Well we /could/ make _scratch_mkfs_sized pass options through to the > underlying _scratch_mkfs. That seems like the right thing to do to me. -Dave.
On Thu, Nov 07, 2024 at 04:40:34PM +1100, Dave Chinner wrote: > On Tue, Nov 05, 2024 at 07:47:12AM -0800, Darrick J. Wong wrote: > > On Tue, Nov 05, 2024 at 07:02:26AM -0800, Christoph Hellwig wrote: > > > On Tue, Nov 05, 2024 at 05:58:03PM +1100, Dave Chinner wrote: > > > > When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS > > > > and uses only the local parameters so the filesystem is set up with > > > > the configuration the test expects. > > > > > > > > In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the > > > > local RTDEV/USE_EXTERNAL test setup. Because the test icurrently > > > > overloads the global MKFS_OPTIONS with local test options, the local > > > > test parameters are dropped along with the global paramters when > > > > there is a conflict. Hence the mkfs_scratch call fails to set the > > > > filesystem up the way the test expects. > > > > > > But the rmapbt can be default on, in which case it does not get > > > removed. And then without the _sized we'll run into the problem that > > > Hans' patches fixed once again. > > > > Well we /could/ make _scratch_mkfs_sized pass options through to the > > underlying _scratch_mkfs. > > That seems like the right thing to do to me. OK, thanks for all of these suggestions, how about below (draft) change[1]. If it's good to all of you, I'll send another patch. Thanks, Zorro diff --git a/common/rc b/common/rc index 2af26f23f..673f056fb 100644 --- a/common/rc +++ b/common/rc @@ -1027,7 +1027,9 @@ _small_fs_size_mb() _try_scratch_mkfs_sized() { local fssize=$1 - local blocksize=$2 + shift + local blocksize=$1 + shift local def_blksz local blocksize_opt local rt_ops @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized() # don't override MKFS_OPTIONS that set a block size. echo $MKFS_OPTIONS |grep -E -q "b\s*size=" if [ $? -eq 0 ]; then - _try_scratch_mkfs_xfs -d size=$fssize $rt_ops + _try_scratch_mkfs_xfs -d size=$fssize $rt_ops $@ else _try_scratch_mkfs_xfs -d size=$fssize $rt_ops \ - -b size=$blocksize + -b size=$blocksize $@ fi ;; ext2|ext3|ext4) @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized() _notrun "Could not make scratch logdev" MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV" fi - ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks + ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks ;; gfs2) # mkfs.gfs2 doesn't automatically shrink journal files on small @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized() (( journal_size >= min_journal_size )) || journal_size=$min_journal_size MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS" fi - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $@ $SCRATCH_DEV $blocks ;; ocfs2) - yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks + yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks ;; udf) - $MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks + $MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks ;; btrfs) local mixed_opt= @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized() # the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size() (( fssize < $((256 * 1024 * 1024)) )) && ! _scratch_btrfs_is_zoned && mixed_opt='--mixed' - $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV + $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $@ $SCRATCH_DEV ;; jfs) - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $@ $SCRATCH_DEV $blocks ;; reiserfs) - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks ;; reiser4) # mkfs.resier4 requires size in KB as input for creating filesystem - $MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \ + $MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $@ $SCRATCH_DEV \ `expr $fssize / 1024` ;; f2fs) # mkfs.f2fs requires # of sectors as an input for the size local sector_size=`blockdev --getss $SCRATCH_DEV` - $MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size` + $MKFS_F2FS_PROG $MKFS_OPTIONS $@ $SCRATCH_DEV `expr $fssize / $sector_size` ;; tmpfs) local free_mem=`_free_memory_bytes` if [ "$free_mem" -lt "$fssize" ] ; then _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes" fi - export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS" + export MOUNT_OPTIONS="-o size=$fssize $@ $TMPFS_MOUNT_OPTIONS" ;; bcachefs) - $MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV + $MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $@ $SCRATCH_DEV ;; *) _notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized" @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized() _scratch_mkfs_sized() { - _try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)" + _try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)" } # Emulate an N-data-disk stripe w/ various stripe units diff --git a/tests/xfs/157 b/tests/xfs/157 index 9b5badbae..f8f102d78 100755 --- a/tests/xfs/157 +++ b/tests/xfs/157 @@ -66,8 +66,7 @@ scenario() { } check_label() { - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ - >> $seqres.full + _scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1 _scratch_xfs_db -c label _scratch_xfs_admin -L newlabel "$@" >> $seqres.full _scratch_xfs_db -c label > > -Dave. > -- > Dave Chinner > david@fromorbit.com >
On Thu, Nov 07, 2024 at 06:10:11PM +0800, Zorro Lang wrote: > On Thu, Nov 07, 2024 at 04:40:34PM +1100, Dave Chinner wrote: > > On Tue, Nov 05, 2024 at 07:47:12AM -0800, Darrick J. Wong wrote: > > > On Tue, Nov 05, 2024 at 07:02:26AM -0800, Christoph Hellwig wrote: > > > > On Tue, Nov 05, 2024 at 05:58:03PM +1100, Dave Chinner wrote: > > > > > When the two conflict, _scratch_mkfs drops the global MKFS_OPTIONS > > > > > and uses only the local parameters so the filesystem is set up with > > > > > the configuration the test expects. > > > > > > > > > > In this case, MKFS_OPTIONS="-m rmapbt=1" which conflicts with the > > > > > local RTDEV/USE_EXTERNAL test setup. Because the test icurrently > > > > > overloads the global MKFS_OPTIONS with local test options, the local > > > > > test parameters are dropped along with the global paramters when > > > > > there is a conflict. Hence the mkfs_scratch call fails to set the > > > > > filesystem up the way the test expects. > > > > > > > > But the rmapbt can be default on, in which case it does not get > > > > removed. And then without the _sized we'll run into the problem that > > > > Hans' patches fixed once again. > > > > > > Well we /could/ make _scratch_mkfs_sized pass options through to the > > > underlying _scratch_mkfs. > > > > That seems like the right thing to do to me. > > OK, thanks for all of these suggestions, how about below (draft) change[1]. > If it's good to all of you, I'll send another patch. > > Thanks, > Zorro > > diff --git a/common/rc b/common/rc > index 2af26f23f..673f056fb 100644 > --- a/common/rc > +++ b/common/rc > @@ -1027,7 +1027,9 @@ _small_fs_size_mb() > _try_scratch_mkfs_sized() > { > local fssize=$1 > - local blocksize=$2 > + shift > + local blocksize=$1 > + shift > local def_blksz > local blocksize_opt > local rt_ops > @@ -1091,10 +1093,10 @@ _try_scratch_mkfs_sized() > # don't override MKFS_OPTIONS that set a block size. > echo $MKFS_OPTIONS |grep -E -q "b\s*size=" > if [ $? -eq 0 ]; then > - _try_scratch_mkfs_xfs -d size=$fssize $rt_ops > + _try_scratch_mkfs_xfs -d size=$fssize $rt_ops $@ > else > _try_scratch_mkfs_xfs -d size=$fssize $rt_ops \ > - -b size=$blocksize > + -b size=$blocksize $@ > fi > ;; > ext2|ext3|ext4) > @@ -1105,7 +1107,7 @@ _try_scratch_mkfs_sized() > _notrun "Could not make scratch logdev" > MKFS_OPTIONS="$MKFS_OPTIONS -J device=$SCRATCH_LOGDEV" > fi > - ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks > + ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks > ;; > gfs2) > # mkfs.gfs2 doesn't automatically shrink journal files on small > @@ -1120,13 +1122,13 @@ _try_scratch_mkfs_sized() > (( journal_size >= min_journal_size )) || journal_size=$min_journal_size > MKFS_OPTIONS="-J $journal_size $MKFS_OPTIONS" > fi > - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $SCRATCH_DEV $blocks > + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -O -b $blocksize $@ $SCRATCH_DEV $blocks > ;; > ocfs2) > - yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks > + yes | ${MKFS_PROG} -t $FSTYP -F $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks > ;; > udf) > - $MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks > + $MKFS_UDF_PROG $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks > ;; > btrfs) > local mixed_opt= > @@ -1134,33 +1136,33 @@ _try_scratch_mkfs_sized() > # the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size() > (( fssize < $((256 * 1024 * 1024)) )) && > ! _scratch_btrfs_is_zoned && mixed_opt='--mixed' > - $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV > + $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $@ $SCRATCH_DEV > ;; > jfs) > - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $SCRATCH_DEV $blocks > + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS $@ $SCRATCH_DEV $blocks > ;; > reiserfs) > - ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $SCRATCH_DEV $blocks > + ${MKFS_PROG} -t $FSTYP $MKFS_OPTIONS -b $blocksize $@ $SCRATCH_DEV $blocks > ;; > reiser4) > # mkfs.resier4 requires size in KB as input for creating filesystem > - $MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $SCRATCH_DEV \ > + $MKFS_REISER4_PROG $MKFS_OPTIONS -y -b $blocksize $@ $SCRATCH_DEV \ > `expr $fssize / 1024` > ;; > f2fs) > # mkfs.f2fs requires # of sectors as an input for the size > local sector_size=`blockdev --getss $SCRATCH_DEV` > - $MKFS_F2FS_PROG $MKFS_OPTIONS $SCRATCH_DEV `expr $fssize / $sector_size` > + $MKFS_F2FS_PROG $MKFS_OPTIONS $@ $SCRATCH_DEV `expr $fssize / $sector_size` > ;; > tmpfs) > local free_mem=`_free_memory_bytes` > if [ "$free_mem" -lt "$fssize" ] ; then > _notrun "Not enough memory ($free_mem) for tmpfs with $fssize bytes" > fi > - export MOUNT_OPTIONS="-o size=$fssize $TMPFS_MOUNT_OPTIONS" > + export MOUNT_OPTIONS="-o size=$fssize $@ $TMPFS_MOUNT_OPTIONS" > ;; > bcachefs) > - $MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $SCRATCH_DEV > + $MKFS_BCACHEFS_PROG $MKFS_OPTIONS --fs_size=$fssize $blocksize_opt $@ $SCRATCH_DEV > ;; > *) > _notrun "Filesystem $FSTYP not supported in _scratch_mkfs_sized" > @@ -1170,7 +1172,7 @@ _try_scratch_mkfs_sized() > > _scratch_mkfs_sized() > { > - _try_scratch_mkfs_sized $* || _notrun "_scratch_mkfs_sized failed with ($*)" > + _try_scratch_mkfs_sized "$@" || _notrun "_scratch_mkfs_sized failed with ($@)" > } > > # Emulate an N-data-disk stripe w/ various stripe units > diff --git a/tests/xfs/157 b/tests/xfs/157 > index 9b5badbae..f8f102d78 100755 > --- a/tests/xfs/157 > +++ b/tests/xfs/157 > @@ -66,8 +66,7 @@ scenario() { > } > > check_label() { > - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ > - >> $seqres.full > + _scratch_mkfs_sized "$fs_size" "" "-L oldlabel" >> $seqres.full 2>&1 > _scratch_xfs_db -c label > _scratch_xfs_admin -L newlabel "$@" >> $seqres.full > _scratch_xfs_db -c label Looks good to me, though you probably still need to fix the double -L case: From: Darrick J. Wong <djwong@kernel.org> Subject: [PATCH] xfs/157: fix test failures when MKFS_OPTIONS has -L options Zorro reports that this test now fails if the test runner set an -L (label) option in MKFS_OPTIONS. Fix the test to work around this with a bunch of horrid sed filtering magic. Cc: <fstests@vger.kernel.org> # v2024.10.14 Fixes: 2f7e1b8a6f09b6 ("xfs/157,xfs/547,xfs/548: switch to using _scratch_mkfs_sized") --- tests/xfs/157 | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/tests/xfs/157 b/tests/xfs/157 index 9b5badbaeb3c76..30f8cc870b9af2 100755 --- a/tests/xfs/157 +++ b/tests/xfs/157 @@ -65,10 +65,35 @@ scenario() { SCRATCH_RTDEV=$orig_rtdev } +extract_mkfs_label() { + set -- $MKFS_OPTIONS + local in_l + + for arg in "$@"; do + if [ "$in_l" = "1" ]; then + echo "$arg" + return 0 + elif [ "$arg" = "-L" ]; then + in_l=1 + fi + done + return 1 +} + check_label() { - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ - >> $seqres.full - _scratch_xfs_db -c label + local existing_label + local filter + + # Handle -L somelabel being set in MKFS_OPTIONS + if existing_label="$(extract_mkfs_label)"; then + filter=(sed -e "s|$existing_label|oldlabel|g") + _scratch_mkfs_sized $fs_size >> $seqres.full + else + filter=(cat) + MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" \ + _scratch_mkfs_sized $fs_size >> $seqres.full + fi + _scratch_xfs_db -c label | "${filter[@]}" _scratch_xfs_admin -L newlabel "$@" >> $seqres.full _scratch_xfs_db -c label _scratch_xfs_repair -n &>> $seqres.full || echo "Check failed?"
diff --git a/tests/xfs/157 b/tests/xfs/157 index 9b5badbae..459c6de7c 100755 --- a/tests/xfs/157 +++ b/tests/xfs/157 @@ -66,8 +66,7 @@ scenario() { } check_label() { - MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size \ - >> $seqres.full + _scratch_mkfs -L oldlabel >> $seqres.full 2>&1 _scratch_xfs_db -c label _scratch_xfs_admin -L newlabel "$@" >> $seqres.full _scratch_xfs_db -c label
The xfs/157 doesn't need to do a "sized" mkfs, the image file is 500MiB, don't need to do _scratch_mkfs_sized with a 500MiB fssize argument, a general _scratch_mkfs is good enough. Besides that, if we do: MKFS_OPTIONS="-L oldlabel $MKFS_OPTIONS" _scratch_mkfs_sized $fs_size the _scratch_mkfs_sized trys to keep the $fs_size, when mkfs fails with incompatible $MKFS_OPTIONS options, likes this: ** mkfs failed with extra mkfs options added to "-L oldlabel -m rmapbt=1" by test 157 ** ** attempting to mkfs using only test 157 options: -d size=524288000 -b size=4096 ** But if we do: _scratch_mkfs -L oldlabel the _scratch_mkfs trys to keep the "-L oldlabel", when mkfs fails with incompatible $MKFS_OPTIONS options, likes this: ** mkfs failed with extra mkfs options added to "-m rmapbt=1" by test 157 ** ** attempting to mkfs using only test 157 options: -L oldlabel ** that's actually what we need. Signed-off-by: Zorro Lang <zlang@kernel.org> --- This test started to fail since 2f7e1b8a6f09 ("xfs/157,xfs/547,xfs/548: switch to using _scratch_mkfs_sized") was merged. FSTYP -- xfs (non-debug) PLATFORM -- Linux/x86_64 MKFS_OPTIONS -- -f -m rmapbt=1 /dev/sda3 MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/sda3 /mnt/scratch xfs/157 7s ... - output mismatch (see /root/git/xfstests/results//xfs/157.out.bad) --- tests/xfs/157.out 2024-11-01 01:05:03.664543576 +0800 +++ /root/git/xfstests/results//xfs/157.out.bad 2024-11-01 02:56:47.994007900 +0800 @@ -6,10 +6,10 @@ label = "oldlabel" label = "newlabel" S3: Check that setting with rtdev works -label = "oldlabel" +label = "" label = "newlabel" S4: Check that setting with rtdev + logdev works ... (Run 'diff -u /root/git/xfstests/tests/xfs/157.out /root/git/xfstests/results//xfs/157.out.bad' to see the entire diff) Ran: xfs/157 Failures: xfs/157 Failed 1 of 1 tests Before that change, the _scratch_mkfs can drop "rmapbt=1" option from $MKFS_OPTIONS, only keep the "-L label" option. That's why this test never failed before. Now it fails on xfs, if MKFS_OPTIONS contains "-m rmapbt=1", the reason as I explained above. Thanks, Zorro tests/xfs/157 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)