[LTP] [PATCH v2] Add read_all file systems test

[LTP] [PATCH v2] Add read_all file systems test

[Richard Palethorpe]

While using a shell script I wrote, which dumps the contents of /sys and
/proc (submitted in a separate patch), I found some minor kernel bugs. There
is already a test specifically for /proc however it is attempting to verify
the behavior of particular files. This test is more general and can be applied
to any file system.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V2: Use a queue structure instead of pipes for distributing work as well as
    many other small changes.

 runtest/fs                              |   4 +
 testcases/kernel/fs/read_all/.gitignore |   1 +
 testcases/kernel/fs/read_all/Makefile   |  23 ++
 testcases/kernel/fs/read_all/read_all.c | 395 ++++++++++++++++++++++++++++++++
 4 files changed, 423 insertions(+)
 create mode 100644 testcases/kernel/fs/read_all/.gitignore
 create mode 100644 testcases/kernel/fs/read_all/Makefile
 create mode 100644 testcases/kernel/fs/read_all/read_all.c

diff --git a/runtest/fs b/runtest/fs
index 3fa210a9f..a595edb98 100644
--- a/runtest/fs
+++ b/runtest/fs
@@ -69,6 +69,10 @@ fs_di fs_di -d $TMPDIR
 # Was not sure why it should reside in runtest/crashme and won´t get tested ever
 proc01 proc01 -m 128
 
+read_all_dev read_all -d /dev -q -r 10
+read_all_proc read_all -d /proc -q -r 10
+read_all_sys read_all -d /sys -q -r 10
+
 #Run the File System Race Condition Check tests as well
 fs_racer fs_racer.sh -t 5
 
diff --git a/testcases/kernel/fs/read_all/.gitignore b/testcases/kernel/fs/read_all/.gitignore
new file mode 100644
index 000000000..ac2f6ae71
--- /dev/null
+++ b/testcases/kernel/fs/read_all/.gitignore
@@ -0,0 +1 @@
+read_all
diff --git a/testcases/kernel/fs/read_all/Makefile b/testcases/kernel/fs/read_all/Makefile
new file mode 100644
index 000000000..efb223435
--- /dev/null
+++ b/testcases/kernel/fs/read_all/Makefile
@@ -0,0 +1,23 @@
+# Copyright (c) 2017 Linux Test Project
+#
+# 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; either version 2 of
+# the License, or (at your option) any later version.
+#
+# 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, see <http://www.gnu.org/licenses/>.
+
+top_srcdir		?= ../../../..
+
+include $(top_srcdir)/include/mk/testcases.mk
+
+CFLAGS			+= -D_GNU_SOURCE
+LDFLAGS 		+= -lpthread
+
+include $(top_srcdir)/include/mk/generic_leaf_target.mk
diff --git a/testcases/kernel/fs/read_all/read_all.c b/testcases/kernel/fs/read_all/read_all.c
new file mode 100644
index 000000000..6f3fe09cc
--- /dev/null
+++ b/testcases/kernel/fs/read_all/read_all.c
@@ -0,0 +1,395 @@
+/*
+ * Copyright (c) 2017 Richard Palethorpe <rpalethorpe@suse.com>
+ *
+ * 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, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will 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, see <http://www.gnu.org/licenses/>.
+ */
+/*
+ * Perform a small read on every file in a directory tree.
+ *
+ * Useful for testing file systems like proc, sysfs and debugfs or anything
+ * which exposes a file like API so long as it respects O_NONBLOCK. This test
+ * is not concerned if a particular file in one of these file systems conforms
+ * exactly to its specific documented behavior. Just whether reading from that
+ * file causes a serious error such as a NULL pointer dereference.
+ *
+ * It is not required to run this as root, but test coverage will be much
+ * higher with full privileges.
+ *
+ * The reads are preformed by worker processes which are given file paths by a
+ * single parent process. The parent process recursively scans a given
+ * directory and passes the file paths it finds to the child processes using a
+ * queue structure stored in shared memory.
+ *
+ * This allows the file system and individual files to be accessed in
+ * parallel. Passing the repeat parameter (-r) will encourage this.
+ */
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <dirent.h>
+#include <errno.h>
+#include <unistd.h>
+#include <string.h>
+#include <limits.h>
+#include <fnmatch.h>
+#include <semaphore.h>
+#include <ctype.h>
+
+#include "tst_test.h"
+
+#define QUEUE_SIZE 16384
+#define BUFFER_SIZE 1024
+#define MAX_PATH 4096
+#define MAX_DISPLAY 40
+
+struct queue {
+	sem_t sem;
+	int front;
+	int back;
+	char data[QUEUE_SIZE];
+};
+
+struct worker {
+	pid_t pid;
+	struct queue *q;
+};
+
+enum dent_action {
+	DA_UNKNOWN,
+	DA_IGNORE,
+	DA_READ,
+	DA_VISIT,
+};
+
+static char *verbose;
+static char *quite;
+static char *root_dir;
+static char *exclude;
+static char *str_repeat;
+static int repeat = 1;
+static long worker_count;
+static struct worker *workers;
+
+static struct tst_option options[] = {
+	{"v", &verbose,
+	 "-v       Print information about successful reads"},
+	{"q", &quite,
+	 "-q       Don't print file read or open errors"},
+	{"d:", &root_dir,
+	 "-d path  Path to the directory to read from, defaults to /sys"},
+	{"e:", &exclude,
+	 "-e pattern Ignore files which match an 'extended' pattern, see fnmatch(3)"},
+	{"r:", &str_repeat,
+	 "-r count The number of times to schedule a file for reading"},
+	{NULL, NULL, NULL}
+};
+
+static int queue_pop(struct queue *q, char *buf)
+{
+	int i = q->front, j = 0;
+
+	sem_wait(&q->sem);
+
+	if (!q->data[i])
+		return 0;
+
+	while (q->data[i]) {
+		buf[j] = q->data[i];
+
+		if (++j >= BUFFER_SIZE - 1)
+			tst_brk(TBROK, "Buffer is too small for path");
+		if (++i >= QUEUE_SIZE)
+			i = 0;
+	}
+
+	buf[j] = '\0';
+	tst_atomic_store(i + 1, &q->front);
+
+	return 1;
+}
+
+static int queue_push(struct queue *q, const char *buf)
+{
+	int i = q->back, j = 0;
+	int front = tst_atomic_load(&q->front);
+
+	while (buf[j]) {
+		q->data[i] = buf[j];
+
+		++j;
+		if (++i >= QUEUE_SIZE)
+			i = 0;
+		if (i == front)
+			return 0;
+	}
+	q->data[i] = '\0';
+
+	q->back = i + 1;
+	sem_post(&q->sem);
+
+	return 1;
+}
+
+static struct queue *queue_init(void)
+{
+	struct queue *q = SAFE_MMAP(NULL, sizeof(*q),
+				    PROT_READ | PROT_WRITE,
+				    MAP_SHARED | MAP_ANONYMOUS,
+				    0, 0);
+
+	sem_init(&q->sem, 1, 0);
+	q->front = 0;
+	q->back = 0;
+
+	return q;
+}
+
+static void queue_destroy(struct queue *q, int is_worker)
+{
+	if (is_worker)
+		sem_destroy(&q->sem);
+
+	SAFE_MUNMAP(q, sizeof(*q));
+}
+
+static void sanitize_str(char *buf, ssize_t count)
+{
+	int i;
+
+	for (i = 0; i < MIN(count, MAX_DISPLAY); i++)
+		if (!isprint(buf[i]))
+			buf[i] = ' ';
+
+	if (count <= MAX_DISPLAY)
+		buf[count] = '\0';
+	else
+		strcpy(buf + MAX_DISPLAY, "...");
+}
+
+static void read_test(const char *path)
+{
+	char buf[BUFFER_SIZE];
+	int fd;
+	ssize_t count;
+
+	if (exclude && !fnmatch(exclude, path, FNM_EXTMATCH)) {
+		if (verbose)
+			tst_res(TINFO, "Ignoring %s", path);
+		return;
+	}
+
+	if (verbose)
+		tst_res(TINFO, "%s(%s)", __func__, path);
+
+	fd = open(path, O_RDONLY | O_NONBLOCK);
+	if (fd < 0 && !quite) {
+		tst_res(TINFO | TERRNO, "open(%s)", path);
+		return;
+	} else if (fd < 0)
+		return;
+
+	count = read(fd, buf, sizeof(buf) - 1);
+	if (count > 0 && verbose) {
+		sanitize_str(buf, count);
+		tst_res(TINFO, "read(%s, buf) = %ld, buf = %s",
+			path, count, buf);
+	} else if (!count && verbose)
+		tst_res(TINFO, "read(%s) = EOF", path);
+	else if (count < 0 && !quite)
+		tst_res(TINFO | TERRNO, "read(%s)", path);
+
+	SAFE_CLOSE(fd);
+}
+
+static int worker_run(struct worker *self)
+{
+	int ret;
+	char buf[BUFFER_SIZE];
+	struct sigaction term_sa = {
+		.sa_handler = SIG_IGN,
+		.sa_flags = 0,
+	};
+	struct queue *q = self->q;
+
+	sigaction(SIGTTIN, &term_sa, NULL);
+
+	for (ret = queue_pop(q, buf); ret; ret = queue_pop(q, buf))
+		read_test(buf);
+
+	queue_destroy(q, 1);
+	fflush(stdout);
+	return 0;
+}
+
+static void spawn_workers(void)
+{
+	int i;
+	struct worker *wa = workers;
+
+	bzero(workers, worker_count * sizeof(*workers));
+
+	for (i = 0; i < worker_count; i++) {
+		wa[i].q = queue_init();
+		wa[i].pid = SAFE_FORK();
+		if (!wa[i].pid)
+			exit(worker_run(wa + i));
+	}
+}
+
+static void stop_workers(void)
+{
+	const char stop_code[1] = { '\0' };
+	int i;
+
+	if (!workers)
+		return;
+
+	for (i = 0; i < worker_count; i++)
+		if (workers[i].q)
+			queue_push(workers[i].q, stop_code);
+
+	for (i = 0; i < worker_count; i++)
+		if (workers[i].q) {
+			queue_destroy(workers[i].q, 0);
+			workers[i].q = 0;
+		}
+}
+
+static void sched_work(const char *path)
+{
+	static int cur;
+	int push_attempts = 0, pushed;
+
+	while (1) {
+		pushed = queue_push(workers[cur].q, path);
+
+		if (++cur >= worker_count)
+			cur = 0;
+
+		if (pushed)
+			break;
+
+		if (++push_attempts > worker_count) {
+			usleep(100);
+			push_attempts = 0;
+		}
+	}
+}
+
+static void rep_sched_work(const char *path, int rep)
+{
+	int i;
+
+	for (i = 0; i < rep; i++)
+		sched_work(path);
+}
+
+static void setup(void)
+{
+	if (tst_parse_int(str_repeat, &repeat, 1, INT_MAX))
+		tst_brk(TBROK,
+			"Invalid repeat (-r) argument: '%s'", str_repeat);
+
+	if (!root_dir)
+		tst_brk(TBROK, "The directory argument (-d) is required");
+
+	worker_count = MIN(MAX(SAFE_SYSCONF(_SC_NPROCESSORS_ONLN) - 1, 1), 15);
+	workers = SAFE_MALLOC(worker_count * sizeof(*workers));
+}
+
+static void cleanup(void)
+{
+	stop_workers();
+	free(workers);
+}
+
+static void visit_dir(const char *path)
+{
+	DIR *dir;
+	struct dirent *dent;
+	struct stat dent_st;
+	char dent_path[MAX_PATH];
+	enum dent_action act;
+
+	dir = opendir(path);
+	if (!dir) {
+		tst_res(TINFO | TERRNO, "opendir(%s)", path);
+		return;
+	}
+
+	while (1) {
+		errno = 0;
+		dent = readdir(dir);
+		if (!dent && errno) {
+			tst_res(TINFO | TERRNO, "readdir(%s)", path);
+			break;
+		} else if (!dent)
+			break;
+
+		if (!strcmp(dent->d_name, ".") ||
+		    !strcmp(dent->d_name, ".."))
+			continue;
+
+		if (dent->d_type == DT_DIR)
+			act = DA_VISIT;
+		else if (dent->d_type == DT_LNK)
+			act = DA_IGNORE;
+		else if (dent->d_type == DT_UNKNOWN)
+			act = DA_UNKNOWN;
+		else
+			act = DA_READ;
+
+		snprintf(dent_path, MAX_PATH,
+			 "%s/%s", path, dent->d_name);
+
+		if (act == DA_UNKNOWN) {
+			if (lstat(dent_path, &dent_st))
+				tst_res(TINFO | TERRNO, "lstat(%s)", path);
+			else if ((dent_st.st_mode & S_IFMT) == S_IFDIR)
+				act = DA_VISIT;
+			else if ((dent_st.st_mode & S_IFMT) == S_IFLNK)
+				act = DA_IGNORE;
+			else
+				act = DA_READ;
+		}
+
+		if (act == DA_VISIT)
+			visit_dir(dent_path);
+		else if (act == DA_READ)
+			rep_sched_work(dent_path, repeat);
+	}
+
+	if (closedir(dir))
+		tst_res(TINFO | TERRNO, "closedir(%s)", path);
+}
+
+static void run(void)
+{
+	spawn_workers();
+	visit_dir(root_dir);
+	stop_workers();
+
+	tst_reap_children();
+	tst_res(TPASS, "Finished reading files");
+}
+
+static struct tst_test test = {
+	.options = options,
+	.setup = setup,
+	.cleanup = cleanup,
+	.test_all = run,
+	.forks_child = 1,
+};
+
-- 
2.15.1

[LTP] [PATCH v2] Add read_all file systems test

[Cyril Hrubis]

Hi!
> +#define QUEUE_SIZE 16384
> +#define BUFFER_SIZE 1024
> +#define MAX_PATH 4096
> +#define MAX_DISPLAY 40
> +
> +struct queue {
> +	sem_t sem;
> +	int front;
> +	int back;
> +	char data[QUEUE_SIZE];
> +};
> +
> +struct worker {
> +	pid_t pid;
> +	struct queue *q;
> +};
> +
> +enum dent_action {
> +	DA_UNKNOWN,
> +	DA_IGNORE,
> +	DA_READ,
> +	DA_VISIT,
> +};
> +
> +static char *verbose;
> +static char *quite;
                  ^
		quiet?

> +static char *root_dir;
> +static char *exclude;
> +static char *str_repeat;
> +static int repeat = 1;
> +static long worker_count;
> +static struct worker *workers;
> +
> +static struct tst_option options[] = {
> +	{"v", &verbose,
> +	 "-v       Print information about successful reads"},
> +	{"q", &quite,
> +	 "-q       Don't print file read or open errors"},
> +	{"d:", &root_dir,
> +	 "-d path  Path to the directory to read from, defaults to /sys"},
> +	{"e:", &exclude,
> +	 "-e pattern Ignore files which match an 'extended' pattern, see fnmatch(3)"},
> +	{"r:", &str_repeat,
> +	 "-r count The number of times to schedule a file for reading"},

I'm not sure repeat is the right name for parameter, I do not have much
better idea though, maybe iteration.

> +	{NULL, NULL, NULL}
> +};
> +
> +static int queue_pop(struct queue *q, char *buf)
> +{
> +	int i = q->front, j = 0;
> +
> +	sem_wait(&q->sem);
> +
> +	if (!q->data[i])
> +		return 0;
> +
> +	while (q->data[i]) {
> +		buf[j] = q->data[i];
> +
> +		if (++j >= BUFFER_SIZE - 1)
> +			tst_brk(TBROK, "Buffer is too small for path");
> +		if (++i >= QUEUE_SIZE)
> +			i = 0;
> +	}
> +
> +	buf[j] = '\0';
> +	tst_atomic_store(i + 1, &q->front);
> +
> +	return 1;
> +}
> +
> +static int queue_push(struct queue *q, const char *buf)
> +{
> +	int i = q->back, j = 0;
> +	int front = tst_atomic_load(&q->front);
> +
> +	while (buf[j]) {
> +		q->data[i] = buf[j];
> +
> +		++j;
> +		if (++i >= QUEUE_SIZE)
> +			i = 0;
> +		if (i == front)
> +			return 0;
> +	}
> +	q->data[i] = '\0';
> +
> +	q->back = i + 1;
> +	sem_post(&q->sem);
> +
> +	return 1;
> +}
> +
> +static struct queue *queue_init(void)
> +{
> +	struct queue *q = SAFE_MMAP(NULL, sizeof(*q),
> +				    PROT_READ | PROT_WRITE,
> +				    MAP_SHARED | MAP_ANONYMOUS,
> +				    0, 0);
> +
> +	sem_init(&q->sem, 1, 0);
> +	q->front = 0;
> +	q->back = 0;
> +
> +	return q;
> +}
> +
> +static void queue_destroy(struct queue *q, int is_worker)
> +{
> +	if (is_worker)
> +		sem_destroy(&q->sem);
> +
> +	SAFE_MUNMAP(q, sizeof(*q));
> +}
> +
> +static void sanitize_str(char *buf, ssize_t count)
> +{
> +	int i;
> +
> +	for (i = 0; i < MIN(count, MAX_DISPLAY); i++)
> +		if (!isprint(buf[i]))
> +			buf[i] = ' ';
> +
> +	if (count <= MAX_DISPLAY)
> +		buf[count] = '\0';
> +	else
> +		strcpy(buf + MAX_DISPLAY, "...");
> +}
> +
> +static void read_test(const char *path)
> +{
> +	char buf[BUFFER_SIZE];
> +	int fd;
> +	ssize_t count;
> +
> +	if (exclude && !fnmatch(exclude, path, FNM_EXTMATCH)) {
> +		if (verbose)
> +			tst_res(TINFO, "Ignoring %s", path);
> +		return;
> +	}
> +
> +	if (verbose)
> +		tst_res(TINFO, "%s(%s)", __func__, path);
> +
> +	fd = open(path, O_RDONLY | O_NONBLOCK);
> +	if (fd < 0 && !quite) {
> +		tst_res(TINFO | TERRNO, "open(%s)", path);
> +		return;
> +	} else if (fd < 0)
> +		return;

I guess that this would be a bit more readable:


	if (fd < 0) {
		if (!quiet)
			tst_res(...)
		return;
	}

> +	count = read(fd, buf, sizeof(buf) - 1);
> +	if (count > 0 && verbose) {
> +		sanitize_str(buf, count);
> +		tst_res(TINFO, "read(%s, buf) = %ld, buf = %s",
> +			path, count, buf);
> +	} else if (!count && verbose)
> +		tst_res(TINFO, "read(%s) = EOF", path);
> +
> +	else if (count < 0 && !quite)
> +		tst_res(TINFO | TERRNO, "read(%s)", path);
> +
> +	SAFE_CLOSE(fd);
> +}
> +
> +static int worker_run(struct worker *self)
> +{
> +	int ret;
> +	char buf[BUFFER_SIZE];
> +	struct sigaction term_sa = {
> +		.sa_handler = SIG_IGN,
> +		.sa_flags = 0,
> +	};
> +	struct queue *q = self->q;
> +
> +	sigaction(SIGTTIN, &term_sa, NULL);
> +
> +	for (ret = queue_pop(q, buf); ret; ret = queue_pop(q, buf))
> +		read_test(buf);

As much as I do like to strech for loop for everything this would be
probably more elegant as an infinite loop with a break.

	for (;;) {
		if (!queue_pop(q, buf))
			break;

		read_test(buf);
	}

> +	queue_destroy(q, 1);
> +	fflush(stdout);

What are trying to flush here? The tst_res() messages are printed
into the stderr btw.

> +	return 0;
> +}
> +
> +static void spawn_workers(void)
> +{
> +	int i;
> +	struct worker *wa = workers;
> +
> +	bzero(workers, worker_count * sizeof(*workers));
> +
> +	for (i = 0; i < worker_count; i++) {
> +		wa[i].q = queue_init();
> +		wa[i].pid = SAFE_FORK();
> +		if (!wa[i].pid)
> +			exit(worker_run(wa + i));
> +	}
> +}
> +
> +static void stop_workers(void)
> +{
> +	const char stop_code[1] = { '\0' };
> +	int i;
> +
> +	if (!workers)
> +		return;
> +
> +	for (i = 0; i < worker_count; i++)
> +		if (workers[i].q)
> +			queue_push(workers[i].q, stop_code);
> +
> +	for (i = 0; i < worker_count; i++)
> +		if (workers[i].q) {
> +			queue_destroy(workers[i].q, 0);
> +			workers[i].q = 0;
> +		}

FYI LKML coding style preferes curly braces around the block in a case
that it spans over multiple lines.

> +}
> +
> +static void sched_work(const char *path)
> +{
> +	static int cur;
> +	int push_attempts = 0, pushed;
> +
> +	while (1) {
> +		pushed = queue_push(workers[cur].q, path);
> +
> +		if (++cur >= worker_count)
> +			cur = 0;
> +
> +		if (pushed)
> +			break;
> +
> +		if (++push_attempts > worker_count) {
> +			usleep(100);
> +			push_attempts = 0;
> +		}
> +	}
> +}
> +
> +static void rep_sched_work(const char *path, int rep)
> +{
> +	int i;
> +
> +	for (i = 0; i < rep; i++)
> +		sched_work(path);
> +}
> +
> +static void setup(void)
> +{
> +	if (tst_parse_int(str_repeat, &repeat, 1, INT_MAX))
> +		tst_brk(TBROK,
> +			"Invalid repeat (-r) argument: '%s'", str_repeat);

> +	if (!root_dir)
> +		tst_brk(TBROK, "The directory argument (-d) is required");
> +
> +	worker_count = MIN(MAX(SAFE_SYSCONF(_SC_NPROCESSORS_ONLN) - 1, 1), 15);

We do have tst_ncpus() because there are older distros that do not
define the _SC_* macros, please use that one instead here.

Also we cap the number of workers on 15 here, shouldn't that be at least
macro constant? I also wonder if we should supply command line option to
override the number of workers.

> +	workers = SAFE_MALLOC(worker_count * sizeof(*workers));
> +}
> +
> +static void cleanup(void)
> +{
> +	stop_workers();

We are stopping the workers at the end of the run(), do we really need
to stop them here as well?

> +	free(workers);
> +}
> +
> +static void visit_dir(const char *path)
> +{
> +	DIR *dir;
> +	struct dirent *dent;
> +	struct stat dent_st;
> +	char dent_path[MAX_PATH];
> +	enum dent_action act;
> +
> +	dir = opendir(path);
> +	if (!dir) {
> +		tst_res(TINFO | TERRNO, "opendir(%s)", path);
> +		return;
> +	}
> +
> +	while (1) {
> +		errno = 0;
> +		dent = readdir(dir);
> +		if (!dent && errno) {
> +			tst_res(TINFO | TERRNO, "readdir(%s)", path);
> +			break;
> +		} else if (!dent)
> +			break;
> +
> +		if (!strcmp(dent->d_name, ".") ||
> +		    !strcmp(dent->d_name, ".."))
> +			continue;
> +
> +		if (dent->d_type == DT_DIR)
> +			act = DA_VISIT;
> +		else if (dent->d_type == DT_LNK)
> +			act = DA_IGNORE;
> +		else if (dent->d_type == DT_UNKNOWN)
> +			act = DA_UNKNOWN;
> +		else
> +			act = DA_READ;
> +
> +		snprintf(dent_path, MAX_PATH,
> +			 "%s/%s", path, dent->d_name);
> +
> +		if (act == DA_UNKNOWN) {
> +			if (lstat(dent_path, &dent_st))
> +				tst_res(TINFO | TERRNO, "lstat(%s)", path);
> +			else if ((dent_st.st_mode & S_IFMT) == S_IFDIR)
> +				act = DA_VISIT;
> +			else if ((dent_st.st_mode & S_IFMT) == S_IFLNK)
> +				act = DA_IGNORE;
> +			else
> +				act = DA_READ;
> +		}
> +
> +		if (act == DA_VISIT)
> +			visit_dir(dent_path);
> +		else if (act == DA_READ)
> +			rep_sched_work(dent_path, repeat);
> +	}
> +
> +	if (closedir(dir))
> +		tst_res(TINFO | TERRNO, "closedir(%s)", path);
> +}
> +
> +static void run(void)
> +{
> +	spawn_workers();
> +	visit_dir(root_dir);
> +	stop_workers();
> +
> +	tst_reap_children();
> +	tst_res(TPASS, "Finished reading files");
> +}
> +
> +static struct tst_test test = {
> +	.options = options,
> +	.setup = setup,
> +	.cleanup = cleanup,
> +	.test_all = run,
> +	.forks_child = 1,
> +};

The rest looks good to me.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] Add read_all file systems test

[Richard Palethorpe]

Hello,

Cyril Hrubis writes:

> Hi!
>> +#define QUEUE_SIZE 16384
>> +#define BUFFER_SIZE 1024
>> +#define MAX_PATH 4096
>> +#define MAX_DISPLAY 40
>> +
>> +struct queue {
>> +	sem_t sem;
>> +	int front;
>> +	int back;
>> +	char data[QUEUE_SIZE];
>> +};
>> +
>> +struct worker {
>> +	pid_t pid;
>> +	struct queue *q;
>> +};
>> +
>> +enum dent_action {
>> +	DA_UNKNOWN,
>> +	DA_IGNORE,
>> +	DA_READ,
>> +	DA_VISIT,
>> +};
>> +
>> +static char *verbose;
>> +static char *quite;
>                   ^
> 		quiet?
>
>> +static char *root_dir;
>> +static char *exclude;
>> +static char *str_repeat;
>> +static int repeat = 1;
>> +static long worker_count;
>> +static struct worker *workers;
>> +
>> +static struct tst_option options[] = {
>> +	{"v", &verbose,
>> +	 "-v       Print information about successful reads"},
>> +	{"q", &quite,
>> +	 "-q       Don't print file read or open errors"},
>> +	{"d:", &root_dir,
>> +	 "-d path  Path to the directory to read from, defaults to /sys"},
>> +	{"e:", &exclude,
>> +	 "-e pattern Ignore files which match an 'extended' pattern, see fnmatch(3)"},
>> +	{"r:", &str_repeat,
>> +	 "-r count The number of times to schedule a file for reading"},
>
> I'm not sure repeat is the right name for parameter, I do not have much
> better idea though, maybe iteration.
>
>> +	{NULL, NULL, NULL}
>> +};
>> +
>> +static int queue_pop(struct queue *q, char *buf)
>> +{
>> +	int i = q->front, j = 0;
>> +
>> +	sem_wait(&q->sem);
>> +
>> +	if (!q->data[i])
>> +		return 0;
>> +
>> +	while (q->data[i]) {
>> +		buf[j] = q->data[i];
>> +
>> +		if (++j >= BUFFER_SIZE - 1)
>> +			tst_brk(TBROK, "Buffer is too small for path");
>> +		if (++i >= QUEUE_SIZE)
>> +			i = 0;
>> +	}
>> +
>> +	buf[j] = '\0';
>> +	tst_atomic_store(i + 1, &q->front);
>> +
>> +	return 1;
>> +}
>> +
>> +static int queue_push(struct queue *q, const char *buf)
>> +{
>> +	int i = q->back, j = 0;
>> +	int front = tst_atomic_load(&q->front);
>> +
>> +	while (buf[j]) {
>> +		q->data[i] = buf[j];
>> +
>> +		++j;
>> +		if (++i >= QUEUE_SIZE)
>> +			i = 0;
>> +		if (i == front)
>> +			return 0;
>> +	}
>> +	q->data[i] = '\0';
>> +
>> +	q->back = i + 1;
>> +	sem_post(&q->sem);
>> +
>> +	return 1;
>> +}
>> +
>> +static struct queue *queue_init(void)
>> +{
>> +	struct queue *q = SAFE_MMAP(NULL, sizeof(*q),
>> +				    PROT_READ | PROT_WRITE,
>> +				    MAP_SHARED | MAP_ANONYMOUS,
>> +				    0, 0);
>> +
>> +	sem_init(&q->sem, 1, 0);
>> +	q->front = 0;
>> +	q->back = 0;
>> +
>> +	return q;
>> +}
>> +
>> +static void queue_destroy(struct queue *q, int is_worker)
>> +{
>> +	if (is_worker)
>> +		sem_destroy(&q->sem);
>> +
>> +	SAFE_MUNMAP(q, sizeof(*q));
>> +}
>> +
>> +static void sanitize_str(char *buf, ssize_t count)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MIN(count, MAX_DISPLAY); i++)
>> +		if (!isprint(buf[i]))
>> +			buf[i] = ' ';
>> +
>> +	if (count <= MAX_DISPLAY)
>> +		buf[count] = '\0';
>> +	else
>> +		strcpy(buf + MAX_DISPLAY, "...");
>> +}
>> +
>> +static void read_test(const char *path)
>> +{
>> +	char buf[BUFFER_SIZE];
>> +	int fd;
>> +	ssize_t count;
>> +
>> +	if (exclude && !fnmatch(exclude, path, FNM_EXTMATCH)) {
>> +		if (verbose)
>> +			tst_res(TINFO, "Ignoring %s", path);
>> +		return;
>> +	}
>> +
>> +	if (verbose)
>> +		tst_res(TINFO, "%s(%s)", __func__, path);
>> +
>> +	fd = open(path, O_RDONLY | O_NONBLOCK);
>> +	if (fd < 0 && !quite) {
>> +		tst_res(TINFO | TERRNO, "open(%s)", path);
>> +		return;
>> +	} else if (fd < 0)
>> +		return;
>
> I guess that this would be a bit more readable:
>
>
> 	if (fd < 0) {
> 		if (!quiet)
> 			tst_res(...)
> 		return;
> 	}
>
>> +	count = read(fd, buf, sizeof(buf) - 1);
>> +	if (count > 0 && verbose) {
>> +		sanitize_str(buf, count);
>> +		tst_res(TINFO, "read(%s, buf) = %ld, buf = %s",
>> +			path, count, buf);
>> +	} else if (!count && verbose)
>> +		tst_res(TINFO, "read(%s) = EOF", path);
>> +
>> +	else if (count < 0 && !quite)
>> +		tst_res(TINFO | TERRNO, "read(%s)", path);
>> +
>> +	SAFE_CLOSE(fd);
>> +}
>> +
>> +static int worker_run(struct worker *self)
>> +{
>> +	int ret;
>> +	char buf[BUFFER_SIZE];
>> +	struct sigaction term_sa = {
>> +		.sa_handler = SIG_IGN,
>> +		.sa_flags = 0,
>> +	};
>> +	struct queue *q = self->q;
>> +
>> +	sigaction(SIGTTIN, &term_sa, NULL);
>> +
>> +	for (ret = queue_pop(q, buf); ret; ret = queue_pop(q, buf))
>> +		read_test(buf);
>
> As much as I do like to strech for loop for everything this would be
> probably more elegant as an infinite loop with a break.
>
> 	for (;;) {
> 		if (!queue_pop(q, buf))
> 			break;
>
> 		read_test(buf);
> 	}
>
>> +	queue_destroy(q, 1);
>> +	fflush(stdout);
>
> What are trying to flush here? The tst_res() messages are printed
> into the stderr btw.

I found that the pass message was being written after some of the
childrens' information messages. Calling fflush on stdout prevents that
from happening even though it is the wrong fd...

I have changed it to stderr, which seems more correct.

>
>> +	return 0;
>> +}
>> +
>> +static void spawn_workers(void)
>> +{
>> +	int i;
>> +	struct worker *wa = workers;
>> +
>> +	bzero(workers, worker_count * sizeof(*workers));
>> +
>> +	for (i = 0; i < worker_count; i++) {
>> +		wa[i].q = queue_init();
>> +		wa[i].pid = SAFE_FORK();
>> +		if (!wa[i].pid)
>> +			exit(worker_run(wa + i));
>> +	}
>> +}
>> +
>> +static void stop_workers(void)
>> +{
>> +	const char stop_code[1] = { '\0' };
>> +	int i;
>> +
>> +	if (!workers)
>> +		return;
>> +
>> +	for (i = 0; i < worker_count; i++)
>> +		if (workers[i].q)
>> +			queue_push(workers[i].q, stop_code);
>> +
>> +	for (i = 0; i < worker_count; i++)
>> +		if (workers[i].q) {
>> +			queue_destroy(workers[i].q, 0);
>> +			workers[i].q = 0;
>> +		}
>
> FYI LKML coding style preferes curly braces around the block in a case
> that it spans over multiple lines.
>
>> +}
>> +
>> +static void sched_work(const char *path)
>> +{
>> +	static int cur;
>> +	int push_attempts = 0, pushed;
>> +
>> +	while (1) {
>> +		pushed = queue_push(workers[cur].q, path);
>> +
>> +		if (++cur >= worker_count)
>> +			cur = 0;
>> +
>> +		if (pushed)
>> +			break;
>> +
>> +		if (++push_attempts > worker_count) {
>> +			usleep(100);
>> +			push_attempts = 0;
>> +		}
>> +	}
>> +}
>> +
>> +static void rep_sched_work(const char *path, int rep)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < rep; i++)
>> +		sched_work(path);
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	if (tst_parse_int(str_repeat, &repeat, 1, INT_MAX))
>> +		tst_brk(TBROK,
>> +			"Invalid repeat (-r) argument: '%s'", str_repeat);
>
>> +	if (!root_dir)
>> +		tst_brk(TBROK, "The directory argument (-d) is required");
>> +
>> +	worker_count = MIN(MAX(SAFE_SYSCONF(_SC_NPROCESSORS_ONLN) - 1, 1), 15);
>
> We do have tst_ncpus() because there are older distros that do not
> define the _SC_* macros, please use that one instead here.
>
> Also we cap the number of workers on 15 here, shouldn't that be at least
> macro constant? I also wonder if we should supply command line option to
> override the number of workers.
>
>> +	workers = SAFE_MALLOC(worker_count * sizeof(*workers));
>> +}
>> +
>> +static void cleanup(void)
>> +{
>> +	stop_workers();
>
> We are stopping the workers at the end of the run(), do we really need
> to stop them here as well?

spawn_workers calls SAFE_FORK which could fail after a few workers have
already been spawned.

>
>> +	free(workers);
>> +}
>> +
>> +static void visit_dir(const char *path)
>> +{
>> +	DIR *dir;
>> +	struct dirent *dent;
>> +	struct stat dent_st;
>> +	char dent_path[MAX_PATH];
>> +	enum dent_action act;
>> +
>> +	dir = opendir(path);
>> +	if (!dir) {
>> +		tst_res(TINFO | TERRNO, "opendir(%s)", path);
>> +		return;
>> +	}
>> +
>> +	while (1) {
>> +		errno = 0;
>> +		dent = readdir(dir);
>> +		if (!dent && errno) {
>> +			tst_res(TINFO | TERRNO, "readdir(%s)", path);
>> +			break;
>> +		} else if (!dent)
>> +			break;
>> +
>> +		if (!strcmp(dent->d_name, ".") ||
>> +		    !strcmp(dent->d_name, ".."))
>> +			continue;
>> +
>> +		if (dent->d_type == DT_DIR)
>> +			act = DA_VISIT;
>> +		else if (dent->d_type == DT_LNK)
>> +			act = DA_IGNORE;
>> +		else if (dent->d_type == DT_UNKNOWN)
>> +			act = DA_UNKNOWN;
>> +		else
>> +			act = DA_READ;
>> +
>> +		snprintf(dent_path, MAX_PATH,
>> +			 "%s/%s", path, dent->d_name);
>> +
>> +		if (act == DA_UNKNOWN) {
>> +			if (lstat(dent_path, &dent_st))
>> +				tst_res(TINFO | TERRNO, "lstat(%s)", path);
>> +			else if ((dent_st.st_mode & S_IFMT) == S_IFDIR)
>> +				act = DA_VISIT;
>> +			else if ((dent_st.st_mode & S_IFMT) == S_IFLNK)
>> +				act = DA_IGNORE;
>> +			else
>> +				act = DA_READ;
>> +		}
>> +
>> +		if (act == DA_VISIT)
>> +			visit_dir(dent_path);
>> +		else if (act == DA_READ)
>> +			rep_sched_work(dent_path, repeat);
>> +	}
>> +
>> +	if (closedir(dir))
>> +		tst_res(TINFO | TERRNO, "closedir(%s)", path);
>> +}
>> +
>> +static void run(void)
>> +{
>> +	spawn_workers();
>> +	visit_dir(root_dir);
>> +	stop_workers();
>> +
>> +	tst_reap_children();
>> +	tst_res(TPASS, "Finished reading files");
>> +}
>> +
>> +static struct tst_test test = {
>> +	.options = options,
>> +	.setup = setup,
>> +	.cleanup = cleanup,
>> +	.test_all = run,
>> +	.forks_child = 1,
>> +};
>
> The rest looks good to me.


-- 
Thank you,
Richard.

[LTP] [PATCH v2] Add read_all file systems test

[Cyril Hrubis]

Hi!
> >> +	queue_destroy(q, 1);
> >> +	fflush(stdout);
> >
> > What are trying to flush here? The tst_res() messages are printed
> > into the stderr btw.
> 
> I found that the pass message was being written after some of the
> childrens' information messages. Calling fflush on stdout prevents that
> from happening even though it is the wrong fd...

You mean the tst_res(TPASS, ...) called after the tst_reap_children() ?

That is really strange since at that time the processes had called
exit() already and the buffers should have been flushed at that point.
I.e. if we haven't flushed it in the worker_run() the exit() should take
care of that, which is guaranteed to run before we return from the
tst_reap_children().

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] Add read_all file systems test

[Richard Palethorpe]

Hello,

Cyril Hrubis writes:

> Hi!
>> >> +	queue_destroy(q, 1);
>> >> +	fflush(stdout);
>> >
>> > What are trying to flush here? The tst_res() messages are printed
>> > into the stderr btw.
>>
>> I found that the pass message was being written after some of the
>> childrens' information messages. Calling fflush on stdout prevents that
>> from happening even though it is the wrong fd...
>
> You mean the tst_res(TPASS, ...) called after the tst_reap_children() ?
>
> That is really strange since at that time the processes had called
> exit() already and the buffers should have been flushed at that point.
> I.e. if we haven't flushed it in the worker_run() the exit() should take
> care of that, which is guaranteed to run before we return from the
> tst_reap_children().

It only happens on older products, so it could be removed if you are not
concerned about that? I'm not sure if investigating it further would be
useful.

Also I found a bug when pushing to a full queue, so I need to do another
patch revision anyway.

--
Thank you,
Richard.