Re: [LTP] [PATCH v4] kernel/fs/fsnotify-stress: fsnotify stress test

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Murphy Zhou <jencce.kernel@gmail.com> writes:

> This is a stress test that exercises fanotify and inotify interfaces
> while IO going on. It intentionally ignores failures or return values
> of some syscalls to let the stress go on. If the kernel does not panic
> or hang after a certain period of time of testing, test pass.
>
> Signed-off-by: Murphy Zhou <jencce.kernel@gmail.com>
> ---
> v3 -> v4:
> 	Convert comment to docparse part.
>
>  runtest/fs                                    |   2 +
>  testcases/kernel/fs/fsnotify-stress/Makefile  |   9 +
>  .../fs/fsnotify-stress/fsnotify-stress.c      | 476 ++++++++++++++++++
>  3 files changed, 487 insertions(+)
>  create mode 100644 testcases/kernel/fs/fsnotify-stress/Makefile
>  create mode 100644 testcases/kernel/fs/fsnotify-stress/fsnotify-stress.c
>
> diff --git a/runtest/fs b/runtest/fs
> index 1d753e0dd..beb43aae4 100644
> --- a/runtest/fs
> +++ b/runtest/fs
> @@ -87,3 +87,5 @@ binfmt_misc01 binfmt_misc01.sh
>  binfmt_misc02 binfmt_misc02.sh
>  
>  squashfs01 squashfs01
> +
> +fsnotify-stress fsnotify-stress
> diff --git a/testcases/kernel/fs/fsnotify-stress/Makefile b/testcases/kernel/fs/fsnotify-stress/Makefile
> new file mode 100644
> index 000000000..451f791f1
> --- /dev/null
> +++ b/testcases/kernel/fs/fsnotify-stress/Makefile
> @@ -0,0 +1,9 @@
> +#
> +#    kernel/fs/fs-notify testcases Makefile.
> +#
> +
> +top_srcdir	?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/fs/fsnotify-stress/fsnotify-stress.c b/testcases/kernel/fs/fsnotify-stress/fsnotify-stress.c
> new file mode 100644
> index 000000000..8130f7f12
> --- /dev/null
> +++ b/testcases/kernel/fs/fsnotify-stress/fsnotify-stress.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022 Red Hat, Inc.  All Rights Reserved.
> + * Author: Murphy Zhou <jencce.kernel@gmail.com>
> + * Copyright (c) Linux Test Project, 2001-2022
> + */
> +
> +/*\
> + * [Description]
> + *
> + * This is an irregular stress test for Linux kernel fanotify/inotify
> + * interfaces. It calls thoese interfaces with possible best coverage
> + * arguments, in a loop. It ignores some return values in the loop to
> + * let the stress going on. At the same time, it initiates IO traffics
> + * by calling IO syscalls.
> + *
> + * If kernel does no panic or hang after the test, test pass.
> + *
> + * It detected a leak in fsnotify code which was fixed by Amir through
> + * this Linux commit:
> + *     4396a731 fsnotify: fix sb_connectors leak

The problem with stress tests is that they are expensive to run. If they
do fail then it's often difficult to reproduce the errors. Eventually
they just get added to a skip list.

Why not make a reproducer for this bug which executes in the minimum
time necessary?

As well as saving CPU time and avoiding random timeouts this helps
create a better understanding of what really matters in the test.

> + *
> + */
> +
> +#define _GNU_SOURCE     /* Needed to get O_LARGEFILE definition */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <poll.h>
> +#include <sys/fanotify.h>
> +#include <sys/inotify.h>
> +#include <sys/time.h>
> +#include <unistd.h>
> +#include <string.h>
> +
> +#include "tst_test.h"
> +#include "../../syscalls/fanotify/fanotify.h"
> +#include "../../syscalls/inotify/inotify.h"
> +
> +static int fd0;
> +
> +#define TESTDIR "testdir"
> +#define TESTFILE "testdir/file"
> +
> +static void cleanup(void)
> +{
> +	if (fd0 > 0) {
> +		SAFE_CLOSE(fd0);
> +	}
> +}
> +
> +static void setup(void)
> +{
> +	SAFE_MKDIR(TESTDIR, 0777);
> +	fd0 = SAFE_OPEN(TESTFILE, O_CREAT|O_RDWR, 0666);
> +}
> +
> +static void fanotify_flushes(char *fn)
> +{
> +	int fd;
> +
> +	fd = SAFE_FANOTIFY_INIT(FAN_CLOEXEC | FAN_CLASS_CONTENT | FAN_NONBLOCK,
> +					   O_RDONLY | O_LARGEFILE);
> +
> +	while (tst_remaining_runtime() > 10) {
> +		/* As a stress test, we ignore the return values here to
> +		 * proceed with the stress.
> +		 */

The LTP style guide forbids inline comments unless they are explaining
something that would be very difficult to understand without them. There
are lots of comments like this in the test.

> +		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, fn);
> +

...

> +static void readfiles(char *fn)
> +{
> +	int fd;
> +	char buf[BUFSIZ];
> +
> +	memset(buf, 1, BUFSIZ);
> +	while (tst_remaining_runtime() > 10) {
> +		fd = open(fn, O_RDONLY);

If this fails then what are we stressing? We could just be testing
spinning in a loop.

> +		if (fd == -1)
> +			continue;
> +		read(fd, buf, BUFSIZ);

Also ignoring the result of read results in compiler warnings.

> +
> +static struct tst_test test = {
> +	.tcnt = 1,
> +	.max_runtime = 60,

Does it need 60 seconds to reproduce the bug?
Why does it need this long to achieve the desired coverage?

Without further evidence I'd assume that full coverage (ignoring setup)
is approached after <1 second on a reasonable system.

Also we should limit the number of iterations.

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp