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

[Zorro Lang <zlang@redhat.com>]

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
>