[PATCH] common: Add infrastructure for syncfs syscall tests

[PATCH] common: Add infrastructure for syncfs syscall tests

[Chengguang Xu]

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

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;
+}
-- 
1.8.3.1

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Chengguang Xu]

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.


Thanks,
Chengguang.


> 在 2017年12月1日，上午11:55，Chengguang Xu <cgxu@mykernel.net> 写道：
> 
> 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
> 
> 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;
> +}
> -- 
> 1.8.3.1
> 
> 

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Eryu Guan]

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.

Thanks,
Eryu

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Chengguang Xu]

> 在 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.

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Amir Goldstein]

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.

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Amir Goldstein]

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.

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Amir Goldstein]

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.

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Amir Goldstein]

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.

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Chengguang Xu]

> 
> 在 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.
> 

Re: [PATCH] common: Add infrastructure for syncfs syscall tests

[Amir Goldstein]

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.