[PATCH v3] generic: add stress test for fanotify and inotify

[PATCH v3] generic: add stress test for fanotify and inotify

[Xiong Zhou]

Stress test for fanotify and inotify. Exercise fanotify and
inotify user interfaces in loop while other stress tests going
on in the watched test directory.

Watching slab object inotify_inode_mark size, report fail
it increases too fast. This may lead to a crash if OOM killer
invoked. Fixed by this series:

ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
3427ce7 fsnotify: clean up fsnotify()
f37650f fanotify: fix fsnotify_prepare_user_wait() failure
9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
0d6ec07 fsnotify: pin both inode and vfsmount mark
24c2030 fsnotify: clean up fsnotify_prepare/finish_user_wait()
7761daa fsnotify: convert fsnotify_group.refcnt from atomic_t to r..
9cf90ce fsnotify: Protect bail out path of fsnotify_add_mark_locked()

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---

Thanks for reviewing!

v3:
  add wait in cleanup
  add kernel commits fixed this issue in comments and commit msg
  fix seq numbers
  fix wording
v2:
  add to dangerous group
  new fsnotify group
  watch inotify_inode_mark slab instead of free memory
  watch inotify_inode_mark slab increasing speed instead
  reduce running time to 2m since we are watching the speed
other than numbers
  kill stress processes in cleanup

 .gitignore            |   1 +
 src/Makefile          |   3 +-
 src/fsnotify_stress.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/478     | 192 ++++++++++++++++++++++++++++
 tests/generic/478.out |   2 +
 tests/generic/group   |   1 +
 6 files changed, 537 insertions(+), 1 deletion(-)
 create mode 100644 src/fsnotify_stress.c
 create mode 100755 tests/generic/478
 create mode 100644 tests/generic/478.out

diff --git a/.gitignore b/.gitignore
index ee7eaed..ae01ed3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,6 +72,7 @@
 /src/fill
 /src/fill2
 /src/fs_perms
+/src/fsnotify_stress
 /src/fssum
 /src/fstest
 /src/fsync-err
diff --git a/src/Makefile b/src/Makefile
index b96b8cf..6b9f296 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -14,7 +14,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
 	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
 	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
-	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption
+	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
+	fsnotify_stress
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fsnotify_stress.c b/src/fsnotify_stress.c
new file mode 100644
index 0000000..f113918
--- /dev/null
+++ b/src/fsnotify_stress.c
@@ -0,0 +1,339 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE     /* Needed to get O_LARGEFILE definition */
+#endif
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <poll.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/fanotify.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <dirent.h>
+#include <sys/inotify.h>
+
+static void handle_events(int fd)
+{
+	const struct fanotify_event_metadata *metadata;
+	struct fanotify_event_metadata buf[200];
+	ssize_t len;
+	struct fanotify_response response;
+
+	/* Loop while events can be read from fanotify file descriptor */
+	for(;;) {
+		/* Read some events */
+		len = read(fd, (void *) &buf, sizeof(buf));
+		if (len == -1 && errno != EAGAIN) {
+			perror("read");
+			exit(EXIT_FAILURE);
+		}
+		/* Check if end of available data reached */
+		if (len <= 0)
+			break;
+		/* Point to the first event in the buffer */
+		metadata = buf;
+		/* Loop over all events in the buffer */
+		while (FAN_EVENT_OK(metadata, len)) {
+			/* Check that run-time and compile-time structures match */
+			if (metadata->vers != FANOTIFY_METADATA_VERSION) {
+				fprintf(stderr,
+				    "Mismatch of fanotify metadata version.\n");
+				exit(EXIT_FAILURE);
+			}
+			if (metadata->fd >= 0) {
+				/* Handle open permission event */
+				if (metadata->mask & FAN_OPEN_PERM) {
+					/* Allow file to be opened */
+					response.fd = metadata->fd;
+					response.response = FAN_ALLOW;
+					write(fd, &response,
+					    sizeof(struct fanotify_response));
+				}
+				/* Handle access permission event */
+				if (metadata->mask & FAN_ACCESS_PERM) {
+					/* Allow file to be accessed */
+					response.fd = metadata->fd;
+					response.response = FAN_ALLOW;
+					write(fd, &response,
+					    sizeof(struct fanotify_response));
+				}
+				/* Close the file descriptor of the event */
+				close(metadata->fd);
+			}
+			/* Advance to next event */
+			metadata = FAN_EVENT_NEXT(metadata, len);
+		}
+	}
+}
+
+static int fanotify_watch(char *arg)
+{
+	int fd, poll_num;
+	nfds_t nfds;
+	struct pollfd fds[2];
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+	/*
+	 * Mark the mount for:
+	 * - permission events before opening files
+	 * - notification events after closing a write-enabled
+	     file descriptor
+	 */
+	if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
+			FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
+			FAN_ONDIR | FAN_EVENT_ON_CHILD,
+			-1, arg) == -1) {
+		perror("fanotify_mark");
+		exit(EXIT_FAILURE);
+	}
+	/* Prepare for polling */
+	nfds = 1;
+	/* Fanotify input */
+	fds[0].fd = fd;
+	fds[0].events = POLLIN;
+	/* This is the loop to wait for incoming events */
+	while (1) {
+		poll_num = poll(fds, nfds, -1);
+		if (poll_num == -1) {
+			if (errno == EINTR)     /* Interrupted by a signal */
+				continue;           /* Restart poll() */
+			perror("poll");         /* Unexpected error */
+			exit(EXIT_FAILURE);
+		}
+		if (poll_num > 0) {
+			if (fds[0].revents & POLLIN) {
+				/* Fanotify events are available */
+				handle_events(fd);
+			}
+		}
+	}
+	exit(EXIT_SUCCESS);
+}
+
+static int fanotify_flush_stress(char *arg)
+{
+	int fd;
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Loop marking all kinds of events and flush */
+	while (1) {
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
+			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
+			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
+			perror("fanotify_mark add");
+
+		if (fanotify_mark(fd, FAN_MARK_FLUSH | FAN_MARK_MOUNT,
+						0, -1, arg) == -1)
+			perror("fanotify_mark flush mount");
+
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
+			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
+			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
+			perror("fanotify_mark add");
+
+		if (fanotify_mark(fd, FAN_MARK_FLUSH, 0, -1, arg) == -1)
+			perror("fanotify_mark flush");
+	}
+	close(fd);
+	exit(EXIT_SUCCESS);
+}
+
+static int fanotify_init_stress(char *arg)
+{
+	int fd;
+
+	while (1) {
+		/* Create the file descriptor for accessing the fanotify API */
+		fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT |
+				FAN_NONBLOCK, O_RDONLY | O_LARGEFILE);
+		if (fd == -1)
+			perror("fanotify_init");
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+				FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
+				FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
+				FAN_ONDIR | FAN_EVENT_ON_CHILD, -1,
+				arg) == -1)
+			perror("fanotify_mark");
+		close(fd);
+	}
+	exit(EXIT_SUCCESS);
+}
+
+static void add_mark(int fd, uint64_t mask, char *path)
+{
+	if (fanotify_mark(fd, FAN_MARK_ADD, mask, -1, path) == -1)
+		perror("fanotify_mark add");
+}
+
+static void remove_mark(int fd, uint64_t mask, char *path)
+{
+	if (fanotify_mark(fd, FAN_MARK_REMOVE, mask, -1, path) == -1)
+		perror("fanotify_mark remove");
+}
+
+static int fanotify_mark_stress(char *arg)
+{
+	int fd;
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+	/* Loop marking all kinds of events */
+	while (1) {
+		add_mark(fd, FAN_ACCESS, arg);
+		remove_mark(fd, FAN_ACCESS, arg);
+		add_mark(fd, FAN_MODIFY, arg);
+		remove_mark(fd, FAN_MODIFY, arg);
+		add_mark(fd, FAN_OPEN_PERM, arg);
+		remove_mark(fd, FAN_OPEN_PERM, arg);
+		add_mark(fd, FAN_CLOSE, arg);
+		remove_mark(fd, FAN_CLOSE, arg);
+		add_mark(fd, FAN_OPEN, arg);
+		remove_mark(fd, FAN_OPEN, arg);
+		add_mark(fd, FAN_ACCESS_PERM, arg);
+		remove_mark(fd, FAN_ACCESS_PERM, arg);
+		add_mark(fd, FAN_ONDIR, arg);
+		remove_mark(fd, FAN_ONDIR, arg);
+		add_mark(fd, FAN_EVENT_ON_CHILD, arg);
+		remove_mark(fd, FAN_EVENT_ON_CHILD, arg);
+	}
+
+	close(fd);
+	exit(EXIT_SUCCESS);
+}
+
+static int inotify_watch(char *arg)
+{
+	int notify_fd;
+	int wd, ret;
+	char *buf;
+
+	buf = malloc(sizeof(struct inotify_event) + NAME_MAX + 1);
+	if (buf == NULL) {
+		perror("malloc");
+		return -1;
+	}
+
+	notify_fd = inotify_init1(IN_CLOEXEC);
+	if (notify_fd == -1) {
+		perror("inotify_init1");
+		return -1;
+	}
+
+	wd = inotify_add_watch(notify_fd, arg,
+		IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE | IN_CLOSE_NOWRITE |
+		IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_MODIFY |
+		IN_MOVE_SELF | IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
+	if (wd < 0) {
+		perror("inotify_add_watch");
+		return -1;
+	}
+
+	while ((ret = read(notify_fd, buf, NAME_MAX)) != -1) {
+			;
+	}
+
+	ret = inotify_rm_watch(notify_fd, wd);
+	if (ret < 0)
+		perror("inotify_rm_watch");
+
+	close(notify_fd);
+	free(buf);
+	return 0;
+}
+
+static int inotify_add_rm_watch_stress(char *arg)
+{
+	int notify_fd;
+	int wd, ret;
+
+	notify_fd = inotify_init1(IN_CLOEXEC);
+	if (notify_fd == -1) {
+		perror("inotify_init1");
+		return -1;
+	}
+
+	while (1) {
+		wd = inotify_add_watch(notify_fd, arg,
+			IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE |
+			IN_CLOSE_NOWRITE | IN_CREATE | IN_DELETE |
+			IN_DELETE_SELF | IN_MODIFY | IN_MOVE_SELF |
+			IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
+		if (wd < 0)
+			perror("inotify_add_watch");
+		ret = inotify_rm_watch(notify_fd, wd);
+		if (ret < 0)
+			perror("inotify_rm_watch");
+	}
+	close(notify_fd);
+	return 0;
+}
+
+static int inotify_init_stress(void)
+{
+	int notify_fd;
+
+	while (1) {
+		notify_fd = inotify_init1(IN_CLOEXEC);
+		if (notify_fd == -1)
+			perror("inotify_init1");
+		close(notify_fd);
+	}
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	pid_t pid;
+
+	if (argc != 2) {
+		fprintf(stderr, "Usage: %s testdir\n", argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	if ((pid = fork()) == 0)
+		fanotify_watch(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_flush_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_init_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_mark_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_watch(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_add_rm_watch_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_init_stress();
+
+	return 0;
+}
diff --git a/tests/generic/478 b/tests/generic/478
new file mode 100755
index 0000000..cd1890f
--- /dev/null
+++ b/tests/generic/478
@@ -0,0 +1,192 @@
+#! /bin/bash
+# FS QA Test 478
+#
+# Stress test for fanotify and inotify.
+#
+# Exercise fanotify and inotify interfaces in loop while
+# other stress tests going on in the watched test directory.
+#
+# Watching slab object inotify_inode_mark size, report fail
+# it increases too fast. This may lead to a crash if OOM killer
+# invoked. Fixed by this series:
+#
+# ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to re..
+# 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
+# 3427ce7 fsnotify: clean up fsnotify()
+# f37650f fanotify: fix fsnotify_prepare_user_wait() failure
+# 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
+# 0d6ec07 fsnotify: pin both inode and vfsmount mark
+# 24c2030 fsnotify: clean up fsnotify_prepare/finish_user_wait()
+# 7761daa fsnotify: convert fsnotify_group.refcnt from atomic_t to ..
+# 9cf90ce fsnotify: Protect bail out path of fsnotify_add_mark_locked()
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Red Hat Inc.  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
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	touch $stopfile
+	while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
+		sleep 1
+	done
+	wait
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+stopfile=$tmp.stop
+TIMEOUT=2m
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+function add_files ()
+{
+	local i=$((RANDOM))
+
+	touch $SCRATCH_MNT/f-$i
+	ln -s $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-sym
+	ln $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-hdl
+	mkdir $SCRATCH_MNT/d-$i
+	mknod $SCRATCH_MNT/c-$i c 1 2
+	mknod $SCRATCH_MNT/b-$i b 1 2
+}
+
+function mv_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		mv -f f-$i f-$i-rename
+	done
+}
+
+function read_files ()
+{
+	find $SCRATCH_MNT/
+	cat $SCRATCH_MNT/f-*
+	ls -R $SCRATCH_MNT/d-*
+}
+
+function write_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		echo 1 > $i
+		echo 2 >> $i
+	done
+}
+
+function rm_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		rm -rf $i
+	done
+}
+
+slab_cal_inotify_inode_mark()
+{
+	echo 3 > /proc/sys/vm/drop_caches
+        local num_obj=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $3}')
+        local objsize=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $4}')
+        echo $(($num_obj * $objsize))
+}
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+# inotify_inode_mark slab size before test
+iim_0=`slab_cal_inotify_inode_mark`
+
+NR_CPUS=$(grep -c processor /proc/cpuinfo)
+[ $NR_CPUS -lt 4 ] && NR_CPUS=4
+opts="-d $SCRATCH_MNT/ -p $NR_CPUS -n 50 -v -l 0 -c $FSSTRESS_AVOID"
+$FSSTRESS_PROG $opts >> $seqres.full 2>&1 &
+
+rm -f $stopfile
+for j in 1 2 ; do
+
+for i in `seq 1 $(($NR_CPUS/7 + 1))` ; do
+	$here/src/fsnotify_stress $SCRATCH_MNT &
+done
+
+# run read/write files operations
+while [ ! -e $stopfile ]; do
+	add_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	mv_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	read_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	write_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	rm_files > /dev/null 2>&1
+done &
+
+done
+
+# stressing for $TIMEOUT
+sleep $TIMEOUT
+
+# inotify_inode_mark slab size after test
+iim_1=`slab_cal_inotify_inode_mark`
+
+# cleanup stress processes
+touch $stopfile
+while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
+	sleep 1
+done
+
+# wait _files functions done
+wait
+
+echo $iim_0 $iim_1 $(($iim_1 - $iim_0)) >> $seqres.full
+# If inotify_inode_mark slab size increases 1024 times of
+# itself in 2m, something bad happens because it could end up
+# invoking OOM killer, test fails.
+if [ $iim_1 -gt $iim_0 ] &&
+   [ $((iim_1-iim_0)) -gt $((1024*$iim_0)) ] ; then
+	echo inotify_inode_mark slab memory leaked
+fi
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/478.out b/tests/generic/478.out
new file mode 100644
index 0000000..4e2107a
--- /dev/null
+++ b/tests/generic/478.out
@@ -0,0 +1,2 @@
+QA output created by 478
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index cce03e9..8416957 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -480,3 +480,4 @@
 475 shutdown auto log metadata
 476 auto rw
 477 auto quick exportfs
+478 auto stress dangerous fsnotify
-- 
1.8.3.1

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Amir Goldstein]

On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> Stress test for fanotify and inotify. Exercise fanotify and
> inotify user interfaces in loop while other stress tests going
> on in the watched test directory.
>
> Watching slab object inotify_inode_mark size, report fail
> it increases too fast. This may lead to a crash if OOM killer
> invoked. Fixed by this series:
>
> ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
> 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
> 3427ce7 fsnotify: clean up fsnotify()
> f37650f fanotify: fix fsnotify_prepare_user_wait() failure
> 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()

That's a bit much. This commit below is the actual fix.
The rest are cleanups and other bug fixes.
It could also help testers to know the upstream kernel version
where the fix landed:

0d6ec07 fsnotify: pin both inode and vfsmount mark

Otherwise, looks good.

Amir.

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Sun, Feb 11, 2018 at 10:03:17PM +0200, Amir Goldstein wrote:
> On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> > Stress test for fanotify and inotify. Exercise fanotify and
> > inotify user interfaces in loop while other stress tests going
> > on in the watched test directory.
> >
> > Watching slab object inotify_inode_mark size, report fail
> > it increases too fast. This may lead to a crash if OOM killer
> > invoked. Fixed by this series:
> >
> > ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
> > 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
> > 3427ce7 fsnotify: clean up fsnotify()
> > f37650f fanotify: fix fsnotify_prepare_user_wait() failure
> > 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
> 
> That's a bit much. This commit below is the actual fix.
> The rest are cleanups and other bug fixes.

Cleanups are preparing for the fix. I believe it's better to list
the whole series. They are a patchset for some reason.

> It could also help testers to know the upstream kernel version
> where the fix landed:

There is no such info in other cases listing kernel commits. If
people want to know, they will find it out.

Thanks,
Xiong

> 
> 0d6ec07 fsnotify: pin both inode and vfsmount mark
> 
> Otherwise, looks good.
> 
> Amir.

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Amir Goldstein]

On Mon, Feb 12, 2018 at 2:41 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> On Sun, Feb 11, 2018 at 10:03:17PM +0200, Amir Goldstein wrote:
>> On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
>> > Stress test for fanotify and inotify. Exercise fanotify and
>> > inotify user interfaces in loop while other stress tests going
>> > on in the watched test directory.
>> >
>> > Watching slab object inotify_inode_mark size, report fail
>> > it increases too fast. This may lead to a crash if OOM killer
>> > invoked. Fixed by this series:
>> >
>> > ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
>> > 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
>> > 3427ce7 fsnotify: clean up fsnotify()
>> > f37650f fanotify: fix fsnotify_prepare_user_wait() failure
>> > 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
>>
>> That's a bit much. This commit below is the actual fix.
>> The rest are cleanups and other bug fixes.
>
> Cleanups are preparing for the fix.

No they are not. They are general cleanups and other bug fixed *after*
the fix. heck, 3 of the patches you listed are not from Miklos' patchset
at all.

If you wanted to include also prep patches that would only be:

24c2030 fsnotify: clean up fsnotify_prepare/finish_user_wait()

But I personally find that information to be irrelevant in test context.

> I believe it's better to list
> the whole series. They are a patchset for some reason.

You gotta know who your audience are.
Patchset is for a reason - the reason is that all patches should be
reviewed and merged together. But that's got nothing to do with the
reason you should list the fix commit in test description.
The reason for that is that testers needs to know if test is expected to
fail or pass on the kernel they are using.

Listing the cleanup patches does not serve this purpose.
In fact, it may confuse people testing stable kernels, because stable
kernel could have fix patches applied but not all cleanup patches.


>
>> It could also help testers to know the upstream kernel version
>> where the fix landed:
>
> There is no such info in other cases listing kernel commits. If
> people want to know, they will find it out.
>

Fair enough. Looking at other tests for common practices is a good
idea - the practice is to list only the fix commits. There are a few tests
that list more than a single commit, but in those cases the bug fix itself
is probably spread over few commits.

Cheers,
Amir.

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Mon, Feb 12, 2018 at 05:50:21AM +0200, Amir Goldstein wrote:
> On Mon, Feb 12, 2018 at 2:41 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> > On Sun, Feb 11, 2018 at 10:03:17PM +0200, Amir Goldstein wrote:
> >> On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> >> > Stress test for fanotify and inotify. Exercise fanotify and
> >> > inotify user interfaces in loop while other stress tests going
> >> > on in the watched test directory.
> >> >
> >> > Watching slab object inotify_inode_mark size, report fail
> >> > it increases too fast. This may lead to a crash if OOM killer
> >> > invoked. Fixed by this series:
> >> >
> >> > ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
> >> > 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
> >> > 3427ce7 fsnotify: clean up fsnotify()
> >> > f37650f fanotify: fix fsnotify_prepare_user_wait() failure
> >> > 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
> >>
> >> That's a bit much. This commit below is the actual fix.
> >> The rest are cleanups and other bug fixes.
> >
> > Cleanups are preparing for the fix.
> 
> No they are not. They are general cleanups and other bug fixed *after*
> the fix. heck, 3 of the patches you listed are not from Miklos' patchset
> at all.

Ya. I messed up, I did not check all the details well enough, just
confirmed there was dependency.

> 
> If you wanted to include also prep patches that would only be:
> 
> 24c2030 fsnotify: clean up fsnotify_prepare/finish_user_wait()
> 
> But I personally find that information to be irrelevant in test context.
> 
> > I believe it's better to list
> > the whole series. They are a patchset for some reason.
> 
> You gotta know who your audience are.

Yes, I know.

> Patchset is for a reason - the reason is that all patches should be
> reviewed and merged together. But that's got nothing to do with the
> reason you should list the fix commit in test description.
> The reason for that is that testers needs to know if test is expected to
> fail or pass on the kernel they are using.

AFAIK, known issue tracking is not handled by fstests. Listing them
is helping make things more clear, not for telling people expected
results. If I post this case before any fix commits available, not
listing any commits does make sense.

> 
> Listing the cleanup patches does not serve this purpose.
> In fact, it may confuse people testing stable kernels, because stable
> kernel could have fix patches applied but not all cleanup patches.

Interesting, I'm listing all of them and I'm saying "fixed by this series"
not "fixed by this commit".

THanks,
Xiong
> 
> 
> >
> >> It could also help testers to know the upstream kernel version
> >> where the fix landed:
> >
> > There is no such info in other cases listing kernel commits. If
> > people want to know, they will find it out.
> >
> 
> Fair enough. Looking at other tests for common practices is a good
> idea - the practice is to list only the fix commits. There are a few tests
> that list more than a single commit, but in those cases the bug fix itself
> is probably spread over few commits.
> 
> Cheers,
> Amir.

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Dave Chinner]

On Mon, Feb 12, 2018 at 12:33:57PM +0800, Xiong Zhou wrote:
> On Mon, Feb 12, 2018 at 05:50:21AM +0200, Amir Goldstein wrote:
> > Patchset is for a reason - the reason is that all patches should be
> > reviewed and merged together. But that's got nothing to do with the
> > reason you should list the fix commit in test description.
> > The reason for that is that testers needs to know if test is expected to
> > fail or pass on the kernel they are using.
> 
> AFAIK, known issue tracking is not handled by fstests.

And by that reasoning, commit IDs for bug fixes should not be in
fstests at all.

However, it is useful to many people to have a reference to the fix
associated with the test (or at least it's initial commit). That can
either be a commit ID or a url that points to the patch on the
mailing list that will be committed to fix it.

> > Listing the cleanup patches does not serve this purpose.
> > In fact, it may confuse people testing stable kernels, because stable
> > kernel could have fix patches applied but not all cleanup patches.
> 
> Interesting, I'm listing all of them and I'm saying "fixed by this series"
> not "fixed by this commit".

That's way too much irrelevant information, especially as "issue
tracking is not handled by fstests".

Please listen to Amir and follow the convention that everyone has
agreed on for referencing the fix a regression test relates to.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Mon, Feb 12, 2018 at 04:08:40PM +1100, Dave Chinner wrote:
> On Mon, Feb 12, 2018 at 12:33:57PM +0800, Xiong Zhou wrote:
> > On Mon, Feb 12, 2018 at 05:50:21AM +0200, Amir Goldstein wrote:
> > > Patchset is for a reason - the reason is that all patches should be
> > > reviewed and merged together. But that's got nothing to do with the
> > > reason you should list the fix commit in test description.
> > > The reason for that is that testers needs to know if test is expected to
> > > fail or pass on the kernel they are using.
> > 
> > AFAIK, known issue tracking is not handled by fstests.
> 
> And by that reasoning, commit IDs for bug fixes should not be in
> fstests at all.
> 
> However, it is useful to many people to have a reference to the fix
> associated with the test (or at least it's initial commit). That can
> either be a commit ID or a url that points to the patch on the
> mailing list that will be committed to fix it.
> 
> > > Listing the cleanup patches does not serve this purpose.
> > > In fact, it may confuse people testing stable kernels, because stable
> > > kernel could have fix patches applied but not all cleanup patches.
> > 
> > Interesting, I'm listing all of them and I'm saying "fixed by this series"
> > not "fixed by this commit".
> 
> That's way too much irrelevant information, especially as "issue
> tracking is not handled by fstests".

Fair enough.

> 
> Please listen to Amir and follow the convention that everyone has
> agreed on for referencing the fix a regression test relates to.

Okay.

Thanks,
Xiong
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

[PATCH v4] generic: add stress test for fanotify and inotify

[Xiong Zhou]

Stress test for fanotify and inotify. Exercise fanotify and
inotify user interfaces in loop while other stress tests going
on in the watched test directory.

Watching slab object inotify_inode_mark size, report fail
it increases too fast. This may lead to a crash if OOM killer
invoked.

kernel commit related to the fixes in v4.15-rc1:
0d6ec07 fsnotify: pin both inode and vfsmount mark

Signed-off-by: Xiong Zhou <xzhou@redhat.com>
---
Thanks for reviewing!

v4:
  only list one commit in the series

v3:
  add wait in cleanup
  add kernel commits fixed this issue in comments and commit msg
  fix seq numbers
  fix wording
v2:
  add to dangerous group
  new fsnotify group
  watch inotify_inode_mark slab instead of free memory
  watch inotify_inode_mark slab increasing speed instead
  reduce running time to 2m since we are watching the speed
other than numbers
  kill stress processes in cleanup

 .gitignore            |   1 +
 src/Makefile          |   3 +-
 src/fsnotify_stress.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/478     | 185 +++++++++++++++++++++++++++
 tests/generic/478.out |   2 +
 tests/generic/group   |   1 +
 6 files changed, 530 insertions(+), 1 deletion(-)
 create mode 100644 src/fsnotify_stress.c
 create mode 100755 tests/generic/478
 create mode 100644 tests/generic/478.out

diff --git a/.gitignore b/.gitignore
index ee7eaed..ae01ed3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,6 +72,7 @@
 /src/fill
 /src/fill2
 /src/fs_perms
+/src/fsnotify_stress
 /src/fssum
 /src/fstest
 /src/fsync-err
diff --git a/src/Makefile b/src/Makefile
index b96b8cf..6b9f296 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -14,7 +14,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
 	t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
 	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
 	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
-	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption
+	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
+	fsnotify_stress
 
 LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
 	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
diff --git a/src/fsnotify_stress.c b/src/fsnotify_stress.c
new file mode 100644
index 0000000..f113918
--- /dev/null
+++ b/src/fsnotify_stress.c
@@ -0,0 +1,339 @@
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE     /* Needed to get O_LARGEFILE definition */
+#endif
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <poll.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/fanotify.h>
+#include <unistd.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <dirent.h>
+#include <sys/inotify.h>
+
+static void handle_events(int fd)
+{
+	const struct fanotify_event_metadata *metadata;
+	struct fanotify_event_metadata buf[200];
+	ssize_t len;
+	struct fanotify_response response;
+
+	/* Loop while events can be read from fanotify file descriptor */
+	for(;;) {
+		/* Read some events */
+		len = read(fd, (void *) &buf, sizeof(buf));
+		if (len == -1 && errno != EAGAIN) {
+			perror("read");
+			exit(EXIT_FAILURE);
+		}
+		/* Check if end of available data reached */
+		if (len <= 0)
+			break;
+		/* Point to the first event in the buffer */
+		metadata = buf;
+		/* Loop over all events in the buffer */
+		while (FAN_EVENT_OK(metadata, len)) {
+			/* Check that run-time and compile-time structures match */
+			if (metadata->vers != FANOTIFY_METADATA_VERSION) {
+				fprintf(stderr,
+				    "Mismatch of fanotify metadata version.\n");
+				exit(EXIT_FAILURE);
+			}
+			if (metadata->fd >= 0) {
+				/* Handle open permission event */
+				if (metadata->mask & FAN_OPEN_PERM) {
+					/* Allow file to be opened */
+					response.fd = metadata->fd;
+					response.response = FAN_ALLOW;
+					write(fd, &response,
+					    sizeof(struct fanotify_response));
+				}
+				/* Handle access permission event */
+				if (metadata->mask & FAN_ACCESS_PERM) {
+					/* Allow file to be accessed */
+					response.fd = metadata->fd;
+					response.response = FAN_ALLOW;
+					write(fd, &response,
+					    sizeof(struct fanotify_response));
+				}
+				/* Close the file descriptor of the event */
+				close(metadata->fd);
+			}
+			/* Advance to next event */
+			metadata = FAN_EVENT_NEXT(metadata, len);
+		}
+	}
+}
+
+static int fanotify_watch(char *arg)
+{
+	int fd, poll_num;
+	nfds_t nfds;
+	struct pollfd fds[2];
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+	/*
+	 * Mark the mount for:
+	 * - permission events before opening files
+	 * - notification events after closing a write-enabled
+	     file descriptor
+	 */
+	if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
+			FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
+			FAN_ONDIR | FAN_EVENT_ON_CHILD,
+			-1, arg) == -1) {
+		perror("fanotify_mark");
+		exit(EXIT_FAILURE);
+	}
+	/* Prepare for polling */
+	nfds = 1;
+	/* Fanotify input */
+	fds[0].fd = fd;
+	fds[0].events = POLLIN;
+	/* This is the loop to wait for incoming events */
+	while (1) {
+		poll_num = poll(fds, nfds, -1);
+		if (poll_num == -1) {
+			if (errno == EINTR)     /* Interrupted by a signal */
+				continue;           /* Restart poll() */
+			perror("poll");         /* Unexpected error */
+			exit(EXIT_FAILURE);
+		}
+		if (poll_num > 0) {
+			if (fds[0].revents & POLLIN) {
+				/* Fanotify events are available */
+				handle_events(fd);
+			}
+		}
+	}
+	exit(EXIT_SUCCESS);
+}
+
+static int fanotify_flush_stress(char *arg)
+{
+	int fd;
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Loop marking all kinds of events and flush */
+	while (1) {
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
+			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
+			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
+			perror("fanotify_mark add");
+
+		if (fanotify_mark(fd, FAN_MARK_FLUSH | FAN_MARK_MOUNT,
+						0, -1, arg) == -1)
+			perror("fanotify_mark flush mount");
+
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
+			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
+			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
+			perror("fanotify_mark add");
+
+		if (fanotify_mark(fd, FAN_MARK_FLUSH, 0, -1, arg) == -1)
+			perror("fanotify_mark flush");
+	}
+	close(fd);
+	exit(EXIT_SUCCESS);
+}
+
+static int fanotify_init_stress(char *arg)
+{
+	int fd;
+
+	while (1) {
+		/* Create the file descriptor for accessing the fanotify API */
+		fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT |
+				FAN_NONBLOCK, O_RDONLY | O_LARGEFILE);
+		if (fd == -1)
+			perror("fanotify_init");
+		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
+				FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
+				FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
+				FAN_ONDIR | FAN_EVENT_ON_CHILD, -1,
+				arg) == -1)
+			perror("fanotify_mark");
+		close(fd);
+	}
+	exit(EXIT_SUCCESS);
+}
+
+static void add_mark(int fd, uint64_t mask, char *path)
+{
+	if (fanotify_mark(fd, FAN_MARK_ADD, mask, -1, path) == -1)
+		perror("fanotify_mark add");
+}
+
+static void remove_mark(int fd, uint64_t mask, char *path)
+{
+	if (fanotify_mark(fd, FAN_MARK_REMOVE, mask, -1, path) == -1)
+		perror("fanotify_mark remove");
+}
+
+static int fanotify_mark_stress(char *arg)
+{
+	int fd;
+
+	/* Create the file descriptor for accessing the fanotify API */
+	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
+					   O_RDONLY | O_LARGEFILE);
+	if (fd == -1) {
+		perror("fanotify_init");
+		exit(EXIT_FAILURE);
+	}
+	/* Loop marking all kinds of events */
+	while (1) {
+		add_mark(fd, FAN_ACCESS, arg);
+		remove_mark(fd, FAN_ACCESS, arg);
+		add_mark(fd, FAN_MODIFY, arg);
+		remove_mark(fd, FAN_MODIFY, arg);
+		add_mark(fd, FAN_OPEN_PERM, arg);
+		remove_mark(fd, FAN_OPEN_PERM, arg);
+		add_mark(fd, FAN_CLOSE, arg);
+		remove_mark(fd, FAN_CLOSE, arg);
+		add_mark(fd, FAN_OPEN, arg);
+		remove_mark(fd, FAN_OPEN, arg);
+		add_mark(fd, FAN_ACCESS_PERM, arg);
+		remove_mark(fd, FAN_ACCESS_PERM, arg);
+		add_mark(fd, FAN_ONDIR, arg);
+		remove_mark(fd, FAN_ONDIR, arg);
+		add_mark(fd, FAN_EVENT_ON_CHILD, arg);
+		remove_mark(fd, FAN_EVENT_ON_CHILD, arg);
+	}
+
+	close(fd);
+	exit(EXIT_SUCCESS);
+}
+
+static int inotify_watch(char *arg)
+{
+	int notify_fd;
+	int wd, ret;
+	char *buf;
+
+	buf = malloc(sizeof(struct inotify_event) + NAME_MAX + 1);
+	if (buf == NULL) {
+		perror("malloc");
+		return -1;
+	}
+
+	notify_fd = inotify_init1(IN_CLOEXEC);
+	if (notify_fd == -1) {
+		perror("inotify_init1");
+		return -1;
+	}
+
+	wd = inotify_add_watch(notify_fd, arg,
+		IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE | IN_CLOSE_NOWRITE |
+		IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_MODIFY |
+		IN_MOVE_SELF | IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
+	if (wd < 0) {
+		perror("inotify_add_watch");
+		return -1;
+	}
+
+	while ((ret = read(notify_fd, buf, NAME_MAX)) != -1) {
+			;
+	}
+
+	ret = inotify_rm_watch(notify_fd, wd);
+	if (ret < 0)
+		perror("inotify_rm_watch");
+
+	close(notify_fd);
+	free(buf);
+	return 0;
+}
+
+static int inotify_add_rm_watch_stress(char *arg)
+{
+	int notify_fd;
+	int wd, ret;
+
+	notify_fd = inotify_init1(IN_CLOEXEC);
+	if (notify_fd == -1) {
+		perror("inotify_init1");
+		return -1;
+	}
+
+	while (1) {
+		wd = inotify_add_watch(notify_fd, arg,
+			IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE |
+			IN_CLOSE_NOWRITE | IN_CREATE | IN_DELETE |
+			IN_DELETE_SELF | IN_MODIFY | IN_MOVE_SELF |
+			IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
+		if (wd < 0)
+			perror("inotify_add_watch");
+		ret = inotify_rm_watch(notify_fd, wd);
+		if (ret < 0)
+			perror("inotify_rm_watch");
+	}
+	close(notify_fd);
+	return 0;
+}
+
+static int inotify_init_stress(void)
+{
+	int notify_fd;
+
+	while (1) {
+		notify_fd = inotify_init1(IN_CLOEXEC);
+		if (notify_fd == -1)
+			perror("inotify_init1");
+		close(notify_fd);
+	}
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	pid_t pid;
+
+	if (argc != 2) {
+		fprintf(stderr, "Usage: %s testdir\n", argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	if ((pid = fork()) == 0)
+		fanotify_watch(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_flush_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_init_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		fanotify_mark_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_watch(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_add_rm_watch_stress(argv[1]);
+
+	if ((pid = fork()) == 0)
+		inotify_init_stress();
+
+	return 0;
+}
diff --git a/tests/generic/478 b/tests/generic/478
new file mode 100755
index 0000000..7139837
--- /dev/null
+++ b/tests/generic/478
@@ -0,0 +1,185 @@
+#! /bin/bash
+# FS QA Test 478
+#
+# Stress test for fanotify and inotify.
+#
+# Exercise fanotify and inotify interfaces in loop while
+# other stress tests going on in the watched test directory.
+#
+# Watching slab object inotify_inode_mark size, report fail
+# it increases too fast. This may lead to a crash if OOM killer
+# invoked.
+#
+# kernel commit related to the fixes in v4.15-rc1:
+# 0d6ec07 fsnotify: pin both inode and vfsmount mark
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2018 Red Hat Inc.  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
+#-----------------------------------------------------------------------
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	touch $stopfile
+	while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
+		sleep 1
+	done
+	wait
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+stopfile=$tmp.stop
+TIMEOUT=2m
+
+# Modify as appropriate.
+_supported_fs generic
+_supported_os Linux
+_require_scratch
+
+function add_files ()
+{
+	local i=$((RANDOM))
+
+	touch $SCRATCH_MNT/f-$i
+	ln -s $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-sym
+	ln $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-hdl
+	mkdir $SCRATCH_MNT/d-$i
+	mknod $SCRATCH_MNT/c-$i c 1 2
+	mknod $SCRATCH_MNT/b-$i b 1 2
+}
+
+function mv_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		mv -f f-$i f-$i-rename
+	done
+}
+
+function read_files ()
+{
+	find $SCRATCH_MNT/
+	cat $SCRATCH_MNT/f-*
+	ls -R $SCRATCH_MNT/d-*
+}
+
+function write_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		echo 1 > $i
+		echo 2 >> $i
+	done
+}
+
+function rm_files ()
+{
+	local i
+	for i in $SCRATCH_MNT/* ; do
+		rm -rf $i
+	done
+}
+
+slab_cal_inotify_inode_mark()
+{
+	echo 3 > /proc/sys/vm/drop_caches
+        local num_obj=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $3}')
+        local objsize=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $4}')
+        echo $(($num_obj * $objsize))
+}
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+# inotify_inode_mark slab size before test
+iim_0=`slab_cal_inotify_inode_mark`
+
+NR_CPUS=$(grep -c processor /proc/cpuinfo)
+[ $NR_CPUS -lt 4 ] && NR_CPUS=4
+opts="-d $SCRATCH_MNT/ -p $NR_CPUS -n 50 -v -l 0 -c $FSSTRESS_AVOID"
+$FSSTRESS_PROG $opts >> $seqres.full 2>&1 &
+
+rm -f $stopfile
+for j in 1 2 ; do
+
+for i in `seq 1 $(($NR_CPUS/7 + 1))` ; do
+	$here/src/fsnotify_stress $SCRATCH_MNT &
+done
+
+# run read/write files operations
+while [ ! -e $stopfile ]; do
+	add_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	mv_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	read_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	write_files > /dev/null 2>&1
+done &
+while [ ! -e $stopfile ]; do
+	rm_files > /dev/null 2>&1
+done &
+
+done
+
+# stressing for $TIMEOUT
+sleep $TIMEOUT
+
+# inotify_inode_mark slab size after test
+iim_1=`slab_cal_inotify_inode_mark`
+
+# cleanup stress processes
+touch $stopfile
+while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
+	sleep 1
+done
+
+# wait _files functions done
+wait
+
+echo $iim_0 $iim_1 $(($iim_1 - $iim_0)) >> $seqres.full
+# If inotify_inode_mark slab size increases 1024 times of
+# itself in 2m, something bad happens because it could end up
+# invoking OOM killer, test fails.
+if [ $iim_1 -gt $iim_0 ] &&
+   [ $((iim_1-iim_0)) -gt $((1024*$iim_0)) ] ; then
+	echo inotify_inode_mark slab memory leaked
+fi
+# success, all done
+echo "Silence is golden"
+status=0
+exit
diff --git a/tests/generic/478.out b/tests/generic/478.out
new file mode 100644
index 0000000..4e2107a
--- /dev/null
+++ b/tests/generic/478.out
@@ -0,0 +1,2 @@
+QA output created by 478
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index cce03e9..8416957 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -480,3 +480,4 @@
 475 shutdown auto log metadata
 476 auto rw
 477 auto quick exportfs
+478 auto stress dangerous fsnotify
-- 
1.8.3.1

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Dave Chinner]

On Mon, Feb 12, 2018 at 01:46:48PM +0800, Xiong Zhou wrote:
> Stress test for fanotify and inotify. Exercise fanotify and
> inotify user interfaces in loop while other stress tests going
> on in the watched test directory.
> 
> Watching slab object inotify_inode_mark size, report fail
> it increases too fast. This may lead to a crash if OOM killer
> invoked.
> 
> kernel commit related to the fixes in v4.15-rc1:
> 0d6ec07 fsnotify: pin both inode and vfsmount mark

Nice and concise!

Thanks for updating the fix reference.

-Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Mon, Feb 12, 2018 at 06:02:55PM +1100, Dave Chinner wrote:
> On Mon, Feb 12, 2018 at 01:46:48PM +0800, Xiong Zhou wrote:
> > Stress test for fanotify and inotify. Exercise fanotify and
> > inotify user interfaces in loop while other stress tests going
> > on in the watched test directory.
> > 
> > Watching slab object inotify_inode_mark size, report fail
> > it increases too fast. This may lead to a crash if OOM killer
> > invoked.
> > 
> > kernel commit related to the fixes in v4.15-rc1:
> > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> 
> Nice and concise!
> 
> Thanks for updating the fix reference.

Thanks all for reviewing!

Xiong

> 
> -Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Jan Kara]

On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> Stress test for fanotify and inotify. Exercise fanotify and
> inotify user interfaces in loop while other stress tests going
> on in the watched test directory.
> 
> Watching slab object inotify_inode_mark size, report fail
> it increases too fast. This may lead to a crash if OOM killer
> invoked.
> 
> kernel commit related to the fixes in v4.15-rc1:
> 0d6ec07 fsnotify: pin both inode and vfsmount mark
> 
> Signed-off-by: Xiong Zhou <xzhou@redhat.com>

I'm sorry for chiming in so late but I was on vacation. Just one question:
Currently, all inotify and fanotify tests are part of LTP. Is there any
good reason for putting this particular test to fstests and not LTP?
Specifically I've refrained from putting notification framework tests to
fstests because there's practically no relation of it to implementation of
any particular filesystem. Also I'd prefer not to have fanotify / inotify
tests in two different frameworks...

								Honza

> ---
> Thanks for reviewing!
> 
> v4:
>   only list one commit in the series
> 
> v3:
>   add wait in cleanup
>   add kernel commits fixed this issue in comments and commit msg
>   fix seq numbers
>   fix wording
> v2:
>   add to dangerous group
>   new fsnotify group
>   watch inotify_inode_mark slab instead of free memory
>   watch inotify_inode_mark slab increasing speed instead
>   reduce running time to 2m since we are watching the speed
> other than numbers
>   kill stress processes in cleanup
> 
>  .gitignore            |   1 +
>  src/Makefile          |   3 +-
>  src/fsnotify_stress.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/478     | 185 +++++++++++++++++++++++++++
>  tests/generic/478.out |   2 +
>  tests/generic/group   |   1 +
>  6 files changed, 530 insertions(+), 1 deletion(-)
>  create mode 100644 src/fsnotify_stress.c
>  create mode 100755 tests/generic/478
>  create mode 100644 tests/generic/478.out
> 
> diff --git a/.gitignore b/.gitignore
> index ee7eaed..ae01ed3 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -72,6 +72,7 @@
>  /src/fill
>  /src/fill2
>  /src/fs_perms
> +/src/fsnotify_stress
>  /src/fssum
>  /src/fstest
>  /src/fsync-err
> diff --git a/src/Makefile b/src/Makefile
> index b96b8cf..6b9f296 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -14,7 +14,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
>  	t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
>  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
>  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
> -	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption
> +	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> +	fsnotify_stress
>  
>  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
>  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/fsnotify_stress.c b/src/fsnotify_stress.c
> new file mode 100644
> index 0000000..f113918
> --- /dev/null
> +++ b/src/fsnotify_stress.c
> @@ -0,0 +1,339 @@
> +#ifndef _GNU_SOURCE
> +#define _GNU_SOURCE     /* Needed to get O_LARGEFILE definition */
> +#endif
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <poll.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/fanotify.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <dirent.h>
> +#include <sys/inotify.h>
> +
> +static void handle_events(int fd)
> +{
> +	const struct fanotify_event_metadata *metadata;
> +	struct fanotify_event_metadata buf[200];
> +	ssize_t len;
> +	struct fanotify_response response;
> +
> +	/* Loop while events can be read from fanotify file descriptor */
> +	for(;;) {
> +		/* Read some events */
> +		len = read(fd, (void *) &buf, sizeof(buf));
> +		if (len == -1 && errno != EAGAIN) {
> +			perror("read");
> +			exit(EXIT_FAILURE);
> +		}
> +		/* Check if end of available data reached */
> +		if (len <= 0)
> +			break;
> +		/* Point to the first event in the buffer */
> +		metadata = buf;
> +		/* Loop over all events in the buffer */
> +		while (FAN_EVENT_OK(metadata, len)) {
> +			/* Check that run-time and compile-time structures match */
> +			if (metadata->vers != FANOTIFY_METADATA_VERSION) {
> +				fprintf(stderr,
> +				    "Mismatch of fanotify metadata version.\n");
> +				exit(EXIT_FAILURE);
> +			}
> +			if (metadata->fd >= 0) {
> +				/* Handle open permission event */
> +				if (metadata->mask & FAN_OPEN_PERM) {
> +					/* Allow file to be opened */
> +					response.fd = metadata->fd;
> +					response.response = FAN_ALLOW;
> +					write(fd, &response,
> +					    sizeof(struct fanotify_response));
> +				}
> +				/* Handle access permission event */
> +				if (metadata->mask & FAN_ACCESS_PERM) {
> +					/* Allow file to be accessed */
> +					response.fd = metadata->fd;
> +					response.response = FAN_ALLOW;
> +					write(fd, &response,
> +					    sizeof(struct fanotify_response));
> +				}
> +				/* Close the file descriptor of the event */
> +				close(metadata->fd);
> +			}
> +			/* Advance to next event */
> +			metadata = FAN_EVENT_NEXT(metadata, len);
> +		}
> +	}
> +}
> +
> +static int fanotify_watch(char *arg)
> +{
> +	int fd, poll_num;
> +	nfds_t nfds;
> +	struct pollfd fds[2];
> +
> +	/* Create the file descriptor for accessing the fanotify API */
> +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> +					   O_RDONLY | O_LARGEFILE);
> +	if (fd == -1) {
> +		perror("fanotify_init");
> +		exit(EXIT_FAILURE);
> +	}
> +	/*
> +	 * Mark the mount for:
> +	 * - permission events before opening files
> +	 * - notification events after closing a write-enabled
> +	     file descriptor
> +	 */
> +	if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> +			FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
> +			FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
> +			FAN_ONDIR | FAN_EVENT_ON_CHILD,
> +			-1, arg) == -1) {
> +		perror("fanotify_mark");
> +		exit(EXIT_FAILURE);
> +	}
> +	/* Prepare for polling */
> +	nfds = 1;
> +	/* Fanotify input */
> +	fds[0].fd = fd;
> +	fds[0].events = POLLIN;
> +	/* This is the loop to wait for incoming events */
> +	while (1) {
> +		poll_num = poll(fds, nfds, -1);
> +		if (poll_num == -1) {
> +			if (errno == EINTR)     /* Interrupted by a signal */
> +				continue;           /* Restart poll() */
> +			perror("poll");         /* Unexpected error */
> +			exit(EXIT_FAILURE);
> +		}
> +		if (poll_num > 0) {
> +			if (fds[0].revents & POLLIN) {
> +				/* Fanotify events are available */
> +				handle_events(fd);
> +			}
> +		}
> +	}
> +	exit(EXIT_SUCCESS);
> +}
> +
> +static int fanotify_flush_stress(char *arg)
> +{
> +	int fd;
> +
> +	/* Create the file descriptor for accessing the fanotify API */
> +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> +					   O_RDONLY | O_LARGEFILE);
> +	if (fd == -1) {
> +		perror("fanotify_init");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	/* Loop marking all kinds of events and flush */
> +	while (1) {
> +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> +			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
> +			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
> +			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
> +			perror("fanotify_mark add");
> +
> +		if (fanotify_mark(fd, FAN_MARK_FLUSH | FAN_MARK_MOUNT,
> +						0, -1, arg) == -1)
> +			perror("fanotify_mark flush mount");
> +
> +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> +			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
> +			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
> +			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
> +			perror("fanotify_mark add");
> +
> +		if (fanotify_mark(fd, FAN_MARK_FLUSH, 0, -1, arg) == -1)
> +			perror("fanotify_mark flush");
> +	}
> +	close(fd);
> +	exit(EXIT_SUCCESS);
> +}
> +
> +static int fanotify_init_stress(char *arg)
> +{
> +	int fd;
> +
> +	while (1) {
> +		/* Create the file descriptor for accessing the fanotify API */
> +		fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT |
> +				FAN_NONBLOCK, O_RDONLY | O_LARGEFILE);
> +		if (fd == -1)
> +			perror("fanotify_init");
> +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> +				FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
> +				FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
> +				FAN_ONDIR | FAN_EVENT_ON_CHILD, -1,
> +				arg) == -1)
> +			perror("fanotify_mark");
> +		close(fd);
> +	}
> +	exit(EXIT_SUCCESS);
> +}
> +
> +static void add_mark(int fd, uint64_t mask, char *path)
> +{
> +	if (fanotify_mark(fd, FAN_MARK_ADD, mask, -1, path) == -1)
> +		perror("fanotify_mark add");
> +}
> +
> +static void remove_mark(int fd, uint64_t mask, char *path)
> +{
> +	if (fanotify_mark(fd, FAN_MARK_REMOVE, mask, -1, path) == -1)
> +		perror("fanotify_mark remove");
> +}
> +
> +static int fanotify_mark_stress(char *arg)
> +{
> +	int fd;
> +
> +	/* Create the file descriptor for accessing the fanotify API */
> +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> +					   O_RDONLY | O_LARGEFILE);
> +	if (fd == -1) {
> +		perror("fanotify_init");
> +		exit(EXIT_FAILURE);
> +	}
> +	/* Loop marking all kinds of events */
> +	while (1) {
> +		add_mark(fd, FAN_ACCESS, arg);
> +		remove_mark(fd, FAN_ACCESS, arg);
> +		add_mark(fd, FAN_MODIFY, arg);
> +		remove_mark(fd, FAN_MODIFY, arg);
> +		add_mark(fd, FAN_OPEN_PERM, arg);
> +		remove_mark(fd, FAN_OPEN_PERM, arg);
> +		add_mark(fd, FAN_CLOSE, arg);
> +		remove_mark(fd, FAN_CLOSE, arg);
> +		add_mark(fd, FAN_OPEN, arg);
> +		remove_mark(fd, FAN_OPEN, arg);
> +		add_mark(fd, FAN_ACCESS_PERM, arg);
> +		remove_mark(fd, FAN_ACCESS_PERM, arg);
> +		add_mark(fd, FAN_ONDIR, arg);
> +		remove_mark(fd, FAN_ONDIR, arg);
> +		add_mark(fd, FAN_EVENT_ON_CHILD, arg);
> +		remove_mark(fd, FAN_EVENT_ON_CHILD, arg);
> +	}
> +
> +	close(fd);
> +	exit(EXIT_SUCCESS);
> +}
> +
> +static int inotify_watch(char *arg)
> +{
> +	int notify_fd;
> +	int wd, ret;
> +	char *buf;
> +
> +	buf = malloc(sizeof(struct inotify_event) + NAME_MAX + 1);
> +	if (buf == NULL) {
> +		perror("malloc");
> +		return -1;
> +	}
> +
> +	notify_fd = inotify_init1(IN_CLOEXEC);
> +	if (notify_fd == -1) {
> +		perror("inotify_init1");
> +		return -1;
> +	}
> +
> +	wd = inotify_add_watch(notify_fd, arg,
> +		IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE | IN_CLOSE_NOWRITE |
> +		IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_MODIFY |
> +		IN_MOVE_SELF | IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
> +	if (wd < 0) {
> +		perror("inotify_add_watch");
> +		return -1;
> +	}
> +
> +	while ((ret = read(notify_fd, buf, NAME_MAX)) != -1) {
> +			;
> +	}
> +
> +	ret = inotify_rm_watch(notify_fd, wd);
> +	if (ret < 0)
> +		perror("inotify_rm_watch");
> +
> +	close(notify_fd);
> +	free(buf);
> +	return 0;
> +}
> +
> +static int inotify_add_rm_watch_stress(char *arg)
> +{
> +	int notify_fd;
> +	int wd, ret;
> +
> +	notify_fd = inotify_init1(IN_CLOEXEC);
> +	if (notify_fd == -1) {
> +		perror("inotify_init1");
> +		return -1;
> +	}
> +
> +	while (1) {
> +		wd = inotify_add_watch(notify_fd, arg,
> +			IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE |
> +			IN_CLOSE_NOWRITE | IN_CREATE | IN_DELETE |
> +			IN_DELETE_SELF | IN_MODIFY | IN_MOVE_SELF |
> +			IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
> +		if (wd < 0)
> +			perror("inotify_add_watch");
> +		ret = inotify_rm_watch(notify_fd, wd);
> +		if (ret < 0)
> +			perror("inotify_rm_watch");
> +	}
> +	close(notify_fd);
> +	return 0;
> +}
> +
> +static int inotify_init_stress(void)
> +{
> +	int notify_fd;
> +
> +	while (1) {
> +		notify_fd = inotify_init1(IN_CLOEXEC);
> +		if (notify_fd == -1)
> +			perror("inotify_init1");
> +		close(notify_fd);
> +	}
> +	return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	pid_t pid;
> +
> +	if (argc != 2) {
> +		fprintf(stderr, "Usage: %s testdir\n", argv[0]);
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if ((pid = fork()) == 0)
> +		fanotify_watch(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		fanotify_flush_stress(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		fanotify_init_stress(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		fanotify_mark_stress(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		inotify_watch(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		inotify_add_rm_watch_stress(argv[1]);
> +
> +	if ((pid = fork()) == 0)
> +		inotify_init_stress();
> +
> +	return 0;
> +}
> diff --git a/tests/generic/478 b/tests/generic/478
> new file mode 100755
> index 0000000..7139837
> --- /dev/null
> +++ b/tests/generic/478
> @@ -0,0 +1,185 @@
> +#! /bin/bash
> +# FS QA Test 478
> +#
> +# Stress test for fanotify and inotify.
> +#
> +# Exercise fanotify and inotify interfaces in loop while
> +# other stress tests going on in the watched test directory.
> +#
> +# Watching slab object inotify_inode_mark size, report fail
> +# it increases too fast. This may lead to a crash if OOM killer
> +# invoked.
> +#
> +# kernel commit related to the fixes in v4.15-rc1:
> +# 0d6ec07 fsnotify: pin both inode and vfsmount mark
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2018 Red Hat Inc.  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
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	touch $stopfile
> +	while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
> +		sleep 1
> +	done
> +	wait
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +stopfile=$tmp.stop
> +TIMEOUT=2m
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_scratch
> +
> +function add_files ()
> +{
> +	local i=$((RANDOM))
> +
> +	touch $SCRATCH_MNT/f-$i
> +	ln -s $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-sym
> +	ln $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-hdl
> +	mkdir $SCRATCH_MNT/d-$i
> +	mknod $SCRATCH_MNT/c-$i c 1 2
> +	mknod $SCRATCH_MNT/b-$i b 1 2
> +}
> +
> +function mv_files ()
> +{
> +	local i
> +	for i in $SCRATCH_MNT/* ; do
> +		mv -f f-$i f-$i-rename
> +	done
> +}
> +
> +function read_files ()
> +{
> +	find $SCRATCH_MNT/
> +	cat $SCRATCH_MNT/f-*
> +	ls -R $SCRATCH_MNT/d-*
> +}
> +
> +function write_files ()
> +{
> +	local i
> +	for i in $SCRATCH_MNT/* ; do
> +		echo 1 > $i
> +		echo 2 >> $i
> +	done
> +}
> +
> +function rm_files ()
> +{
> +	local i
> +	for i in $SCRATCH_MNT/* ; do
> +		rm -rf $i
> +	done
> +}
> +
> +slab_cal_inotify_inode_mark()
> +{
> +	echo 3 > /proc/sys/vm/drop_caches
> +        local num_obj=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $3}')
> +        local objsize=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $4}')
> +        echo $(($num_obj * $objsize))
> +}
> +
> +_scratch_mkfs >>$seqres.full 2>&1
> +_scratch_mount
> +
> +# inotify_inode_mark slab size before test
> +iim_0=`slab_cal_inotify_inode_mark`
> +
> +NR_CPUS=$(grep -c processor /proc/cpuinfo)
> +[ $NR_CPUS -lt 4 ] && NR_CPUS=4
> +opts="-d $SCRATCH_MNT/ -p $NR_CPUS -n 50 -v -l 0 -c $FSSTRESS_AVOID"
> +$FSSTRESS_PROG $opts >> $seqres.full 2>&1 &
> +
> +rm -f $stopfile
> +for j in 1 2 ; do
> +
> +for i in `seq 1 $(($NR_CPUS/7 + 1))` ; do
> +	$here/src/fsnotify_stress $SCRATCH_MNT &
> +done
> +
> +# run read/write files operations
> +while [ ! -e $stopfile ]; do
> +	add_files > /dev/null 2>&1
> +done &
> +while [ ! -e $stopfile ]; do
> +	mv_files > /dev/null 2>&1
> +done &
> +while [ ! -e $stopfile ]; do
> +	read_files > /dev/null 2>&1
> +done &
> +while [ ! -e $stopfile ]; do
> +	write_files > /dev/null 2>&1
> +done &
> +while [ ! -e $stopfile ]; do
> +	rm_files > /dev/null 2>&1
> +done &
> +
> +done
> +
> +# stressing for $TIMEOUT
> +sleep $TIMEOUT
> +
> +# inotify_inode_mark slab size after test
> +iim_1=`slab_cal_inotify_inode_mark`
> +
> +# cleanup stress processes
> +touch $stopfile
> +while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
> +	sleep 1
> +done
> +
> +# wait _files functions done
> +wait
> +
> +echo $iim_0 $iim_1 $(($iim_1 - $iim_0)) >> $seqres.full
> +# If inotify_inode_mark slab size increases 1024 times of
> +# itself in 2m, something bad happens because it could end up
> +# invoking OOM killer, test fails.
> +if [ $iim_1 -gt $iim_0 ] &&
> +   [ $((iim_1-iim_0)) -gt $((1024*$iim_0)) ] ; then
> +	echo inotify_inode_mark slab memory leaked
> +fi
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/478.out b/tests/generic/478.out
> new file mode 100644
> index 0000000..4e2107a
> --- /dev/null
> +++ b/tests/generic/478.out
> @@ -0,0 +1,2 @@
> +QA output created by 478
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index cce03e9..8416957 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -480,3 +480,4 @@
>  475 shutdown auto log metadata
>  476 auto rw
>  477 auto quick exportfs
> +478 auto stress dangerous fsnotify
> -- 
> 1.8.3.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Amir Goldstein]

On Wed, Feb 14, 2018 at 1:03 PM, Jan Kara <jack@suse.cz> wrote:
> On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
>> Stress test for fanotify and inotify. Exercise fanotify and
>> inotify user interfaces in loop while other stress tests going
>> on in the watched test directory.
>>
>> Watching slab object inotify_inode_mark size, report fail
>> it increases too fast. This may lead to a crash if OOM killer
>> invoked.
>>
>> kernel commit related to the fixes in v4.15-rc1:
>> 0d6ec07 fsnotify: pin both inode and vfsmount mark
>>
>> Signed-off-by: Xiong Zhou <xzhou@redhat.com>
>
> I'm sorry for chiming in so late but I was on vacation. Just one question:
> Currently, all inotify and fanotify tests are part of LTP. Is there any
> good reason for putting this particular test to fstests and not LTP?
> Specifically I've refrained from putting notification framework tests to
> fstests because there's practically no relation of it to implementation of
> any particular filesystem. Also I'd prefer not to have fanotify / inotify
> tests in two different frameworks...
>

I second that.
Also, Eryu did not chime in yet (and Dave probably did not look closely),
but I think fstests in general try to refrain from single purpose test
programs such as fsnotify_stress.

Xiong,

If you do choose to implement an LTP test as Jan recommended,
please base it on top of my patches to convert LTP inotify tests to
new lib https://github.com/linux-test-project/ltp/pull/246.

Thanks,
Amir.

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Eryu Guan]

On Wed, Feb 14, 2018 at 01:22:06PM +0200, Amir Goldstein wrote:
> On Wed, Feb 14, 2018 at 1:03 PM, Jan Kara <jack@suse.cz> wrote:
> > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> >> Stress test for fanotify and inotify. Exercise fanotify and
> >> inotify user interfaces in loop while other stress tests going
> >> on in the watched test directory.
> >>
> >> Watching slab object inotify_inode_mark size, report fail
> >> it increases too fast. This may lead to a crash if OOM killer
> >> invoked.
> >>
> >> kernel commit related to the fixes in v4.15-rc1:
> >> 0d6ec07 fsnotify: pin both inode and vfsmount mark
> >>
> >> Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> >
> > I'm sorry for chiming in so late but I was on vacation. Just one question:
> > Currently, all inotify and fanotify tests are part of LTP. Is there any
> > good reason for putting this particular test to fstests and not LTP?
> > Specifically I've refrained from putting notification framework tests to
> > fstests because there's practically no relation of it to implementation of
> > any particular filesystem. Also I'd prefer not to have fanotify / inotify
> > tests in two different frameworks...
> >
> 
> I second that.
> Also, Eryu did not chime in yet (and Dave probably did not look closely),
> but I think fstests in general try to refrain from single purpose test
> programs such as fsnotify_stress.

I have no problem with committing fsnotify tests in fstests, we do have
tests that exercise the vfs level infrastructure purely, e.g. the shared
subtree operations tests.

But if both fanotify subsystem maintainer and main reviewer suggest
putting this test to LTP, I have no problem with that too :)

Thanks,
Eryu

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Amir Goldstein]

On Wed, Feb 14, 2018 at 5:03 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Wed, Feb 14, 2018 at 01:22:06PM +0200, Amir Goldstein wrote:
>> On Wed, Feb 14, 2018 at 1:03 PM, Jan Kara <jack@suse.cz> wrote:
>> > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
>> >> Stress test for fanotify and inotify. Exercise fanotify and
>> >> inotify user interfaces in loop while other stress tests going
>> >> on in the watched test directory.
>> >>
>> >> Watching slab object inotify_inode_mark size, report fail
>> >> it increases too fast. This may lead to a crash if OOM killer
>> >> invoked.
>> >>
>> >> kernel commit related to the fixes in v4.15-rc1:
>> >> 0d6ec07 fsnotify: pin both inode and vfsmount mark
>> >>
>> >> Signed-off-by: Xiong Zhou <xzhou@redhat.com>
>> >
>> > I'm sorry for chiming in so late but I was on vacation. Just one question:
>> > Currently, all inotify and fanotify tests are part of LTP. Is there any
>> > good reason for putting this particular test to fstests and not LTP?
>> > Specifically I've refrained from putting notification framework tests to
>> > fstests because there's practically no relation of it to implementation of
>> > any particular filesystem. Also I'd prefer not to have fanotify / inotify
>> > tests in two different frameworks...
>> >
>>
>> I second that.
>> Also, Eryu did not chime in yet (and Dave probably did not look closely),
>> but I think fstests in general try to refrain from single purpose test
>> programs such as fsnotify_stress.
>
> I have no problem with committing fsnotify tests in fstests, we do have
> tests that exercise the vfs level infrastructure purely, e.g. the shared
> subtree operations tests.
>
> But if both fanotify subsystem maintainer and main reviewer suggest
> putting this test to LTP, I have no problem with that too :)
>

Clarification:
Although I am siding Jan on LTP being an appropriate place for this
test I have no objection for merging this test to fstests and you can also
add Reviewed-by: Amir Goldstein <amir73il@gmail.com> for v4.

Also, the statement that "there's practically no relation of [notification
framework] to implementation of any particular filesystem" is not accurate.
It would have been more accurate to say that the regression that this test
checks for has no relation to any particular filesystem and therefore the
test belongs to LTP, which tests the kernel inotify/fanotify syscalls.

As I wrote, I just posted 2 LTP inotify tests which are generic, but only fail
on overlayfs: https://github.com/linux-test-project/ltp/pull/246.
I would have preferred to write those tests as generic fsnotify tests for
fstests and let the tests pass on all filesystems and fail on overlayfs.

The reason I did not do that is because it would mean one of two things:
1. Add 2 single purpose C programs to src - This is the standard practice in
    LTP, but LTP provides a nice library to make the C test program minimal
    and fstests does not.
2. Write a multi purpose C program (e.g. fsnotify_expect) that could be
    invoked from bash tests to verify expected events.

Option #1 I ruled out.
Option #2 is on my wish/TODO list.
I envision feeding the output of fsstress into fsnotiy_expect.
I am not aware of any test coverage for fsnotify that does something like that.

Thanks,
Amir.

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Wed, Feb 14, 2018 at 12:03:31PM +0100, Jan Kara wrote:
> On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> > Stress test for fanotify and inotify. Exercise fanotify and
> > inotify user interfaces in loop while other stress tests going
> > on in the watched test directory.
> > 
> > Watching slab object inotify_inode_mark size, report fail
> > it increases too fast. This may lead to a crash if OOM killer
> > invoked.
> > 
> > kernel commit related to the fixes in v4.15-rc1:
> > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > 
> > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> 
> I'm sorry for chiming in so late but I was on vacation. Just one question:
> Currently, all inotify and fanotify tests are part of LTP. Is there any
> good reason for putting this particular test to fstests and not LTP?

It's handy to test with bash and c. fstests is more convenient to do that.

Thanks,
Xiong

> Specifically I've refrained from putting notification framework tests to
> fstests because there's practically no relation of it to implementation of
> any particular filesystem. Also I'd prefer not to have fanotify / inotify
> tests in two different frameworks...
> 
> 								Honza
> 
> > ---
> > Thanks for reviewing!
> > 
> > v4:
> >   only list one commit in the series
> > 
> > v3:
> >   add wait in cleanup
> >   add kernel commits fixed this issue in comments and commit msg
> >   fix seq numbers
> >   fix wording
> > v2:
> >   add to dangerous group
> >   new fsnotify group
> >   watch inotify_inode_mark slab instead of free memory
> >   watch inotify_inode_mark slab increasing speed instead
> >   reduce running time to 2m since we are watching the speed
> > other than numbers
> >   kill stress processes in cleanup
> > 
> >  .gitignore            |   1 +
> >  src/Makefile          |   3 +-
> >  src/fsnotify_stress.c | 339 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/478     | 185 +++++++++++++++++++++++++++
> >  tests/generic/478.out |   2 +
> >  tests/generic/group   |   1 +
> >  6 files changed, 530 insertions(+), 1 deletion(-)
> >  create mode 100644 src/fsnotify_stress.c
> >  create mode 100755 tests/generic/478
> >  create mode 100644 tests/generic/478.out
> > 
> > diff --git a/.gitignore b/.gitignore
> > index ee7eaed..ae01ed3 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -72,6 +72,7 @@
> >  /src/fill
> >  /src/fill2
> >  /src/fs_perms
> > +/src/fsnotify_stress
> >  /src/fssum
> >  /src/fstest
> >  /src/fsync-err
> > diff --git a/src/Makefile b/src/Makefile
> > index b96b8cf..6b9f296 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -14,7 +14,8 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> >  	t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
> >  	holetest t_truncate_self t_mmap_dio af_unix t_mmap_stale_pmd \
> >  	t_mmap_cow_race t_mmap_fallocate fsync-err t_mmap_write_ro \
> > -	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption
> > +	t_ext4_dax_journal_corruption t_ext4_dax_inline_corruption \
> > +	fsnotify_stress
> >  
> >  LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> >  	preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > diff --git a/src/fsnotify_stress.c b/src/fsnotify_stress.c
> > new file mode 100644
> > index 0000000..f113918
> > --- /dev/null
> > +++ b/src/fsnotify_stress.c
> > @@ -0,0 +1,339 @@
> > +#ifndef _GNU_SOURCE
> > +#define _GNU_SOURCE     /* Needed to get O_LARGEFILE definition */
> > +#endif
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +#include <poll.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <sys/fanotify.h>
> > +#include <unistd.h>
> > +#include <string.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <dirent.h>
> > +#include <sys/inotify.h>
> > +
> > +static void handle_events(int fd)
> > +{
> > +	const struct fanotify_event_metadata *metadata;
> > +	struct fanotify_event_metadata buf[200];
> > +	ssize_t len;
> > +	struct fanotify_response response;
> > +
> > +	/* Loop while events can be read from fanotify file descriptor */
> > +	for(;;) {
> > +		/* Read some events */
> > +		len = read(fd, (void *) &buf, sizeof(buf));
> > +		if (len == -1 && errno != EAGAIN) {
> > +			perror("read");
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		/* Check if end of available data reached */
> > +		if (len <= 0)
> > +			break;
> > +		/* Point to the first event in the buffer */
> > +		metadata = buf;
> > +		/* Loop over all events in the buffer */
> > +		while (FAN_EVENT_OK(metadata, len)) {
> > +			/* Check that run-time and compile-time structures match */
> > +			if (metadata->vers != FANOTIFY_METADATA_VERSION) {
> > +				fprintf(stderr,
> > +				    "Mismatch of fanotify metadata version.\n");
> > +				exit(EXIT_FAILURE);
> > +			}
> > +			if (metadata->fd >= 0) {
> > +				/* Handle open permission event */
> > +				if (metadata->mask & FAN_OPEN_PERM) {
> > +					/* Allow file to be opened */
> > +					response.fd = metadata->fd;
> > +					response.response = FAN_ALLOW;
> > +					write(fd, &response,
> > +					    sizeof(struct fanotify_response));
> > +				}
> > +				/* Handle access permission event */
> > +				if (metadata->mask & FAN_ACCESS_PERM) {
> > +					/* Allow file to be accessed */
> > +					response.fd = metadata->fd;
> > +					response.response = FAN_ALLOW;
> > +					write(fd, &response,
> > +					    sizeof(struct fanotify_response));
> > +				}
> > +				/* Close the file descriptor of the event */
> > +				close(metadata->fd);
> > +			}
> > +			/* Advance to next event */
> > +			metadata = FAN_EVENT_NEXT(metadata, len);
> > +		}
> > +	}
> > +}
> > +
> > +static int fanotify_watch(char *arg)
> > +{
> > +	int fd, poll_num;
> > +	nfds_t nfds;
> > +	struct pollfd fds[2];
> > +
> > +	/* Create the file descriptor for accessing the fanotify API */
> > +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> > +					   O_RDONLY | O_LARGEFILE);
> > +	if (fd == -1) {
> > +		perror("fanotify_init");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	/*
> > +	 * Mark the mount for:
> > +	 * - permission events before opening files
> > +	 * - notification events after closing a write-enabled
> > +	     file descriptor
> > +	 */
> > +	if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> > +			FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
> > +			FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
> > +			FAN_ONDIR | FAN_EVENT_ON_CHILD,
> > +			-1, arg) == -1) {
> > +		perror("fanotify_mark");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	/* Prepare for polling */
> > +	nfds = 1;
> > +	/* Fanotify input */
> > +	fds[0].fd = fd;
> > +	fds[0].events = POLLIN;
> > +	/* This is the loop to wait for incoming events */
> > +	while (1) {
> > +		poll_num = poll(fds, nfds, -1);
> > +		if (poll_num == -1) {
> > +			if (errno == EINTR)     /* Interrupted by a signal */
> > +				continue;           /* Restart poll() */
> > +			perror("poll");         /* Unexpected error */
> > +			exit(EXIT_FAILURE);
> > +		}
> > +		if (poll_num > 0) {
> > +			if (fds[0].revents & POLLIN) {
> > +				/* Fanotify events are available */
> > +				handle_events(fd);
> > +			}
> > +		}
> > +	}
> > +	exit(EXIT_SUCCESS);
> > +}
> > +
> > +static int fanotify_flush_stress(char *arg)
> > +{
> > +	int fd;
> > +
> > +	/* Create the file descriptor for accessing the fanotify API */
> > +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> > +					   O_RDONLY | O_LARGEFILE);
> > +	if (fd == -1) {
> > +		perror("fanotify_init");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	/* Loop marking all kinds of events and flush */
> > +	while (1) {
> > +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> > +			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
> > +			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
> > +			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
> > +			perror("fanotify_mark add");
> > +
> > +		if (fanotify_mark(fd, FAN_MARK_FLUSH | FAN_MARK_MOUNT,
> > +						0, -1, arg) == -1)
> > +			perror("fanotify_mark flush mount");
> > +
> > +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> > +			  FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM | FAN_CLOSE |
> > +			  FAN_OPEN | FAN_ACCESS_PERM | FAN_ONDIR |
> > +			  FAN_EVENT_ON_CHILD, -1, arg) == -1)
> > +			perror("fanotify_mark add");
> > +
> > +		if (fanotify_mark(fd, FAN_MARK_FLUSH, 0, -1, arg) == -1)
> > +			perror("fanotify_mark flush");
> > +	}
> > +	close(fd);
> > +	exit(EXIT_SUCCESS);
> > +}
> > +
> > +static int fanotify_init_stress(char *arg)
> > +{
> > +	int fd;
> > +
> > +	while (1) {
> > +		/* Create the file descriptor for accessing the fanotify API */
> > +		fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT |
> > +				FAN_NONBLOCK, O_RDONLY | O_LARGEFILE);
> > +		if (fd == -1)
> > +			perror("fanotify_init");
> > +		if (fanotify_mark(fd, FAN_MARK_ADD | FAN_MARK_MOUNT,
> > +				FAN_ACCESS | FAN_MODIFY | FAN_OPEN_PERM |
> > +				FAN_CLOSE | FAN_OPEN | FAN_ACCESS_PERM |
> > +				FAN_ONDIR | FAN_EVENT_ON_CHILD, -1,
> > +				arg) == -1)
> > +			perror("fanotify_mark");
> > +		close(fd);
> > +	}
> > +	exit(EXIT_SUCCESS);
> > +}
> > +
> > +static void add_mark(int fd, uint64_t mask, char *path)
> > +{
> > +	if (fanotify_mark(fd, FAN_MARK_ADD, mask, -1, path) == -1)
> > +		perror("fanotify_mark add");
> > +}
> > +
> > +static void remove_mark(int fd, uint64_t mask, char *path)
> > +{
> > +	if (fanotify_mark(fd, FAN_MARK_REMOVE, mask, -1, path) == -1)
> > +		perror("fanotify_mark remove");
> > +}
> > +
> > +static int fanotify_mark_stress(char *arg)
> > +{
> > +	int fd;
> > +
> > +	/* Create the file descriptor for accessing the fanotify API */
> > +	fd = fanotify_init(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> > +					   O_RDONLY | O_LARGEFILE);
> > +	if (fd == -1) {
> > +		perror("fanotify_init");
> > +		exit(EXIT_FAILURE);
> > +	}
> > +	/* Loop marking all kinds of events */
> > +	while (1) {
> > +		add_mark(fd, FAN_ACCESS, arg);
> > +		remove_mark(fd, FAN_ACCESS, arg);
> > +		add_mark(fd, FAN_MODIFY, arg);
> > +		remove_mark(fd, FAN_MODIFY, arg);
> > +		add_mark(fd, FAN_OPEN_PERM, arg);
> > +		remove_mark(fd, FAN_OPEN_PERM, arg);
> > +		add_mark(fd, FAN_CLOSE, arg);
> > +		remove_mark(fd, FAN_CLOSE, arg);
> > +		add_mark(fd, FAN_OPEN, arg);
> > +		remove_mark(fd, FAN_OPEN, arg);
> > +		add_mark(fd, FAN_ACCESS_PERM, arg);
> > +		remove_mark(fd, FAN_ACCESS_PERM, arg);
> > +		add_mark(fd, FAN_ONDIR, arg);
> > +		remove_mark(fd, FAN_ONDIR, arg);
> > +		add_mark(fd, FAN_EVENT_ON_CHILD, arg);
> > +		remove_mark(fd, FAN_EVENT_ON_CHILD, arg);
> > +	}
> > +
> > +	close(fd);
> > +	exit(EXIT_SUCCESS);
> > +}
> > +
> > +static int inotify_watch(char *arg)
> > +{
> > +	int notify_fd;
> > +	int wd, ret;
> > +	char *buf;
> > +
> > +	buf = malloc(sizeof(struct inotify_event) + NAME_MAX + 1);
> > +	if (buf == NULL) {
> > +		perror("malloc");
> > +		return -1;
> > +	}
> > +
> > +	notify_fd = inotify_init1(IN_CLOEXEC);
> > +	if (notify_fd == -1) {
> > +		perror("inotify_init1");
> > +		return -1;
> > +	}
> > +
> > +	wd = inotify_add_watch(notify_fd, arg,
> > +		IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE | IN_CLOSE_NOWRITE |
> > +		IN_CREATE | IN_DELETE | IN_DELETE_SELF | IN_MODIFY |
> > +		IN_MOVE_SELF | IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
> > +	if (wd < 0) {
> > +		perror("inotify_add_watch");
> > +		return -1;
> > +	}
> > +
> > +	while ((ret = read(notify_fd, buf, NAME_MAX)) != -1) {
> > +			;
> > +	}
> > +
> > +	ret = inotify_rm_watch(notify_fd, wd);
> > +	if (ret < 0)
> > +		perror("inotify_rm_watch");
> > +
> > +	close(notify_fd);
> > +	free(buf);
> > +	return 0;
> > +}
> > +
> > +static int inotify_add_rm_watch_stress(char *arg)
> > +{
> > +	int notify_fd;
> > +	int wd, ret;
> > +
> > +	notify_fd = inotify_init1(IN_CLOEXEC);
> > +	if (notify_fd == -1) {
> > +		perror("inotify_init1");
> > +		return -1;
> > +	}
> > +
> > +	while (1) {
> > +		wd = inotify_add_watch(notify_fd, arg,
> > +			IN_ACCESS | IN_ATTRIB | IN_CLOSE_WRITE |
> > +			IN_CLOSE_NOWRITE | IN_CREATE | IN_DELETE |
> > +			IN_DELETE_SELF | IN_MODIFY | IN_MOVE_SELF |
> > +			IN_MOVED_FROM | IN_MOVED_TO | IN_OPEN);
> > +		if (wd < 0)
> > +			perror("inotify_add_watch");
> > +		ret = inotify_rm_watch(notify_fd, wd);
> > +		if (ret < 0)
> > +			perror("inotify_rm_watch");
> > +	}
> > +	close(notify_fd);
> > +	return 0;
> > +}
> > +
> > +static int inotify_init_stress(void)
> > +{
> > +	int notify_fd;
> > +
> > +	while (1) {
> > +		notify_fd = inotify_init1(IN_CLOEXEC);
> > +		if (notify_fd == -1)
> > +			perror("inotify_init1");
> > +		close(notify_fd);
> > +	}
> > +	return 0;
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	pid_t pid;
> > +
> > +	if (argc != 2) {
> > +		fprintf(stderr, "Usage: %s testdir\n", argv[0]);
> > +		exit(EXIT_FAILURE);
> > +	}
> > +
> > +	if ((pid = fork()) == 0)
> > +		fanotify_watch(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		fanotify_flush_stress(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		fanotify_init_stress(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		fanotify_mark_stress(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		inotify_watch(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		inotify_add_rm_watch_stress(argv[1]);
> > +
> > +	if ((pid = fork()) == 0)
> > +		inotify_init_stress();
> > +
> > +	return 0;
> > +}
> > diff --git a/tests/generic/478 b/tests/generic/478
> > new file mode 100755
> > index 0000000..7139837
> > --- /dev/null
> > +++ b/tests/generic/478
> > @@ -0,0 +1,185 @@
> > +#! /bin/bash
> > +# FS QA Test 478
> > +#
> > +# Stress test for fanotify and inotify.
> > +#
> > +# Exercise fanotify and inotify interfaces in loop while
> > +# other stress tests going on in the watched test directory.
> > +#
> > +# Watching slab object inotify_inode_mark size, report fail
> > +# it increases too fast. This may lead to a crash if OOM killer
> > +# invoked.
> > +#
> > +# kernel commit related to the fixes in v4.15-rc1:
> > +# 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > +#
> > +#-----------------------------------------------------------------------
> > +# Copyright (c) 2018 Red Hat Inc.  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
> > +#-----------------------------------------------------------------------
> > +#
> > +
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	touch $stopfile
> > +	while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
> > +		sleep 1
> > +	done
> > +	wait
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +stopfile=$tmp.stop
> > +TIMEOUT=2m
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_supported_os Linux
> > +_require_scratch
> > +
> > +function add_files ()
> > +{
> > +	local i=$((RANDOM))
> > +
> > +	touch $SCRATCH_MNT/f-$i
> > +	ln -s $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-sym
> > +	ln $SCRATCH_MNT/f-$i $SCRATCH_MNT/f-$i-hdl
> > +	mkdir $SCRATCH_MNT/d-$i
> > +	mknod $SCRATCH_MNT/c-$i c 1 2
> > +	mknod $SCRATCH_MNT/b-$i b 1 2
> > +}
> > +
> > +function mv_files ()
> > +{
> > +	local i
> > +	for i in $SCRATCH_MNT/* ; do
> > +		mv -f f-$i f-$i-rename
> > +	done
> > +}
> > +
> > +function read_files ()
> > +{
> > +	find $SCRATCH_MNT/
> > +	cat $SCRATCH_MNT/f-*
> > +	ls -R $SCRATCH_MNT/d-*
> > +}
> > +
> > +function write_files ()
> > +{
> > +	local i
> > +	for i in $SCRATCH_MNT/* ; do
> > +		echo 1 > $i
> > +		echo 2 >> $i
> > +	done
> > +}
> > +
> > +function rm_files ()
> > +{
> > +	local i
> > +	for i in $SCRATCH_MNT/* ; do
> > +		rm -rf $i
> > +	done
> > +}
> > +
> > +slab_cal_inotify_inode_mark()
> > +{
> > +	echo 3 > /proc/sys/vm/drop_caches
> > +        local num_obj=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $3}')
> > +        local objsize=$(cat /proc/slabinfo  | grep inotify_inode_mark | awk '{print $4}')
> > +        echo $(($num_obj * $objsize))
> > +}
> > +
> > +_scratch_mkfs >>$seqres.full 2>&1
> > +_scratch_mount
> > +
> > +# inotify_inode_mark slab size before test
> > +iim_0=`slab_cal_inotify_inode_mark`
> > +
> > +NR_CPUS=$(grep -c processor /proc/cpuinfo)
> > +[ $NR_CPUS -lt 4 ] && NR_CPUS=4
> > +opts="-d $SCRATCH_MNT/ -p $NR_CPUS -n 50 -v -l 0 -c $FSSTRESS_AVOID"
> > +$FSSTRESS_PROG $opts >> $seqres.full 2>&1 &
> > +
> > +rm -f $stopfile
> > +for j in 1 2 ; do
> > +
> > +for i in `seq 1 $(($NR_CPUS/7 + 1))` ; do
> > +	$here/src/fsnotify_stress $SCRATCH_MNT &
> > +done
> > +
> > +# run read/write files operations
> > +while [ ! -e $stopfile ]; do
> > +	add_files > /dev/null 2>&1
> > +done &
> > +while [ ! -e $stopfile ]; do
> > +	mv_files > /dev/null 2>&1
> > +done &
> > +while [ ! -e $stopfile ]; do
> > +	read_files > /dev/null 2>&1
> > +done &
> > +while [ ! -e $stopfile ]; do
> > +	write_files > /dev/null 2>&1
> > +done &
> > +while [ ! -e $stopfile ]; do
> > +	rm_files > /dev/null 2>&1
> > +done &
> > +
> > +done
> > +
> > +# stressing for $TIMEOUT
> > +sleep $TIMEOUT
> > +
> > +# inotify_inode_mark slab size after test
> > +iim_1=`slab_cal_inotify_inode_mark`
> > +
> > +# cleanup stress processes
> > +touch $stopfile
> > +while killall fsnotify_stress fsstress > /dev/null 2>&1 ; do
> > +	sleep 1
> > +done
> > +
> > +# wait _files functions done
> > +wait
> > +
> > +echo $iim_0 $iim_1 $(($iim_1 - $iim_0)) >> $seqres.full
> > +# If inotify_inode_mark slab size increases 1024 times of
> > +# itself in 2m, something bad happens because it could end up
> > +# invoking OOM killer, test fails.
> > +if [ $iim_1 -gt $iim_0 ] &&
> > +   [ $((iim_1-iim_0)) -gt $((1024*$iim_0)) ] ; then
> > +	echo inotify_inode_mark slab memory leaked
> > +fi
> > +# success, all done
> > +echo "Silence is golden"
> > +status=0
> > +exit
> > diff --git a/tests/generic/478.out b/tests/generic/478.out
> > new file mode 100644
> > index 0000000..4e2107a
> > --- /dev/null
> > +++ b/tests/generic/478.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 478
> > +Silence is golden
> > diff --git a/tests/generic/group b/tests/generic/group
> > index cce03e9..8416957 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -480,3 +480,4 @@
> >  475 shutdown auto log metadata
> >  476 auto rw
> >  477 auto quick exportfs
> > +478 auto stress dangerous fsnotify
> > -- 
> > 1.8.3.1
> > 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Jan Kara]

On Thu 15-02-18 08:49:40, Xiong Zhou wrote:
> On Wed, Feb 14, 2018 at 12:03:31PM +0100, Jan Kara wrote:
> > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> > > Stress test for fanotify and inotify. Exercise fanotify and
> > > inotify user interfaces in loop while other stress tests going
> > > on in the watched test directory.
> > > 
> > > Watching slab object inotify_inode_mark size, report fail
> > > it increases too fast. This may lead to a crash if OOM killer
> > > invoked.
> > > 
> > > kernel commit related to the fixes in v4.15-rc1:
> > > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > > 
> > > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > 
> > I'm sorry for chiming in so late but I was on vacation. Just one question:
> > Currently, all inotify and fanotify tests are part of LTP. Is there any
> > good reason for putting this particular test to fstests and not LTP?
> 
> It's handy to test with bash and c. fstests is more convenient to do that.

Hum, I don't understand. You can just run the executables created by LTP.
E.g.:

jack@quack2:~/source/ltp> testcases/kernel/syscalls/inotify/inotify01 
inotify01    1  TPASS  :  get event: wd=1 mask=4 cookie=0 len=0
inotify01    2  TPASS  :  get event: wd=1 mask=20 cookie=0 len=0
inotify01    3  TPASS  :  get event: wd=1 mask=1 cookie=0 len=0
inotify01    4  TPASS  :  get event: wd=1 mask=10 cookie=0 len=0
inotify01    5  TPASS  :  get event: wd=1 mask=20 cookie=0 len=0
inotify01    6  TPASS  :  get event: wd=1 mask=2 cookie=0 len=0
inotify01    7  TPASS  :  get event: wd=1 mask=8 cookie=0 len=0
jack@quack2:~/source/ltp>

I don't see anything less handy than in fstests. I do agree that LTP is huge
so the initial step of downloading it and compiling takes longer than for
fstests but that's about it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Thu, Feb 15, 2018 at 10:23:43AM +0100, Jan Kara wrote:
> On Thu 15-02-18 08:49:40, Xiong Zhou wrote:
> > On Wed, Feb 14, 2018 at 12:03:31PM +0100, Jan Kara wrote:
> > > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> > > > Stress test for fanotify and inotify. Exercise fanotify and
> > > > inotify user interfaces in loop while other stress tests going
> > > > on in the watched test directory.
> > > > 
> > > > Watching slab object inotify_inode_mark size, report fail
> > > > it increases too fast. This may lead to a crash if OOM killer
> > > > invoked.
> > > > 
> > > > kernel commit related to the fixes in v4.15-rc1:
> > > > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > > > 
> > > > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > > 
> > > I'm sorry for chiming in so late but I was on vacation. Just one question:
> > > Currently, all inotify and fanotify tests are part of LTP. Is there any
> > > good reason for putting this particular test to fstests and not LTP?
> > 
> > It's handy to test with bash and c. fstests is more convenient to do that.
> 
> Hum, I don't understand. You can just run the executables created by LTP.

Like *_files functions and run fsstress in this patch. It's doable by c,
just with more code.

Thanks,
Xiong

> E.g.:
> 
> jack@quack2:~/source/ltp> testcases/kernel/syscalls/inotify/inotify01 
> inotify01    1  TPASS  :  get event: wd=1 mask=4 cookie=0 len=0
> inotify01    2  TPASS  :  get event: wd=1 mask=20 cookie=0 len=0
> inotify01    3  TPASS  :  get event: wd=1 mask=1 cookie=0 len=0
> inotify01    4  TPASS  :  get event: wd=1 mask=10 cookie=0 len=0
> inotify01    5  TPASS  :  get event: wd=1 mask=20 cookie=0 len=0
> inotify01    6  TPASS  :  get event: wd=1 mask=2 cookie=0 len=0
> inotify01    7  TPASS  :  get event: wd=1 mask=8 cookie=0 len=0
> jack@quack2:~/source/ltp>
> 
> I don't see anything less handy than in fstests. I do agree that LTP is huge
> so the initial step of downloading it and compiling takes longer than for
> fstests but that's about it.
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Jan Kara]

On Fri 16-02-18 08:38:23, Xiong Zhou wrote:
> On Thu, Feb 15, 2018 at 10:23:43AM +0100, Jan Kara wrote:
> > On Thu 15-02-18 08:49:40, Xiong Zhou wrote:
> > > On Wed, Feb 14, 2018 at 12:03:31PM +0100, Jan Kara wrote:
> > > > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> > > > > Stress test for fanotify and inotify. Exercise fanotify and
> > > > > inotify user interfaces in loop while other stress tests going
> > > > > on in the watched test directory.
> > > > > 
> > > > > Watching slab object inotify_inode_mark size, report fail
> > > > > it increases too fast. This may lead to a crash if OOM killer
> > > > > invoked.
> > > > > 
> > > > > kernel commit related to the fixes in v4.15-rc1:
> > > > > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > > > > 
> > > > > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > > > 
> > > > I'm sorry for chiming in so late but I was on vacation. Just one question:
> > > > Currently, all inotify and fanotify tests are part of LTP. Is there any
> > > > good reason for putting this particular test to fstests and not LTP?
> > > 
> > > It's handy to test with bash and c. fstests is more convenient to do that.
> > 
> > Hum, I don't understand. You can just run the executables created by LTP.
> 
> Like *_files functions and run fsstress in this patch. It's doable by c,
> just with more code.

I see thanks for explanation. That's a fair point but LTP also has support
for testcases in shell. So you could do basically what you did for fstests
in LTP as well. See e.g. how testcases/network/nfs/nfs_stress/nfs06 uses
fsstress.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Fri, Feb 16, 2018 at 10:52:20AM +0100, Jan Kara wrote:
> On Fri 16-02-18 08:38:23, Xiong Zhou wrote:
> > On Thu, Feb 15, 2018 at 10:23:43AM +0100, Jan Kara wrote:
> > > On Thu 15-02-18 08:49:40, Xiong Zhou wrote:
> > > > On Wed, Feb 14, 2018 at 12:03:31PM +0100, Jan Kara wrote:
> > > > > On Mon 12-02-18 13:46:48, Xiong Zhou wrote:
> > > > > > Stress test for fanotify and inotify. Exercise fanotify and
> > > > > > inotify user interfaces in loop while other stress tests going
> > > > > > on in the watched test directory.
> > > > > > 
> > > > > > Watching slab object inotify_inode_mark size, report fail
> > > > > > it increases too fast. This may lead to a crash if OOM killer
> > > > > > invoked.
> > > > > > 
> > > > > > kernel commit related to the fixes in v4.15-rc1:
> > > > > > 0d6ec07 fsnotify: pin both inode and vfsmount mark
> > > > > > 
> > > > > > Signed-off-by: Xiong Zhou <xzhou@redhat.com>
> > > > > 
> > > > > I'm sorry for chiming in so late but I was on vacation. Just one question:
> > > > > Currently, all inotify and fanotify tests are part of LTP. Is there any
> > > > > good reason for putting this particular test to fstests and not LTP?
> > > > 
> > > > It's handy to test with bash and c. fstests is more convenient to do that.
> > > 
> > > Hum, I don't understand. You can just run the executables created by LTP.
> > 
> > Like *_files functions and run fsstress in this patch. It's doable by c,
> > just with more code.
> 
> I see thanks for explanation. That's a fair point but LTP also has support
> for testcases in shell. So you could do basically what you did for fstests
> in LTP as well. See e.g. how testcases/network/nfs/nfs_stress/nfs06 uses
> fsstress.

Thanks for the direction!

Xiong
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Xiong Zhou]

On Mon, Feb 12, 2018 at 12:33:57PM +0800, Xiong Zhou wrote:
> On Mon, Feb 12, 2018 at 05:50:21AM +0200, Amir Goldstein wrote:
> > On Mon, Feb 12, 2018 at 2:41 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> > > On Sun, Feb 11, 2018 at 10:03:17PM +0200, Amir Goldstein wrote:
> > >> On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> > >> > Stress test for fanotify and inotify. Exercise fanotify and
> > >> > inotify user interfaces in loop while other stress tests going
> > >> > on in the watched test directory.
> > >> >
> > >> > Watching slab object inotify_inode_mark size, report fail
> > >> > it increases too fast. This may lead to a crash if OOM killer
> > >> > invoked. Fixed by this series:
> > >> >
> > >> > ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
> > >> > 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
> > >> > 3427ce7 fsnotify: clean up fsnotify()
> > >> > f37650f fanotify: fix fsnotify_prepare_user_wait() failure
> > >> > 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
> > >>
> > >> That's a bit much. This commit below is the actual fix.
> > >> The rest are cleanups and other bug fixes.
> > >
> > > Cleanups are preparing for the fix.
> > 
> > No they are not. They are general cleanups and other bug fixed *after*
> > the fix. heck, 3 of the patches you listed are not from Miklos' patchset
> > at all.
> 
> Ya. I messed up, I did not check all the details well enough, just
> confirmed there was dependency.

Initially I was checking email and saw
http://lkml.iu.edu/hypermail/linux/kernel/1710.3/01317.html
So I grabbed some commits from git log.

I am not sure which specific commit fixes that issue, or all of them is
needed, so I listed the series for accuracy(lazy).

You think it's too many and may confuse people. I think it's better
to make it clear that fix comes from a series, if you want to
revert or cherry-pick etc, be careful.

Thanks,
Xiong

> 
> > 
> > If you wanted to include also prep patches that would only be:
> > 
> > 24c2030 fsnotify: clean up fsnotify_prepare/finish_user_wait()
> > 
> > But I personally find that information to be irrelevant in test context.
> > 
> > > I believe it's better to list
> > > the whole series. They are a patchset for some reason.
> > 
> > You gotta know who your audience are.
> 
> Yes, I know.
> 
> > Patchset is for a reason - the reason is that all patches should be
> > reviewed and merged together. But that's got nothing to do with the
> > reason you should list the fix commit in test description.
> > The reason for that is that testers needs to know if test is expected to
> > fail or pass on the kernel they are using.
> 
> AFAIK, known issue tracking is not handled by fstests. Listing them
> is helping make things more clear, not for telling people expected
> results. If I post this case before any fix commits available, not
> listing any commits does make sense.
> 
> > 
> > Listing the cleanup patches does not serve this purpose.
> > In fact, it may confuse people testing stable kernels, because stable
> > kernel could have fix patches applied but not all cleanup patches.
> 
> Interesting, I'm listing all of them and I'm saying "fixed by this series"
> not "fixed by this commit".
> 
> THanks,
> Xiong
> > 
> > 
> > >
> > >> It could also help testers to know the upstream kernel version
> > >> where the fix landed:
> > >
> > > There is no such info in other cases listing kernel commits. If
> > > people want to know, they will find it out.
> > >
> > 
> > Fair enough. Looking at other tests for common practices is a good
> > idea - the practice is to list only the fix commits. There are a few tests
> > that list more than a single commit, but in those cases the bug fix itself
> > is probably spread over few commits.
> > 
> > Cheers,
> > Amir.

Re: [PATCH v3] generic: add stress test for fanotify and inotify

[Amir Goldstein]

On Mon, Feb 12, 2018 at 7:17 AM, Xiong Zhou <xzhou@redhat.com> wrote:
> On Mon, Feb 12, 2018 at 12:33:57PM +0800, Xiong Zhou wrote:
>> On Mon, Feb 12, 2018 at 05:50:21AM +0200, Amir Goldstein wrote:
>> > On Mon, Feb 12, 2018 at 2:41 AM, Xiong Zhou <xzhou@redhat.com> wrote:
>> > > On Sun, Feb 11, 2018 at 10:03:17PM +0200, Amir Goldstein wrote:
>> > >> On Sun, Feb 11, 2018 at 8:59 AM, Xiong Zhou <xzhou@redhat.com> wrote:
>> > >> > Stress test for fanotify and inotify. Exercise fanotify and
>> > >> > inotify user interfaces in loop while other stress tests going
>> > >> > on in the watched test directory.
>> > >> >
>> > >> > Watching slab object inotify_inode_mark size, report fail
>> > >> > it increases too fast. This may lead to a crash if OOM killer
>> > >> > invoked. Fixed by this series:
>> > >> >
>> > >> > ab97f87 fsnotify: convert fsnotify_mark.refcnt from atomic_t to ref..
>> > >> > 6685df3 fanotify: clean up CONFIG_FANOTIFY_ACCESS_PERMISSIONS ifdefs
>> > >> > 3427ce7 fsnotify: clean up fsnotify()
>> > >> > f37650f fanotify: fix fsnotify_prepare_user_wait() failure
>> > >> > 9a31d7a fsnotify: fix pinning group in fsnotify_prepare_user_wait()
>> > >>
>> > >> That's a bit much. This commit below is the actual fix.
>> > >> The rest are cleanups and other bug fixes.
>> > >
>> > > Cleanups are preparing for the fix.
>> >
>> > No they are not. They are general cleanups and other bug fixed *after*
>> > the fix. heck, 3 of the patches you listed are not from Miklos' patchset
>> > at all.
>>
>> Ya. I messed up, I did not check all the details well enough, just
>> confirmed there was dependency.
>
> Initially I was checking email and saw
> http://lkml.iu.edu/hypermail/linux/kernel/1710.3/01317.html
> So I grabbed some commits from git log.
>
> I am not sure which specific commit fixes that issue, or all of them is
> needed, so I listed the series for accuracy(lazy).
>

Your confusion is understandable.
The cover letter says that with "the patch" applied, your reproducer does
not fail. However, with v2 "the patch" (which was the first patch of v1) was
split into 3 patches, i.e.:
fsnotify: clean up fsnotify_prepare/finish_user_wait()
fsnotify: pin both inode and vfsmount mark
fsnotify: fix pinning group in fsnotify_prepare_user_wait()

Out of these 3, listing the last 2 would have been informative enough.

Anyway, I find your wording in V4 informative and concise to the right level.

While we are being prudent about associating a fix with the test, can anyone
explain why the fixed bugs cause a memory leak, which is what this
test checks for?
because both relevant fixes claim to fix use after free.

Thanks,
Amir.