| Message ID | 20230307145517.1359-3-pvorel@suse.cz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | df01.sh: fix for XFS on kernel >= 5.19 | expand |
Hi! > XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze > implementation to force all the background garbage collection to run > to completion. > > Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ > Suggested-by: Eric Sandeen <sandeen@redhat.com> > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > testcases/commands/df/df01.sh | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh > index ae0449c3c..699d1538f 100755 > --- a/testcases/commands/df/df01.sh > +++ b/testcases/commands/df/df01.sh > @@ -1,7 +1,7 @@ > #!/bin/sh > # SPDX-License-Identifier: GPL-2.0-or-later > # Copyright (c) 2015 Fujitsu Ltd. > -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> > +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> > # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> > # > # Test df command with some basic options. > @@ -46,6 +46,11 @@ df_test() > > ROD_SILENT rm -rf $TST_MNTPOINT/testimg > > + # force all the background garbage collection to run to completion > + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then > + tst_fsfreeze $TST_MNTPOINT > + fi This looks overly specific, can't we just freeze and unfreeze the FS without looking at kernel version? Or will we get errors on older XFS? I suppose that this may still start to fail on distribution kernels if some of the newer functionality gets backported... Otherwise it looks good.
On 3/13/23 9:38 AM, Cyril Hrubis wrote: > Hi! >> XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze >> implementation to force all the background garbage collection to run >> to completion. >> >> Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ >> Suggested-by: Eric Sandeen <sandeen@redhat.com> >> Signed-off-by: Petr Vorel <pvorel@suse.cz> >> --- >> testcases/commands/df/df01.sh | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh >> index ae0449c3c..699d1538f 100755 >> --- a/testcases/commands/df/df01.sh >> +++ b/testcases/commands/df/df01.sh >> @@ -1,7 +1,7 @@ >> #!/bin/sh >> # SPDX-License-Identifier: GPL-2.0-or-later >> # Copyright (c) 2015 Fujitsu Ltd. >> -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> >> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> >> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> >> # >> # Test df command with some basic options. >> @@ -46,6 +46,11 @@ df_test() >> >> ROD_SILENT rm -rf $TST_MNTPOINT/testimg >> >> + # force all the background garbage collection to run to completion >> + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then >> + tst_fsfreeze $TST_MNTPOINT >> + fi > > This looks overly specific, can't we just freeze and unfreeze the FS > without looking at kernel version? Or will we get errors on older XFS? > > I suppose that this may still start to fail on distribution kernels if > some of the newer functionality gets backported... Yup, I agree. Freeze should be safe for any kernel, I wouldn't condition it either. (You do want to be very sure that you're not freezing the root fs, tho, if that is any possibility.) -Eric > Otherwise it looks good. >
> On 3/13/23 9:38 AM, Cyril Hrubis wrote: > > Hi! > >> XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze > >> implementation to force all the background garbage collection to run > >> to completion. > >> Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ > >> Suggested-by: Eric Sandeen <sandeen@redhat.com> > >> Signed-off-by: Petr Vorel <pvorel@suse.cz> > >> --- > >> testcases/commands/df/df01.sh | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh > >> index ae0449c3c..699d1538f 100755 > >> --- a/testcases/commands/df/df01.sh > >> +++ b/testcases/commands/df/df01.sh > >> @@ -1,7 +1,7 @@ > >> #!/bin/sh > >> # SPDX-License-Identifier: GPL-2.0-or-later > >> # Copyright (c) 2015 Fujitsu Ltd. > >> -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> > >> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> > >> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> > >> # Test df command with some basic options. > >> @@ -46,6 +46,11 @@ df_test() > >> ROD_SILENT rm -rf $TST_MNTPOINT/testimg > >> + # force all the background garbage collection to run to completion > >> + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then > >> + tst_fsfreeze $TST_MNTPOINT > >> + fi > > This looks overly specific, can't we just freeze and unfreeze the FS > > without looking at kernel version? Or will we get errors on older XFS? > > I suppose that this may still start to fail on distribution kernels if > > some of the newer functionality gets backported... So far it's OK on SLES, likely nothing related has been backported to it. I wonder if we should remove the check or just wait till first complaint. > Yup, I agree. Freeze should be safe for any kernel, I wouldn't condition it either. > (You do want to be very sure that you're not freezing the root fs, tho, > if that is any possibility.) $TST_MNTPOINT in on $TMPDIR, which is by default /tmp. In case of /tmp being on root fs we're freezing root tmpfs. But it works on openSUSE, which found the problem. Kind regards, Petr > -Eric > > Otherwise it looks good.
> > On 3/13/23 9:38 AM, Cyril Hrubis wrote: > > > Hi! > > >> XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze > > >> implementation to force all the background garbage collection to run > > >> to completion. > > >> Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ > > >> Suggested-by: Eric Sandeen <sandeen@redhat.com> > > >> Signed-off-by: Petr Vorel <pvorel@suse.cz> > > >> --- > > >> testcases/commands/df/df01.sh | 7 ++++++- > > >> 1 file changed, 6 insertions(+), 1 deletion(-) > > >> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh > > >> index ae0449c3c..699d1538f 100755 > > >> --- a/testcases/commands/df/df01.sh > > >> +++ b/testcases/commands/df/df01.sh > > >> @@ -1,7 +1,7 @@ > > >> #!/bin/sh > > >> # SPDX-License-Identifier: GPL-2.0-or-later > > >> # Copyright (c) 2015 Fujitsu Ltd. > > >> -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> > > >> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> > > >> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> > > >> # Test df command with some basic options. > > >> @@ -46,6 +46,11 @@ df_test() > > >> ROD_SILENT rm -rf $TST_MNTPOINT/testimg > > >> + # force all the background garbage collection to run to completion > > >> + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then > > >> + tst_fsfreeze $TST_MNTPOINT > > >> + fi > > > This looks overly specific, can't we just freeze and unfreeze the FS > > > without looking at kernel version? Or will we get errors on older XFS? > > > I suppose that this may still start to fail on distribution kernels if > > > some of the newer functionality gets backported... > So far it's OK on SLES, likely nothing related has been backported to it. > I wonder if we should remove the check or just wait till first complaint. Well, I haven't seen any problems with older kernels, but I'll retest it more. I don't expect any problems, thus I'm OK with removing it. Kind regards, Petr > > Yup, I agree. Freeze should be safe for any kernel, I wouldn't condition it either. > > (You do want to be very sure that you're not freezing the root fs, tho, > > if that is any possibility.) > $TST_MNTPOINT in on $TMPDIR, which is by default /tmp. In case of /tmp being on > root fs we're freezing root tmpfs. But it works on openSUSE, which found the > problem. > Kind regards, > Petr > > -Eric > > > Otherwise it looks good.
On Mon, Mar 13, 2023 at 04:33:09PM +0100, Petr Vorel wrote: > > > On 3/13/23 9:38 AM, Cyril Hrubis wrote: > > > > Hi! > > > >> XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze > > > >> implementation to force all the background garbage collection to run > > > >> to completion. > > > > >> Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ > > > >> Suggested-by: Eric Sandeen <sandeen@redhat.com> > > > >> Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > >> --- > > > >> testcases/commands/df/df01.sh | 7 ++++++- > > > >> 1 file changed, 6 insertions(+), 1 deletion(-) > > > > >> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh > > > >> index ae0449c3c..699d1538f 100755 > > > >> --- a/testcases/commands/df/df01.sh > > > >> +++ b/testcases/commands/df/df01.sh > > > >> @@ -1,7 +1,7 @@ > > > >> #!/bin/sh > > > >> # SPDX-License-Identifier: GPL-2.0-or-later > > > >> # Copyright (c) 2015 Fujitsu Ltd. > > > >> -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> > > > >> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> > > > >> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> > > > > >> # Test df command with some basic options. > > > >> @@ -46,6 +46,11 @@ df_test() > > > > >> ROD_SILENT rm -rf $TST_MNTPOINT/testimg > > > > >> + # force all the background garbage collection to run to completion > > > >> + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then > > > >> + tst_fsfreeze $TST_MNTPOINT > > > >> + fi > > > > > This looks overly specific, can't we just freeze and unfreeze the FS > > > > without looking at kernel version? Or will we get errors on older XFS? > > > > > I suppose that this may still start to fail on distribution kernels if > > > > some of the newer functionality gets backported... > > > So far it's OK on SLES, likely nothing related has been backported to it. > > I wonder if we should remove the check or just wait till first complaint. > > Well, I haven't seen any problems with older kernels, but I'll retest it more. > I don't expect any problems, thus I'm OK with removing it. If the fs doesn't support freezing (e.g. tmpfs), you'll get a -1 return value and errno == EOPNOTUPP or NOTTY. Is that going to cause the test to fail due to tst_brk? > Kind regards, > Petr > > > > Yup, I agree. Freeze should be safe for any kernel, I wouldn't condition it either. > > > > (You do want to be very sure that you're not freezing the root fs, tho, > > > if that is any possibility.) Why? Are you worried about not being able to unfreeze the rootfs? I know fstests has had problems with people doing: xfs_io -c freeze "/$moo" # whoops, moo is undefined <something that blocks> xfs_io -c thaw "/$moo" # never gets here But that's not the case here. Even if it is the rootfs, the text page containing the two ioctls should be in memory due to the instruction pointer, and even if it gets paged out, freezes don't block read faults. --D > > $TST_MNTPOINT in on $TMPDIR, which is by default /tmp. In case of /tmp being on > > root fs we're freezing root tmpfs. But it works on openSUSE, which found the > > problem. > > > Kind regards, > > Petr > > > > -Eric > > > > > Otherwise it looks good. > >
> On Mon, Mar 13, 2023 at 04:33:09PM +0100, Petr Vorel wrote: > > > > On 3/13/23 9:38 AM, Cyril Hrubis wrote: > > > > > Hi! > > > > >> XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze > > > > >> implementation to force all the background garbage collection to run > > > > >> to completion. > > > > >> Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ > > > > >> Suggested-by: Eric Sandeen <sandeen@redhat.com> > > > > >> Signed-off-by: Petr Vorel <pvorel@suse.cz> > > > > >> --- > > > > >> testcases/commands/df/df01.sh | 7 ++++++- > > > > >> 1 file changed, 6 insertions(+), 1 deletion(-) > > > > >> diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh > > > > >> index ae0449c3c..699d1538f 100755 > > > > >> --- a/testcases/commands/df/df01.sh > > > > >> +++ b/testcases/commands/df/df01.sh > > > > >> @@ -1,7 +1,7 @@ > > > > >> #!/bin/sh > > > > >> # SPDX-License-Identifier: GPL-2.0-or-later > > > > >> # Copyright (c) 2015 Fujitsu Ltd. > > > > >> -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> > > > > >> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> > > > > >> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> > > > > >> # Test df command with some basic options. > > > > >> @@ -46,6 +46,11 @@ df_test() > > > > >> ROD_SILENT rm -rf $TST_MNTPOINT/testimg > > > > >> + # force all the background garbage collection to run to completion > > > > >> + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then > > > > >> + tst_fsfreeze $TST_MNTPOINT > > > > >> + fi > > > > > This looks overly specific, can't we just freeze and unfreeze the FS > > > > > without looking at kernel version? Or will we get errors on older XFS? > > > > > I suppose that this may still start to fail on distribution kernels if > > > > > some of the newer functionality gets backported... > > > So far it's OK on SLES, likely nothing related has been backported to it. > > > I wonder if we should remove the check or just wait till first complaint. > > Well, I haven't seen any problems with older kernels, but I'll retest it more. > > I don't expect any problems, thus I'm OK with removing it. > If the fs doesn't support freezing (e.g. tmpfs), you'll get a -1 return > value and errno == EOPNOTUPP or NOTTY. Is that going to cause the test > to fail due to tst_brk? FYI I haven't noticed any problem so far, but need to test more. > > Kind regards, > > Petr > > > > Yup, I agree. Freeze should be safe for any kernel, I wouldn't condition it either. > > > > (You do want to be very sure that you're not freezing the root fs, tho, > > > > if that is any possibility.) > Why? Are you worried about not being able to unfreeze the rootfs? > I know fstests has had problems with people doing: > xfs_io -c freeze "/$moo" # whoops, moo is undefined > <something that blocks> > xfs_io -c thaw "/$moo" # never gets here > But that's not the case here. Yes, if you mean "/", that's not going to happen ($TST_MNTPOINT is never going to be "/"). I'm sorry, I mixed up mountpoint (used by fsfreeze and therefore also tst_fsfreeze.c) and device (=> there shouldn't be a problem). Kind regards, Petr > Even if it is the rootfs, the text page containing the two ioctls should > be in memory due to the instruction pointer, and even if it gets paged > out, freezes don't block read faults. > --D > > > $TST_MNTPOINT in on $TMPDIR, which is by default /tmp. In case of /tmp being on > > > root fs we're freezing root tmpfs. But it works on openSUSE, which found the > > > problem. > > > Kind regards, > > > Petr > > > > -Eric > > > > > Otherwise it looks good.
Hi all, FYI merged without kernel specific check. Thanks all for your input. Kind regards, Petr
diff --git a/testcases/commands/df/df01.sh b/testcases/commands/df/df01.sh index ae0449c3c..699d1538f 100755 --- a/testcases/commands/df/df01.sh +++ b/testcases/commands/df/df01.sh @@ -1,7 +1,7 @@ #!/bin/sh # SPDX-License-Identifier: GPL-2.0-or-later # Copyright (c) 2015 Fujitsu Ltd. -# Copyright (c) 2018-2022 Petr Vorel <pvorel@suse.cz> +# Copyright (c) 2018-2023 Petr Vorel <pvorel@suse.cz> # Author: Zhang Jin <jy_zhangjin@cn.fujitsu.com> # # Test df command with some basic options. @@ -46,6 +46,11 @@ df_test() ROD_SILENT rm -rf $TST_MNTPOINT/testimg + # force all the background garbage collection to run to completion + if [ "$TST_FS_TYPE" = "xfs" ] && tst_kvcmp -ge "5.19"; then + tst_fsfreeze $TST_MNTPOINT + fi + # flush file system buffers, then we can get the actual sizes. sync }
XFS since kernel 5.19 postpone certain operation. Use LTP fsfreeze implementation to force all the background garbage collection to run to completion. Link: https://lore.kernel.org/linux-block/974cc110-d47e-5fae-af5f-e2e610720e2d@redhat.com/ Suggested-by: Eric Sandeen <sandeen@redhat.com> Signed-off-by: Petr Vorel <pvorel@suse.cz> --- testcases/commands/df/df01.sh | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)