[PATCH v2] generic/623: fix test for runing on overlayfs

[PATCH v2] generic/623: fix test for runing on overlayfs

[Amir Goldstein]

For this test to run on overlayfs we open a different file to perform
shutdown while keeping the writeback target file open.

xfs_io -c fsync perform fsync also on the writeback target file, which
is needed for triggering the write fault.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---

Zorro,

Following your comment on v1, this version does not change the
behavior of the test when running on non-overlayfs.

I tested that this test passes for both xfs and overlayfs+xfs on v5.18
and tested that both configs fail with the same warning on v5.10.109.

Thanks,
Amir.

 tests/generic/623 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/generic/623 b/tests/generic/623
index ea016d91..5971717c 100755
--- a/tests/generic/623
+++ b/tests/generic/623
@@ -24,10 +24,22 @@ _scratch_mount
 # XFS had a regression where it failed to check shutdown status in the fault
 # path. This produced an iomap warning because writeback failure clears Uptodate
 # status on the page.
+
+# For this test to run on overlayfs we open a different file to perform
+# shutdown while keeping the writeback target file open.
+# xfs_io -c fsync post-shutdown performs fsync also on the writeback target file,
+# which is critical for trigerring the writeback failure.
+shutdown_cmd=()
+shutdown_handle="$(_scratch_shutdown_handle)"
+if [ "$shutdown_handle" != "$SCRATCH_MNT" ];then
+	shutdown_cmd+=("-c" "open $shutdown_handle")
+fi
+shutdown_cmd+=("-c" "shutdown")
+
 file=$SCRATCH_MNT/file
 $XFS_IO_PROG -fc "pwrite 0 4k" -c fsync $file | _filter_xfs_io
 ulimit -c 0
-$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
+$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" "${shutdown_cmd[@]}" -c fsync \
 	-c "mwrite 0 4k" $file | _filter_xfs_io
 
 # success, all done
-- 
2.25.1

Re: [PATCH v2] generic/623: fix test for runing on overlayfs

[Dave Chinner]

On Wed, Jun 01, 2022 at 03:34:06PM +0300, Amir Goldstein wrote:
> For this test to run on overlayfs we open a different file to perform
> shutdown while keeping the writeback target file open.
> 
> xfs_io -c fsync perform fsync also on the writeback target file, which
> is needed for triggering the write fault.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> 
> Zorro,
> 
> Following your comment on v1, this version does not change the
> behavior of the test when running on non-overlayfs.
> 
> I tested that this test passes for both xfs and overlayfs+xfs on v5.18
> and tested that both configs fail with the same warning on v5.10.109.
> 
> Thanks,
> Amir.
> 
>  tests/generic/623 | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/generic/623 b/tests/generic/623
> index ea016d91..5971717c 100755
> --- a/tests/generic/623
> +++ b/tests/generic/623
> @@ -24,10 +24,22 @@ _scratch_mount
>  # XFS had a regression where it failed to check shutdown status in the fault
>  # path. This produced an iomap warning because writeback failure clears Uptodate
>  # status on the page.
> +
> +# For this test to run on overlayfs we open a different file to perform
> +# shutdown while keeping the writeback target file open.
> +# xfs_io -c fsync post-shutdown performs fsync also on the writeback target file,
> +# which is critical for trigerring the writeback failure.
> +shutdown_cmd=()
> +shutdown_handle="$(_scratch_shutdown_handle)"
> +if [ "$shutdown_handle" != "$SCRATCH_MNT" ];then
> +	shutdown_cmd+=("-c" "open $shutdown_handle")
> +fi
> +shutdown_cmd+=("-c" "shutdown")

IMO, this is unnecessary complexity. The original patch with the
"fsync acts on all open files" comment above explains the xfs_io
fsync quirk that enables the test to do what it is supposed to be
doing without any of the this conditional command construction.

The less special case handling we need to splice into the test code,
the better.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] generic/623: fix test for runing on overlayfs

[Zorro Lang]

On Thu, Jun 02, 2022 at 09:44:43AM +1000, Dave Chinner wrote:
> On Wed, Jun 01, 2022 at 03:34:06PM +0300, Amir Goldstein wrote:
> > For this test to run on overlayfs we open a different file to perform
> > shutdown while keeping the writeback target file open.
> > 
> > xfs_io -c fsync perform fsync also on the writeback target file, which
> > is needed for triggering the write fault.
> > 
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > 
> > Zorro,
> > 
> > Following your comment on v1, this version does not change the
> > behavior of the test when running on non-overlayfs.
> > 
> > I tested that this test passes for both xfs and overlayfs+xfs on v5.18
> > and tested that both configs fail with the same warning on v5.10.109.
> > 
> > Thanks,
> > Amir.
> > 
> >  tests/generic/623 | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/generic/623 b/tests/generic/623
> > index ea016d91..5971717c 100755
> > --- a/tests/generic/623
> > +++ b/tests/generic/623
> > @@ -24,10 +24,22 @@ _scratch_mount
> >  # XFS had a regression where it failed to check shutdown status in the fault
> >  # path. This produced an iomap warning because writeback failure clears Uptodate
> >  # status on the page.
> > +
> > +# For this test to run on overlayfs we open a different file to perform
> > +# shutdown while keeping the writeback target file open.
> > +# xfs_io -c fsync post-shutdown performs fsync also on the writeback target file,
> > +# which is critical for trigerring the writeback failure.
> > +shutdown_cmd=()
> > +shutdown_handle="$(_scratch_shutdown_handle)"
> > +if [ "$shutdown_handle" != "$SCRATCH_MNT" ];then
> > +	shutdown_cmd+=("-c" "open $shutdown_handle")
> > +fi
> > +shutdown_cmd+=("-c" "shutdown")
> 
> IMO, this is unnecessary complexity. The original patch with the
> "fsync acts on all open files" comment above explains the xfs_io
> fsync quirk that enables the test to do what it is supposed to be
> doing without any of the this conditional command construction.
> 
> The less special case handling we need to splice into the test code,
> the better.

So you'd like to give below change a RVB?

-$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c shutdown -c fsync \
+$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" \
+             -c "open $(_scratch_shutdown_handle)" -c shutdown -c fsync \

I don't want complexity, just hope to keep the original testing logic.
As I don't know if the current behavior of open_f and fsync_f is stable, it
won't be changed in one day. Especially when I saw "open_f" says it
"Closes the current file, and opens the file specified by path instead"
but it doesn't. Now we depend on it opens a file as "current file", then
shutdown will happen on this file, then fsync will sync all 2 opened files.
That's the complexity for me.

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>