On Tue, May 31, 2022 at 12:42:01PM +0300, Amir Goldstein wrote:
> On Tue, May 31, 2022 at 11:59 AM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> >
> > On Tue, May 31, 2022 at 07:58:48AM +1000, Dave Chinner wrote:
> > > On Mon, May 30, 2022 at 07:17:42PM +0300, Amir Goldstein wrote:
> > > > On Mon, May 30, 2022 at 4:29 PM Zorro Lang <zlang@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, May 30, 2022 at 02:29:05PM +0300, Amir Goldstein wrote:
> > > > > > For this test to run on overlayfs we open a different file to perform
> > > > > > shutdown+fsync while keeping the writeback target file open.
> > > > > >
> > > > > > We should probably perform fsync on the writeback target file, but
> > > > > > the bug is reproduced on xfs and overlayfs+xfs also as is.
> > > > > >
> > > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > > > > ---
> > > > > >
> > > > > > Zorro,
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > I tried changing fsync to syncfs for the test to be more correct in the
> > > > > > overlayfs case, but then golden output of xfs and overlayfs+xfs differ
> > > > > > and that would need some more output filtering (or disregarding output
> > > > > > completely).
> > > > > >
> > > > > > Since this minimal change does the job and does not change test behavior
> > > > > > on xfs on any of the tested kernels, I thought it might be good enough.
> > > > > >
> > > > > > Thanks,
> > > > > > Amir.
> > > > > >
> > > > > > tests/generic/623 | 5 ++++-
> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/tests/generic/623 b/tests/generic/623
> > > > > > index ea016d91..bb36ad25 100755
> > > > > > --- a/tests/generic/623
> > > > > > +++ b/tests/generic/623
> > > > > > @@ -24,10 +24,13 @@ _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+fsync while keeping the writeback target file open.
> > >
> > > To trigger the original bug, the post-shutdown fsync needs to be run
> > > on the original file. That triggers a writeback error writeback
> > > which clears the uptodate state on the mapped page. The mwrite that
> > > follows then trips over the state of the page and attempts IO
> > > operations without first checking shutdown state.
> > >
> > > Hence moving the fsync to a different file will break the mechanism
> > > the regression test uses to trigger the original bug.
> > >
> > > > > > 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" \
> > > > > > + -c "open $(_scratch_shutdown_handle)" -c shutdown -c fsync \
> > > > >
> > > > > Did you try to reproduce the original bug which this test case covers?
> > > > >
> > > >
> > > > Yes. As I wrote:
> > > > "tested that both configs fail with the same warning on v5.10.109"
> > > > Meaning the same bug that the test triggered before my change
> > > > in v5.10 is still triggered on xfs in v5.10 and it is triggered on both
> > > > xfs and overlayfs+xfs in v5.10 with my change.
> > >
> > > It reproduced on 5.10, but not because of the reasons you are
> > > suggesting.
> > >
> > > >
> > > > > According to the "man xfs_io":
> > > > >
> > > > > open [[ -acdfrstRTPL ] path ]
> > > > > Closes the current file, and opens the file specified by path instead.
> > > >
> > > > The documentation is incorrect.
> > > > Current file is not closed.
> > >
> > > It is not closed, but it's also not the target of subsequent file
> > > operations until you use "file 0" to switch back to it...
> >
> > I checked the xfsprogs/io/open.c, it truely doesn't "Close the current file",
> > (need to update the man-page?) and it looks like make all opened files as a
> > list, then operate them one by one(?):
> >
> > execve("/usr/sbin/xfs_io", ["xfs_io", "-c", "mmap 0 4k", "-c", "mwrite 0 4k", "-c", "open testfile", "-c", "fsync", "-c", "mwrite 0 4k", "testfile"], 0x7ffdc2285b78 /* 42 vars */) = 0
> > ...
> > openat(AT_FDCWD, "testfile", O_RDWR) = 3
> > ...
> > mmap(NULL, 4096, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_SHARED, 3, 0) = 0x7efc04548000
> > ...
> > openat(AT_FDCWD, "testfile", O_RDWR) = 4
> > ...
> > fsync(3) = 0
> > fsync(4) = 0
> > exit_group(0) = ?
> > +++ exited with 0 +++
> >
> > Anyway, looks like we're stuck at here.
>
> We are not stuck at all.
> It is very simple to fix.
> As you can see from the strace above and from Dave explanation,
> The test as I posted in v1 already does the right thing, because
> it does fsync the original file.
> Which explains why the test I posted DOES work as expected on
> both xfs and overlayfs and DOES reproduce the bug.
>
> However, if we want to write the test in a way that does NOT assume
> that xfs_io -c fsync operates on all the open files, I need to add some
> more commands:
>
> $XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" \
> -c "open $(_scratch_shutdown_handle)" -c shutdown \
> -c "file 0" -c fsync \
> -c "mwrite 0 4k" $file | _filter_scratch | _filter_xfs_io
>
> The only problem is this complicates the golden output
> which does not produce the same output for overlayfs and xfs
> open files, so I need to write another filter for that.
>
> The question is, is it worth it?
>
> Are we ever going to change xfs_io -c fsync to behave
> differently and fsync only the current file?
>
> If not, that I can just change the comment and keep the command
> and golden output:
>
> # 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+fsync while keeping the writeback target file open.
> +# 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 triggering the writeback failure.
> file=$SCRATCH_MNT/file
> $XFS_IO_PROG -fc "pwrite 0 4k" -c fsync $file | _filter_xfs_io
> $XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" \
> -c "open $(_scratch_shutdown_handle)" -c shutdown -c fsync \
> -c "mwrite 0 4k" $file | _filter_xfs_io
OK, if you persist, my personal opinion is: at least don't change the
original test logic of other fs especially when they're not wrong but
the change is a little tricky.
For example (not tested):
shutdown_cmd="shutdown"
# shutdown the underlying fs if there is
shutdown_handle="$(_scratch_shutdown_handle)"
if [ "$shutdown_handle" != "$SCRATCH_MNT" ];then
shutdown_cmd="\"open $shutdown_handle\" -c shutdown -c close"
fi
$XFS_IO_PROG -x -c "mmap 0 4k" -c "mwrite 0 4k" -c ${shutdown_cmd}
-c fsync -c "mwrite 0 4k" $file | _filter_xfs_io
Thanks,
Zorro
>
>
> > So I have 3 crude methods which is just
> > out of my mind, hope to get review:)
> >
> > 1) Skip this test for overlay directly, as this patch does.
> > 2) Add a _require_real_scratch_shutdown() helper, require the $FSTYP supporting
> > GOINGDOWN ioctl, don't fallback to the underlying fs. Then let generic/623
> > use it to skip overlay and other fs which really doesn't support shutdown.
> > 3) Change xfsprogs/io/shutdown.c, let shutdown_f() accept an extra mountpoint
> > path argument, to shutdown a specified mountpoint (?? not sure if it's
> > reasonable for xfs_io :). Then let generic/623 require and use this feature.
> >
>
> All of the above are overkills.
> The test fix already works.
>
> Thanks,
> Amir.
>
