| Message ID | 20220601123406.265475-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | [v2] generic/623: fix test for runing on overlayfs | expand |
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.
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 >
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
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(-)