Message ID | 1512100554-16368-1-git-send-email-cgxu@mykernel.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> 在 2017年12月1日,下午12:13,Eryu Guan <eguan@redhat.com> 写道: > > On Fri, Dec 01, 2017 at 12:04:44PM +0800, Chengguang Xu wrote: >> Hi Eryu, >> >> Actually, in my another test case generic/470 will need to check whether fs supports syncfs or not. >> I make shared infrastructure for checking that, and because it is common component >> I post as an individual patch instead of including in the case of generic/470. > > I think "_require_xfs_io_command syncfs" should be fine, there's no need > & not encouraged to add new binary & usage like this. If you want to run > syncfs(2) to make sure the kernel actually supports it, you can add a > new 'syncfs' switch case in _require_xfs_io_command. > Failure of _require_xfs_io_command check leads to notrun, if we have several sync patterns(combination of fsync/fdatasync/syncfs/sync) in an actual test case, the case will lose downward compatibility for old kernel. In this situation, we have to split test case though they look similar. Thanks, Chengguang.-- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 1, 2017 at 8:38 AM, Chengguang Xu <cgxu@mykernel.net> wrote: > >> 在 2017年12月1日,下午12:13,Eryu Guan <eguan@redhat.com> 写道: >> >> On Fri, Dec 01, 2017 at 12:04:44PM +0800, Chengguang Xu wrote: >>> Hi Eryu, >>> >>> Actually, in my another test case generic/470 will need to check whether fs supports syncfs or not. >>> I make shared infrastructure for checking that, and because it is common component >>> I post as an individual patch instead of including in the case of generic/470. >> >> I think "_require_xfs_io_command syncfs" should be fine, there's no need >> & not encouraged to add new binary & usage like this. If you want to run >> syncfs(2) to make sure the kernel actually supports it, you can add a >> new 'syncfs' switch case in _require_xfs_io_command. >> > > Failure of _require_xfs_io_command check leads to notrun, if we have several > sync patterns(combination of fsync/fdatasync/syncfs/sync) in an actual test case, > the case will lose downward compatibility for old kernel. > In this situation, we have to split test case though they look similar. > You have 2 options: Easy - split 2 tests - 1 requires syncfs and tests syncfs, 1 does not require and does not test syncfs Work - re-factor _require_xfs_io_command to _check_xfs_io_command that returns the error message but does not _notrun. use that hepler in your test to conditionally test syncfs. Anyway, I see people are not so fond of the delalloc "canary test". Perhaps a still simple and quick test would be to do small buffered write; sync; small direct io read? This test can still pass sometimes for buggy fs, but that will always be a problem with verifying that 'our' sync call did the job and not another thread in the system. Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 01, 2017 at 09:21:15AM +0200, Amir Goldstein wrote: > On Fri, Dec 1, 2017 at 8:38 AM, Chengguang Xu <cgxu@mykernel.net> wrote: > > > >> 在 2017年12月1日,下午12:13,Eryu Guan <eguan@redhat.com> 写道: > >> > >> On Fri, Dec 01, 2017 at 12:04:44PM +0800, Chengguang Xu wrote: > >>> Hi Eryu, > >>> > >>> Actually, in my another test case generic/470 will need to check whether fs supports syncfs or not. > >>> I make shared infrastructure for checking that, and because it is common component > >>> I post as an individual patch instead of including in the case of generic/470. > >> > >> I think "_require_xfs_io_command syncfs" should be fine, there's no need > >> & not encouraged to add new binary & usage like this. If you want to run > >> syncfs(2) to make sure the kernel actually supports it, you can add a > >> new 'syncfs' switch case in _require_xfs_io_command. > >> > > > > Failure of _require_xfs_io_command check leads to notrun, if we have several > > sync patterns(combination of fsync/fdatasync/syncfs/sync) in an actual test case, > > the case will lose downward compatibility for old kernel. > > In this situation, we have to split test case though they look similar. > > > > You have 2 options: > Easy - split 2 tests - 1 requires syncfs and tests syncfs, 1 does not > require and does not test syncfs > Work - re-factor _require_xfs_io_command to _check_xfs_io_command > that returns the error > message but does not _notrun. use that hepler in your test to > conditionally test syncfs. > > Anyway, I see people are not so fond of the delalloc "canary test". > Perhaps a still simple and quick test would be to do small buffered > write; sync; small direct io read? No, because the direct IO read can flush dirty buffers across the range the IO is being done on. IOWs, you don't know if the sync actually flushed it, the direct IO read flushed it, or the direct IO read fell back to buffered IO and simply read the in memory copy. Let's step back a moment: What bug/regression are you actually trying to expose/reproduce here? Why is it not already covered by at least one of the other generic sync/fsync tests we already have? Cheers, Dave.
On Fri, Dec 1, 2017 at 11:43 PM, Dave Chinner <david@fromorbit.com> wrote: > On Fri, Dec 01, 2017 at 09:21:15AM +0200, Amir Goldstein wrote: [...] >> >> Anyway, I see people are not so fond of the delalloc "canary test". >> Perhaps a still simple and quick test would be to do small buffered >> write; sync; small direct io read? > > No, because the direct IO read can flush dirty buffers across the > range the IO is being done on. IOWs, you don't know if the sync > actually flushed it, the direct IO read flushed it, or the direct IO > read fell back to buffered IO and simply read the in memory copy. > > Let's step back a moment: What bug/regression are you actually > trying to expose/reproduce here? The overlayfs bug (not regression) that Chengguang reported and posted a fix for - syncfs on overlayfs doesn't flush dirty inodes: https://marc.info/?l=linux-unionfs&m=151192099131198&w=2 > Why is it not already covered by at > least one of the other generic sync/fsync tests we already have? > Because there are zero "syncfs" tests. AFAIK, "fsync" is not broken on overlayfs, because it operated on the real underlying inode and "sync" is not a problem, because dirty underlying inodes will be flushed by sync_filesystem() of the underlying fs. Still, I recommended that Chengguang's test, whichever method is chosen, will be generic and cover all those sync variants. If I would to write a generic syncfs test for a blockdev fs, I would have chosen to _mount_flakey, call _flakey_drop_and_remount after syncfs and examine compare md5sum of the written file. Alas, overlayfs is not a blockdev fs, so using the flakey helpers as is the generic test won't run on overlayfs. It is possible to write an overlayfs specific test, which sets up a dm-flakey target over a loop device and uses that fs as the overlayfs upper fs, but then the test won't be generic. If you think we should go for non-generic overlayfs test, that is fine by me. If you have a clever idea how to write a generic "syncfs" test that would also apply to non blockdev fs, please do share it, because *that* was the requirement that lead me to the "delalloc canary" test. Thanks, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 2, 2017 at 12:43 PM, Amir Goldstein <amir73il@gmail.com> wrote: > On Fri, Dec 1, 2017 at 11:43 PM, Dave Chinner <david@fromorbit.com> wrote: >> On Fri, Dec 01, 2017 at 09:21:15AM +0200, Amir Goldstein wrote: > [...] >>> >>> Anyway, I see people are not so fond of the delalloc "canary test". >>> Perhaps a still simple and quick test would be to do small buffered >>> write; sync; small direct io read? >> >> No, because the direct IO read can flush dirty buffers across the >> range the IO is being done on. IOWs, you don't know if the sync >> actually flushed it, the direct IO read flushed it, or the direct IO >> read fell back to buffered IO and simply read the in memory copy. >> >> Let's step back a moment: What bug/regression are you actually >> trying to expose/reproduce here? > > > The overlayfs bug (not regression) that Chengguang reported and posted > a fix for - syncfs on overlayfs doesn't flush dirty inodes: > https://marc.info/?l=linux-unionfs&m=151192099131198&w=2 > >> Why is it not already covered by at >> least one of the other generic sync/fsync tests we already have? >> > > Because there are zero "syncfs" tests. > > AFAIK, "fsync" is not broken on overlayfs, because it operated on > the real underlying inode and "sync" is not a problem, because > dirty underlying inodes will be flushed by sync_filesystem() of the > underlying fs. > > Still, I recommended that Chengguang's test, whichever method > is chosen, will be generic and cover all those sync variants. > > If I would to write a generic syncfs test for a blockdev fs, I would have > chosen to _mount_flakey, call _flakey_drop_and_remount after syncfs > and examine compare md5sum of the written file. > > Alas, overlayfs is not a blockdev fs, so using the flakey helpers as is > the generic test won't run on overlayfs. > > It is possible to write an overlayfs specific test, which sets up a > dm-flakey target over a loop device and uses that fs as the overlayfs > upper fs, but then the test won't be generic. If you think we should go > for non-generic overlayfs test, that is fine by me. > > If you have a clever idea how to write a generic "syncfs" test that > would also apply to non blockdev fs, please do share it, because *that* > was the requirement that lead me to the "delalloc canary" test. > Another option is to start a new class of tests - "overlay group" tests. Instead of writing an "overlay fs" test with "_supported_fs overlay", we can write a "generic" test or even fs specific test and add it to "overlay" group with "_require_overlay_mount $SCRATCH_MNT" which checks that fs can be used as underlying fs for overlay mount. _overlay_mount_dirs() helper can be used in those generic tests. The discussed syncfs test would be in group generic/overlay and will use both _mount_flakey and _overlay_mount_dirs. A tester interested in overlayfs would have to invoke two different xfstest runs: ./check -overlay -g auto AND ./check -g overlay This may look strange, but it would be a *lot* simpler than teaching _require_dm_target and all the common/dm* helpers about OVL_BASE_SCRATCH_DEV and all the _overlay_scratch* helpers. Eryu, How do you feel about this option? Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 02, 2017 at 12:43:18PM +0200, Amir Goldstein wrote: > On Fri, Dec 1, 2017 at 11:43 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Fri, Dec 01, 2017 at 09:21:15AM +0200, Amir Goldstein wrote: > [...] > >> > >> Anyway, I see people are not so fond of the delalloc "canary test". > >> Perhaps a still simple and quick test would be to do small buffered > >> write; sync; small direct io read? > > > > No, because the direct IO read can flush dirty buffers across the > > range the IO is being done on. IOWs, you don't know if the sync > > actually flushed it, the direct IO read flushed it, or the direct IO > > read fell back to buffered IO and simply read the in memory copy. > > > > Let's step back a moment: What bug/regression are you actually > > trying to expose/reproduce here? > > > The overlayfs bug (not regression) that Chengguang reported and posted > a fix for - syncfs on overlayfs doesn't flush dirty inodes: > https://marc.info/?l=linux-unionfs&m=151192099131198&w=2 > > > Why is it not already covered by at > > least one of the other generic sync/fsync tests we already have? > > > > Because there are zero "syncfs" tests. It uses the same code as sync, so all the sync tests are testing syncfs, too. If syncfs is broken, then sync was also broken on overlay. > AFAIK, "fsync" is not broken on overlayfs, because it operated on > the real underlying inode and "sync" is not a problem, because > dirty underlying inodes will be flushed by sync_filesystem() of the > underlying fs. Which means nobody had tested a setup where the order of syncing inodes in different overlay layers exposed crash inconsistencies.... > Still, I recommended that Chengguang's test, whichever method > is chosen, will be generic and cover all those sync variants. > > If I would to write a generic syncfs test for a blockdev fs, I would have > chosen to _mount_flakey, call _flakey_drop_and_remount after syncfs > and examine compare md5sum of the written file. > > Alas, overlayfs is not a blockdev fs, so using the flakey helpers as is > the generic test won't run on overlayfs. That doesn't make stuffing about with the extent state of underlying filesytems any less hacky. > It is possible to write an overlayfs specific test, which sets up a > dm-flakey target over a loop device and uses that fs as the overlayfs > upper fs, but then the test won't be generic. If you think we should go > for non-generic overlayfs test, that is fine by me. That's precisely what the per-filesystem test directories are for. Put tests that are overlay specific or too dodgy to reliably run on all filesystems into tests/overlay. I don't care what you put in their because those tests then don't affect my XFS testing, or anyone else's non-overlay testing. > If you have a clever idea how to write a generic "syncfs" test > that would also apply to non blockdev fs, please do share it, > because *that* was the requirement that lead me to the "delalloc > canary" test. Implement FS_IOC_SHUTDOWN on overlay, then you can test it via software controlled shutdown. i.e. make overlay return true for _require_scratch_shutdown() and implement what is needed to shut it all down and prevent further writes of dirty inodes. Thens do data/metadata checks after a shutdown/unmount/mount cycle. Then what you have on disk after the mount cycle is what was written by sync/fsync/syncfs... We've only been using this shutdown mechanism to test fsync/sync mechanisms on XFS for ~20 years. Cheers, Dave.
On Sat, Dec 2, 2017 at 11:40 PM, Dave Chinner <david@fromorbit.com> wrote: > On Sat, Dec 02, 2017 at 12:43:18PM +0200, Amir Goldstein wrote: >> On Fri, Dec 1, 2017 at 11:43 PM, Dave Chinner <david@fromorbit.com> wrote: [...] > > Implement FS_IOC_SHUTDOWN on overlay, then you can test it via > software controlled shutdown. i.e. make overlay return true for > _require_scratch_shutdown() and implement what is needed to shut it > all down and prevent further writes of dirty inodes. Thens do > data/metadata checks after a shutdown/unmount/mount cycle. Then > what you have on disk after the mount cycle is what was written by > sync/fsync/syncfs... > > We've only been using this shutdown mechanism to test fsync/sync > mechanisms on XFS for ~20 years. > All right. We don't need to implement FS_IOC_SHUTDOWN on overlay, we only need to shutdown upper fs if upper fs supports shutdown. Doing that would be the equivalent of a dm-flakey test for a blokdev fs, because upper fs is the only persistent storage of overlayfs. Thanks for the advice. Chengguang, I propose that you teach _require_scratch_shutdown about OVL_BASE_SCRATCH_MNT and write and overlay/* test that tests syncfs by shutting down upper fs if upper fs supports shutdown. It doesn't matter much that the test won't run on all underlying fs as long as a regression will be caught by someone running overlay tests over xfs/ext4. Cheers, Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 在 2017年12月3日,下午2:45,Amir Goldstein <amir73il@gmail.com> 写道: > > On Sat, Dec 2, 2017 at 11:40 PM, Dave Chinner <david@fromorbit.com> wrote: >> On Sat, Dec 02, 2017 at 12:43:18PM +0200, Amir Goldstein wrote: >>> On Fri, Dec 1, 2017 at 11:43 PM, Dave Chinner <david@fromorbit.com> wrote: > [...] > >> >> Implement FS_IOC_SHUTDOWN on overlay, then you can test it via >> software controlled shutdown. i.e. make overlay return true for >> _require_scratch_shutdown() and implement what is needed to shut it >> all down and prevent further writes of dirty inodes. Thens do >> data/metadata checks after a shutdown/unmount/mount cycle. Then >> what you have on disk after the mount cycle is what was written by >> sync/fsync/syncfs... >> >> We've only been using this shutdown mechanism to test fsync/sync >> mechanisms on XFS for ~20 years. >> > > All right. > We don't need to implement FS_IOC_SHUTDOWN on overlay, we > only need to shutdown upper fs if upper fs supports shutdown. > Doing that would be the equivalent of a dm-flakey test for a blokdev fs, > because upper fs is the only persistent storage of overlayfs. > Thanks for the advice. > > Chengguang, > > I propose that you teach _require_scratch_shutdown about > OVL_BASE_SCRATCH_MNT and write and overlay/* test > that tests syncfs by shutting down upper fs if upper fs supports shutdown. OK, let me try this way, for data correctness I’ll use stat & md5_checksum. Is there anything else should I add for checking? > > It doesn't matter much that the test won't run on all underlying fs as long > as a regression will be caught by someone running overlay tests over > xfs/ext4. > > Cheers, > Amir. > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Dec 4, 2017 at 8:05 AM, Chengguang Xu <cgxu@mykernel.net> wrote: > ... >> All right. >> We don't need to implement FS_IOC_SHUTDOWN on overlay, we >> only need to shutdown upper fs if upper fs supports shutdown. >> Doing that would be the equivalent of a dm-flakey test for a blokdev fs, >> because upper fs is the only persistent storage of overlayfs. >> Thanks for the advice. >> >> Chengguang, >> >> I propose that you teach _require_scratch_shutdown about >> OVL_BASE_SCRATCH_MNT and write and overlay/* test >> that tests syncfs by shutting down upper fs if upper fs supports shutdown. > > OK, let me try this way, for data correctness I’ll use stat & md5_checksum. > Is there anything else should I add for checking? > Sounds good to me. Amir. -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/.gitignore b/.gitignore index f27c30a..54f010c 100644 --- a/.gitignore +++ b/.gitignore @@ -142,6 +142,7 @@ /src/writemod /src/writev_on_pagefault /src/xfsctl +/src/syncfs /src/aio-dio-regress/aio-dio-append-write-read-race /src/aio-dio-regress/aio-dio-cow-race /src/aio-dio-regress/aio-dio-cycle-write diff --git a/common/syncfs b/common/syncfs new file mode 100644 index 0000000..125ee4c --- /dev/null +++ b/common/syncfs @@ -0,0 +1,33 @@ +###### +# +# syncfs helpers +# +#----------------------------------------------------------------------- +# Copyright (c) 2017 Chengguang Xu <cgxu@mykernel.net>. +# All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +# This checks whether the syncfs syscall is supported. +_requires_syncfs() +{ + if test ! -x src/syncfs; then + _notrun "syncfs binary not found" + fi + + if ! src/syncfs $TEST_DIR; then + _notrun "kernel doesn't support syncfs syscall" + fi +} diff --git a/configure.ac b/configure.ac index 57092f1..7207a47 100644 --- a/configure.ac +++ b/configure.ac @@ -70,6 +70,7 @@ AC_PACKAGE_WANT_LINUX_PRCTL_H AC_PACKAGE_WANT_LINUX_FS_H AC_CHECK_FUNCS([renameat2]) +AC_CHECK_FUNCS([syncfs]) AC_CONFIG_HEADER(include/config.h) AC_CONFIG_FILES([include/builddefs]) diff --git a/src/Makefile b/src/Makefile index b101217..83451a9 100644 --- a/src/Makefile +++ b/src/Makefile @@ -23,7 +23,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ seek_copy_test t_readdir_1 t_readdir_2 fsync-tester nsexec cloner \ renameat2 t_getcwd e4compact test-nextquota punch-alternating \ attr-list-by-handle-cursor-test listxattr dio-interleaved t_dir_type \ - dio-invalidate-cache stat_test t_encrypted_d_revalidate + dio-invalidate-cache stat_test t_encrypted_d_revalidate syncfs SUBDIRS = log-writes perf diff --git a/src/syncfs.c b/src/syncfs.c new file mode 100644 index 0000000..d30a3e6 --- /dev/null +++ b/src/syncfs.c @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2017, Chengguang Xu <cgxu@mykernel.net> + * All Rights Reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it would be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* This is a trivial wrapper around the syncfs syscall. */ + +#include "global.h" + +#ifndef HAVE_SYNCFS +#include <sys/syscall.h> + +#if !defined(SYS_syncfs) && defined(__x86_64__) +#define SYS_syncfs 306 +#endif /* !defined(SYS_syncfs) && defined(__x86_64__) */ + +#if !defined(SYS_syncfs) && defined(__i386__) +#define SYS_syncfs 344 +#endif /* !defined(SYS_syncfs) && defined(__i386__) */ + +static int syncfs(int fd) +{ +#ifdef SYS_syncfs + return syscall(SYS_syncfs, fd); +#else + errno = ENOSYS; + return -1; +#endif /* SYS_syncfs */ +} +#endif /* HAVE_SYNCFS */ + +int main(int argc, char **argv) +{ + int fd, ret; + const char *fpath = NULL; + + if (argc != 2) + goto usage; + + fpath = argv[1]; + fd = open(fpath, O_RDONLY); + if (fd < 0) { + perror(""); + return 1; + } + + ret = syncfs(fd); + if (ret == -1 && errno == ENOSYS) + ret = 1; + else + ret = 0; + + close(fd); + return ret; + +usage: + fprintf(stderr, "usage: %s path\n", argv[0]); + return 1; +}
This adds binary utility and common script of syncfs for the actual test scripts. Signed-off-by: Chengguang Xu <cgxu@mykernel.net> --- .gitignore | 1 + common/syncfs | 33 +++++++++++++++++++++++++++ configure.ac | 1 + src/Makefile | 2 +- src/syncfs.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 common/syncfs create mode 100644 src/syncfs.c