Skip to contentSkip to conversation

Tests for readahead() and fadvise() on overlayfs

PATCH Superseded Amir Goldstein 2 participants 14 messages Oct 10, 2018 – Nov 28, 2018

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover Tests for readahead() and fadvise() on overlayfs

Cyril,

The following series adds test covergae for readahead() syscall over overlayfs file and adds test coverage for a specific posix_fadvise() syscall advice (POSIX_FADV_WILLNEED).

So far, the posix_fadvise syscall tests in LTP only test for error conditions, but not functionality.

The functionality of the advice POSIX_FADV_WILLNEED is identical to readahead() and since kernel commit 3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED"), the implementations are also bound together.

To add test coverage of the advice POSIX_FADV_WILLNEED, I decided not to duplicate the readahead() functional test and add test cases to readahead02 that use the POSIX_FADV_WILLNEED implementation.

Thanks,
Amir.

Changes since v2:
- Runtime check for readahead/fadvise support
- Abort test case immediately if syscall fail

Changes since v1:
- Fix bugs in loop invocation of test (i.e. -i 2)
- Address review comments from Jan Stancek
- Make cached_max a global maximum over all test cases
- Improve reliability of overlayfs readahead test case failure

Amir Goldstein (6):
syscalls/readahead01: Convert to newlib syscalls/readahead02: Convert to newlib and cleanup syscalls/readahead02: abort test if readahead syscall fails syscalls/readahead02: test readahead() on an overlayfs file syscalls/readahead02: test readahead using posix_fadvise() syscalls/readahead02: fail test if readahead did not use any cache

 .../kernel/syscalls/readahead/readahead01.c   | 119 ++----
 .../kernel/syscalls/readahead/readahead02.c   | 339 +++++++++---------
 2 files changed, 198 insertions(+), 260 deletions(-)
Signature · 1 line
2.17.1
#1 lore raw

PATCH v3 1/6 syscalls/readahead01: Convert to newlib

* Use SPDX-License-Identifier

---
 .../kernel/syscalls/readahead/readahead01.c   | 119 +++++-------------
 1 file changed, 34 insertions(+), 85 deletions(-)
testcases/kernel/syscalls/readahead/readahead01.c +34 -85
index f35019488..682b524b3 100644
@@ -1,25 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Linux Test Project, Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 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.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * 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 Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
  */
 
 /*
@@ -36,47 +17,36 @@
 #include <sys/syscall.h>
 #include <sys/types.h>
 #include "config.h"
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 #include "lapi/syscalls.h"
 
-char *TCID = "readahead01";
-int TST_TOTAL = 1;
-
-option_t options[] = {
-	{NULL, NULL, NULL}
-};
-
 #if defined(__NR_readahead)
-static void setup(void);
-static void cleanup(void);
-
 static int check_ret(long expected_ret)
 {
-	if (expected_ret == TEST_RETURN) {
-		tst_resm(TPASS, "expected ret success - "
-			 "returned value = %ld", TEST_RETURN);
+	if (expected_ret == TST_RET) {
+		tst_res(TPASS, "expected ret success - "
+			"returned value = %ld", TST_RET);
 		return 0;
 	}
-	tst_resm(TFAIL, "unexpected failure - "
-		 "returned value = %ld, expected: %ld",
-		 TEST_RETURN, expected_ret);
+	tst_res(TFAIL, "unexpected failure - "
+		"returned value = %ld, expected: %ld",
+		TST_RET, expected_ret);
 	return 1;
 }
 
 static int check_errno(long expected_errno)
 {
-	if (TEST_ERRNO == expected_errno) {
-		tst_resm(TPASS | TTERRNO, "expected failure");
+	if (TST_ERR == expected_errno) {
+		tst_res(TPASS | TTERRNO, "expected failure");
 		return 0;
 	}
 
-	if (TEST_ERRNO == 0)
-		tst_resm(TFAIL, "call succeeded unexpectedly");
+	if (TST_ERR == 0)
+		tst_res(TFAIL, "call succeeded unexpectedly");
 	else
-		tst_resm(TFAIL | TTERRNO, "unexpected failure - "
-			 "expected = %ld : %s, actual",
-			 expected_errno, strerror(expected_errno));
+		tst_res(TFAIL | TTERRNO, "unexpected failure - "
+			"expected = %ld : %s, actual",
+			expected_errno, strerror(expected_errno));
 	return 1;
 }
 
@@ -85,19 +55,17 @@ static void test_bad_fd(void)
 	char tempname[PATH_MAX] = "readahead01_XXXXXX";
 	int fd;
 
-	tst_resm(TINFO, "test_bad_fd -1");
+	tst_res(TINFO, "test_bad_fd -1");
 	TEST(readahead(-1, 0, getpagesize()));
 	check_ret(-1);
 	check_errno(EBADF);
 
-	tst_resm(TINFO, "test_bad_fd O_WRONLY");
+	tst_res(TINFO, "test_bad_fd O_WRONLY");
 	fd = mkstemp(tempname);
 	if (fd == -1)
-		tst_resm(TBROK | TERRNO, "mkstemp failed");
+		tst_res(TBROK | TERRNO, "mkstemp failed");
 	close(fd);
-	fd = open(tempname, O_WRONLY);
-	if (fd == -1)
-		tst_resm(TBROK | TERRNO, "Failed to open testfile");
+	fd = SAFE_OPEN(tempname, O_WRONLY);
 	TEST(readahead(fd, 0, getpagesize()));
 	check_ret(-1);
 	check_errno(EBADF);
@@ -109,60 +77,41 @@ static void test_invalid_fd(void)
 {
 	int fd[2];
 
-	tst_resm(TINFO, "test_invalid_fd pipe");
-	if (pipe(fd) < 0)
-		tst_resm(TBROK | TERRNO, "Failed to create pipe");
+	tst_res(TINFO, "test_invalid_fd pipe");
+	SAFE_PIPE(fd);
 	TEST(readahead(fd[0], 0, getpagesize()));
 	check_ret(-1);
 	check_errno(EINVAL);
 	close(fd[0]);
 	close(fd[1]);
 
-	tst_resm(TINFO, "test_invalid_fd socket");
-	fd[0] = socket(AF_INET, SOCK_STREAM, 0);
-	if (fd[0] < 0)
-		tst_resm(TBROK | TERRNO, "Failed to create socket");
+	tst_res(TINFO, "test_invalid_fd socket");
+	fd[0] = SAFE_SOCKET(AF_INET, SOCK_STREAM, 0);
 	TEST(readahead(fd[0], 0, getpagesize()));
 	check_ret(-1);
 	check_errno(EINVAL);
 	close(fd[0]);
 }
 
-int main(int argc, char *argv[])
+void test_readahead(void)
 {
-	int lc;
-
-	tst_parse_opts(argc, argv, options, NULL);
-
-	setup();
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-		test_bad_fd();
-		test_invalid_fd();
-	}
-	cleanup();
-	tst_exit();
+	test_bad_fd();
+	test_invalid_fd();
 }
 
 static void setup(void)
 {
-	tst_require_root();
-	tst_tmpdir();
-
 	/* check if readahead syscall is supported */
-	ltp_syscall(__NR_readahead, 0, 0, 0);
-
-	TEST_PAUSE;
+	tst_syscall(__NR_readahead, 0, 0, 0);
 }
 
-static void cleanup(void)
-{
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.setup = setup,
+	.test_all = test_readahead,
+};
 
 #else /* __NR_readahead */
-int main(void)
-{
-	tst_brkm(TCONF, NULL, "System doesn't support __NR_readahead");
-}
+	TST_TEST_TCONF("System doesn't support __NR_readahead");
 #endif
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH v3 1/6] syscalls/readahead01: Convert to newlib

Hi!
Pushed with minor changes:

* close() -> SAFE_CLOSE()

* Changed the check_* functions to void since we do not use the returned value

* Removed .needs_root = 1 since the test isn't doing anything priviledged

Thanks.

Signature · 2 lines
Cyril Hrubis
[email protected]
#3 lore raw

PATCH v3 2/6 syscalls/readahead02: Convert to newlib and cleanup

In reply to Amir Goldstein

* Use SAFE macros

* Use SPDX-License-Identifier

* No need to cleanup test file from temp dir

---
 .../kernel/syscalls/readahead/readahead02.c   | 248 +++++-------------
 1 file changed, 72 insertions(+), 176 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +72 -176
index c9d92a6a4..c739d3ba2 100644
@@ -1,25 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Linux Test Project, Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 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.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * 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 Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
  */
 
 /*
@@ -43,86 +24,57 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include "config.h"
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 #include "lapi/syscalls.h"
 
-char *TCID = "readahead02";
-int TST_TOTAL = 1;
-
 #if defined(__NR_readahead)
 static char testfile[PATH_MAX] = "testfile";
 static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
 static const char meminfo_fname[] = "/proc/meminfo";
 static size_t testfile_size = 64 * 1024 * 1024;
-static int opt_fsize;
 static char *opt_fsizestr;
 static int pagesize;
-static const char *fs_type;
-static const char *device;
-static int mount_flag;
 
 #define MNTPOINT        "mntpoint"
 #define MIN_SANE_READAHEAD (4u * 1024u)
 
-option_t options[] = {
-	{"s:", &opt_fsize, &opt_fsizestr},
+static const char mntpoint[] = MNTPOINT;
+
+static struct tst_option options[] = {
+	{"s:", &opt_fsizestr, "-s    testfile size (default 64MB)"},
 	{NULL, NULL, NULL}
 };
 
-static void setup(void);
-static void cleanup(void);
-
-static void help(void)
-{
-	printf("  -s x    testfile size (default 64MB)\n");
-}
-
 static int check_ret(long expected_ret)
 {
-	if (expected_ret == TEST_RETURN) {
-		tst_resm(TPASS, "expected ret success - "
-			 "returned value = %ld", TEST_RETURN);
+	if (expected_ret == TST_RET) {
+		tst_res(TPASS, "expected ret success - "
+			"returned value = %ld", TST_RET);
 		return 0;
 	}
-	tst_resm(TFAIL | TTERRNO, "unexpected failure - "
-		 "returned value = %ld, expected: %ld",
-		 TEST_RETURN, expected_ret);
+	tst_res(TFAIL | TTERRNO, "unexpected failure - "
+		"returned value = %ld, expected: %ld",
+		TST_RET, expected_ret);
 	return 1;
 }
 
 static int has_file(const char *fname, int required)
 {
-	int ret;
 	struct stat buf;
-	ret = stat(fname, &buf);
-	if (ret == -1) {
-		if (errno == ENOENT)
-			if (required)
-				tst_brkm(TCONF, cleanup, "%s not available",
-					 fname);
-			else
-				return 0;
-		else
-			tst_brkm(TBROK | TERRNO, cleanup, "stat %s", fname);
+
+	if (stat(fname, &buf) == -1) {
+		if (errno != ENOENT)
+			tst_brk(TBROK | TERRNO, "stat %s", fname);
+		if (required)
+			tst_brk(TCONF, "%s not available", fname);
+		return 0;
 	}
 	return 1;
 }
 
 static void drop_caches(void)
 {
-	int ret;
-	FILE *f;
-
-	f = fopen(drop_caches_fname, "w");
-	if (f) {
-		ret = fprintf(f, "1");
-		fclose(f);
-		if (ret < 1)
-			tst_brkm(TBROK, cleanup, "Failed to drop caches");
-	} else {
-		tst_brkm(TBROK, cleanup, "Failed to open drop_caches");
-	}
+	SAFE_FILE_PRINTF(drop_caches_fname, "1");
 }
 
 static unsigned long parse_entry(const char *fname, const char *entry)
@@ -161,32 +113,21 @@ static unsigned long get_cached_size(void)
 
 static void create_testfile(void)
 {
-	FILE *f;
+	int fd;
 	char *tmp;
 	size_t i;
 
-	tst_resm(TINFO, "creating test file of size: %zu", testfile_size);
-	tmp = SAFE_MALLOC(cleanup, pagesize);
+	tst_res(TINFO, "creating test file of size: %zu", testfile_size);
+	tmp = SAFE_MALLOC(pagesize);
 
 	/* round to page size */
 	testfile_size = testfile_size & ~((long)pagesize - 1);
 
-	f = fopen(testfile, "w");
-	if (!f) {
-		free(tmp);
-		tst_brkm(TBROK | TERRNO, cleanup, "Failed to create %s",
-			 testfile);
-	}
-
+	fd = SAFE_CREAT(testfile, 0644);
 	for (i = 0; i < testfile_size; i += pagesize)
-		if (fwrite(tmp, pagesize, 1, f) < 1) {
-			free(tmp);
-			tst_brkm(TBROK, cleanup, "Failed to create %s",
-				 testfile);
-		}
-	fflush(f);
-	fsync(fileno(f));
-	fclose(f);
+		SAFE_WRITE(1, fd, tmp, pagesize);
+	SAFE_FSYNC(fd);
+	SAFE_CLOSE(fd);
 	free(tmp);
 }
 
@@ -215,13 +156,13 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 	off_t offset = 0;
 	struct timeval now;
 
-	fd = SAFE_OPEN(cleanup, fname, O_RDONLY);
+	fd = SAFE_OPEN(fname, O_RDONLY);
 
 	if (do_readahead) {
 		cached_start = get_cached_size();
 		do {
 			TEST(readahead(fd, offset, fsize - offset));
-			if (TEST_RETURN != 0) {
+			if (TST_RET != 0) {
 				check_ret(0);
 				break;
 			}
@@ -232,7 +173,7 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 				if (*cached > cached_start) {
 					max_ra_estimate = (1024 *
 						(*cached - cached_start));
-					tst_resm(TINFO, "max ra estimate: %lu",
+					tst_res(TINFO, "max ra estimate: %lu",
 						max_ra_estimate);
 				}
 				max_ra_estimate = MAX(max_ra_estimate,
@@ -242,27 +183,23 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 			i++;
 			offset += max_ra_estimate;
 		} while ((size_t)offset < fsize);
-		tst_resm(TINFO, "readahead calls made: %zu", i);
+		tst_res(TINFO, "readahead calls made: %zu", i);
 		*cached = get_cached_size();
 
 		/* offset of file shouldn't change after readahead */
-		offset = lseek(fd, 0, SEEK_CUR);
-		if (offset == (off_t) - 1)
-			tst_brkm(TBROK | TERRNO, cleanup, "lseek failed");
+		offset = SAFE_LSEEK(fd, 0, SEEK_CUR);
 		if (offset == 0)
-			tst_resm(TPASS, "offset is still at 0 as expected");
+			tst_res(TPASS, "offset is still at 0 as expected");
 		else
-			tst_resm(TFAIL, "offset has changed to: %lu", offset);
+			tst_res(TFAIL, "offset has changed to: %lu", offset);
 	}
 
 	if (gettimeofday(&now, NULL) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "gettimeofday failed");
+		tst_brk(TBROK | TERRNO, "gettimeofday failed");
 	time_start_usec = now.tv_sec * 1000000 + now.tv_usec;
 	read_bytes_start = get_bytes_read();
 
-	p = mmap(NULL, fsize, PROT_READ, MAP_SHARED | MAP_POPULATE, fd, 0);
-	if (p == MAP_FAILED)
-		tst_brkm(TBROK | TERRNO, cleanup, "mmap failed");
+	p = SAFE_MMAP(NULL, fsize, PROT_READ, MAP_SHARED | MAP_POPULATE, fd, 0);
 
 	/* for old kernels, where MAP_POPULATE doesn't work, touch each page */
 	tmp = 0;
@@ -270,20 +207,20 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 		tmp = tmp ^ p[i];
 	/* prevent gcc from optimizing out loop above */
 	if (tmp != 0)
-		tst_brkm(TBROK, NULL, "This line should not be reached");
+		tst_brk(TBROK, "This line should not be reached");
 
 	if (!do_readahead)
 		*cached = get_cached_size();
 
-	SAFE_MUNMAP(cleanup, p, fsize);
+	SAFE_MUNMAP(p, fsize);
 
 	*read_bytes = get_bytes_read() - read_bytes_start;
 	if (gettimeofday(&now, NULL) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "gettimeofday failed");
+		tst_brk(TBROK | TERRNO, "gettimeofday failed");
 	time_end_usec = now.tv_sec * 1000000 + now.tv_usec;
 	*usec = time_end_usec - time_start_usec;
 
-	SAFE_CLOSE(cleanup, fd);
+	SAFE_CLOSE(fd);
 }
 
 static void test_readahead(void)
@@ -302,7 +239,7 @@ static void test_readahead(void)
 	cached_low = get_cached_size();
 	cached_max = cached_max - cached_low;
 
-	tst_resm(TINFO, "read_testfile(0)");
+	tst_res(TINFO, "read_testfile(0)");
 	read_testfile(0, testfile, testfile_size, &read_bytes, &usec, &cached);
 	if (cached > cached_low)
 		cached = cached - cached_low;
@@ -312,7 +249,7 @@ static void test_readahead(void)
 	sync();
 	drop_caches();
 	cached_low = get_cached_size();
-	tst_resm(TINFO, "read_testfile(1)");
+	tst_res(TINFO, "read_testfile(1)");
 	read_testfile(1, testfile, testfile_size, &read_bytes_ra,
 		      &usec_ra, &cached_ra);
 	if (cached_ra > cached_low)
@@ -320,25 +257,25 @@ static void test_readahead(void)
 	else
 		cached_ra = 0;
 
-	tst_resm(TINFO, "read_testfile(0) took: %ld usec", usec);
-	tst_resm(TINFO, "read_testfile(1) took: %ld usec", usec_ra);
+	tst_res(TINFO, "read_testfile(0) took: %ld usec", usec);
+	tst_res(TINFO, "read_testfile(1) took: %ld usec", usec_ra);
 	if (has_file(proc_io_fname, 0)) {
-		tst_resm(TINFO, "read_testfile(0) read: %ld bytes", read_bytes);
-		tst_resm(TINFO, "read_testfile(1) read: %ld bytes",
-			 read_bytes_ra);
+		tst_res(TINFO, "read_testfile(0) read: %ld bytes", read_bytes);
+		tst_res(TINFO, "read_testfile(1) read: %ld bytes",
+			read_bytes_ra);
 		/* actual number of read bytes depends on total RAM */
 		if (read_bytes_ra < read_bytes)
-			tst_resm(TPASS, "readahead saved some I/O");
+			tst_res(TPASS, "readahead saved some I/O");
 		else
-			tst_resm(TFAIL, "readahead failed to save any I/O");
+			tst_res(TFAIL, "readahead failed to save any I/O");
 	} else {
-		tst_resm(TCONF, "Your system doesn't have /proc/pid/io,"
-			 " unable to determine read bytes during test");
+		tst_res(TCONF, "Your system doesn't have /proc/pid/io,"
+			" unable to determine read bytes during test");
 	}
 
-	tst_resm(TINFO, "cache can hold at least: %ld kB", cached_max);
-	tst_resm(TINFO, "read_testfile(0) used cache: %ld kB", cached);
-	tst_resm(TINFO, "read_testfile(1) used cache: %ld kB", cached_ra);
+	tst_res(TINFO, "cache can hold at least: %ld kB", cached_max);
+	tst_res(TINFO, "read_testfile(0) used cache: %ld kB", cached);
+	tst_res(TINFO, "read_testfile(1) used cache: %ld kB", cached_ra);
 
 	if (cached_max * 1024 >= testfile_size) {
 		/*
@@ -346,83 +283,42 @@ static void test_readahead(void)
 		 * for readahead should be at least testfile_size/2
 		 */
 		if (cached_ra * 1024 > testfile_size / 2)
-			tst_resm(TPASS, "using cache as expected");
+			tst_res(TPASS, "using cache as expected");
 		else
-			tst_resm(TWARN, "using less cache than expected");
+			tst_res(TWARN, "using less cache than expected");
 	} else {
-		tst_resm(TCONF, "Page cache on your system is too small "
-			 "to hold whole testfile.");
+		tst_res(TCONF, "Page cache on your system is too small "
+			"to hold whole testfile.");
 	}
 }
 
-int main(int argc, char *argv[])
-{
-	int lc;
-
-	tst_parse_opts(argc, argv, options, help);
-
-	if (opt_fsize)
-		testfile_size = atoi(opt_fsizestr);
-
-	setup();
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-		test_readahead();
-	}
-	cleanup();
-	tst_exit();
-}
-
 static void setup(void)
 {
-	tst_require_root();
-	tst_tmpdir();
-	TEST_PAUSE;
+	if (opt_fsizestr)
+		testfile_size = SAFE_STRTOL(opt_fsizestr, 1, INT_MAX);
 
 	has_file(drop_caches_fname, 1);
 	has_file(meminfo_fname, 1);
 
 	/* check if readahead is supported */
-	ltp_syscall(__NR_readahead, 0, 0, 0);
+	tst_syscall(__NR_readahead, 0, 0, 0);
 
 	pagesize = getpagesize();
 
-	if (tst_fs_type(cleanup, ".") == TST_TMPFS_MAGIC) {
-		tst_resm(TINFO, "TMPFS detected, creating loop device");
-
-		fs_type = tst_dev_fs_type();
-		device = tst_acquire_device(cleanup);
-		if (!device) {
-			tst_brkm(TCONF, cleanup,
-				"Failed to obtain block device");
-		}
-
-		tst_mkfs(cleanup, device, fs_type, NULL, NULL);
-
-		SAFE_MKDIR(cleanup, MNTPOINT, 0755);
-		SAFE_MOUNT(cleanup, device, MNTPOINT, fs_type, 0, NULL);
-		mount_flag = 1;
-
-		sprintf(testfile, MNTPOINT"/testfile");
-	}
+	sprintf(testfile, "%s/testfile", mntpoint);
 	create_testfile();
 }
 
-static void cleanup(void)
-{
-	unlink(testfile);
-	if (mount_flag && tst_umount(MNTPOINT) < 0)
-		tst_resm(TWARN | TERRNO, "umount device:%s failed", device);
-
-	if (device)
-		tst_release_device(device);
-
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.mount_device = 1,
+	.mntpoint = mntpoint,
+	.setup = setup,
+	.options = options,
+	.test_all = test_readahead,
+};
 
 #else /* __NR_readahead */
-int main(void)
-{
-	tst_brkm(TCONF, NULL, "System doesn't support __NR_readahead");
-}
+	TST_TEST_TCONF("System doesn't support __NR_readahead");
 #endif
Signature · 1 line
2.17.1
#4 lore raw

Re: [PATCH v3 2/6] syscalls/readahead02: Convert to newlib and cleanup

Hi!

Quoted 5 lines

* Use SAFE macros

* Use SPDX-License-Identifier

* No need to cleanup test file from temp dir

Generally looks good, a few comments below.

Quoted 88 lines
---
 .../kernel/syscalls/readahead/readahead02.c   | 248 +++++-------------
 1 file changed, 72 insertions(+), 176 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +9 -42
index c9d92a6a4..c739d3ba2 100644
@@ -1,25 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (C) 2012 Linux Test Project, Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 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.
- *
- * Further, this software is distributed without any warranty that it
- * is free of the rightful claim of any third person regarding
- * infringement or the like.  Any license provided herein, whether
- * implied or otherwise, applies only to this software file.  Patent
- * licenses, if any, provided herein do not apply to combinations of
- * this program with other software, or any other product whatsoever.
- *
- * 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 Street, Fifth Floor, Boston, MA
- * 02110-1301, USA.
  */
 
 /*
@@ -43,86 +24,57 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include "config.h"
-#include "test.h"
-#include "safe_macros.h"
+#include "tst_test.h"
 #include "lapi/syscalls.h"
 
-char *TCID = "readahead02";
-int TST_TOTAL = 1;
-
 #if defined(__NR_readahead)
 static char testfile[PATH_MAX] = "testfile";
 static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
 static const char meminfo_fname[] = "/proc/meminfo";
 static size_t testfile_size = 64 * 1024 * 1024;
-static int opt_fsize;
 static char *opt_fsizestr;
 static int pagesize;
-static const char *fs_type;
-static const char *device;
-static int mount_flag;
 
 #define MNTPOINT        "mntpoint"
 #define MIN_SANE_READAHEAD (4u * 1024u)
 
-option_t options[] = {
-	{"s:", &opt_fsize, &opt_fsizestr},
+static const char mntpoint[] = MNTPOINT;
+
+static struct tst_option options[] = {
+	{"s:", &opt_fsizestr, "-s    testfile size (default 64MB)"},
 	{NULL, NULL, NULL}
 };
 
-static void setup(void);
-static void cleanup(void);
-
-static void help(void)
-{
-	printf("  -s x    testfile size (default 64MB)\n");
-}
-
 static int check_ret(long expected_ret)
 {
-	if (expected_ret == TEST_RETURN) {
-		tst_resm(TPASS, "expected ret success - "
-			 "returned value = %ld", TEST_RETURN);
+	if (expected_ret == TST_RET) {
+		tst_res(TPASS, "expected ret success - "
+			"returned value = %ld", TST_RET);

testcases/kernel/syscalls/readahead/readahead02.c line 59

Quoted 10 lines
testcases/kernel/syscalls/readahead/readahead02.c +3 -3
 		return 0;
 	}
-	tst_resm(TFAIL | TTERRNO, "unexpected failure - "
-		 "returned value = %ld, expected: %ld",
-		 TEST_RETURN, expected_ret);
+	tst_res(TFAIL | TTERRNO, "unexpected failure - "
+		"returned value = %ld, expected: %ld",
+		TST_RET, expected_ret);
 	return 1;
 }

I would be happier if we got rid of this function.

Maybe we should just move the loop from read_file() to a separate function do_readahead() then the indentation would be one level less there and the code could be put directly there.

Quoted 122 lines
testcases/kernel/syscalls/readahead/readahead02.c +20 -48
 static int has_file(const char *fname, int required)
 {
-	int ret;
 	struct stat buf;
-	ret = stat(fname, &buf);
-	if (ret == -1) {
-		if (errno == ENOENT)
-			if (required)
-				tst_brkm(TCONF, cleanup, "%s not available",
-					 fname);
-			else
-				return 0;
-		else
-			tst_brkm(TBROK | TERRNO, cleanup, "stat %s", fname);
+
+	if (stat(fname, &buf) == -1) {
+		if (errno != ENOENT)
+			tst_brk(TBROK | TERRNO, "stat %s", fname);
+		if (required)
+			tst_brk(TCONF, "%s not available", fname);
+		return 0;
 	}
 	return 1;
 }
 
 static void drop_caches(void)
 {
-	int ret;
-	FILE *f;
-
-	f = fopen(drop_caches_fname, "w");
-	if (f) {
-		ret = fprintf(f, "1");
-		fclose(f);
-		if (ret < 1)
-			tst_brkm(TBROK, cleanup, "Failed to drop caches");
-	} else {
-		tst_brkm(TBROK, cleanup, "Failed to open drop_caches");
-	}
+	SAFE_FILE_PRINTF(drop_caches_fname, "1");
 }
 
 static unsigned long parse_entry(const char *fname, const char *entry)
@@ -161,32 +113,21 @@ static unsigned long get_cached_size(void)
 
 static void create_testfile(void)
 {
-	FILE *f;
+	int fd;
 	char *tmp;
 	size_t i;
 
-	tst_resm(TINFO, "creating test file of size: %zu", testfile_size);
-	tmp = SAFE_MALLOC(cleanup, pagesize);
+	tst_res(TINFO, "creating test file of size: %zu", testfile_size);
+	tmp = SAFE_MALLOC(pagesize);
 
 	/* round to page size */
 	testfile_size = testfile_size & ~((long)pagesize - 1);
 
-	f = fopen(testfile, "w");
-	if (!f) {
-		free(tmp);
-		tst_brkm(TBROK | TERRNO, cleanup, "Failed to create %s",
-			 testfile);
-	}
-
+	fd = SAFE_CREAT(testfile, 0644);
 	for (i = 0; i < testfile_size; i += pagesize)
-		if (fwrite(tmp, pagesize, 1, f) < 1) {
-			free(tmp);
-			tst_brkm(TBROK, cleanup, "Failed to create %s",
-				 testfile);
-		}
-	fflush(f);
-	fsync(fileno(f));
-	fclose(f);
+		SAFE_WRITE(1, fd, tmp, pagesize);
+	SAFE_FSYNC(fd);
+	SAFE_CLOSE(fd);
 	free(tmp);
 }
 
@@ -215,13 +156,13 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 	off_t offset = 0;
 	struct timeval now;
 
-	fd = SAFE_OPEN(cleanup, fname, O_RDONLY);
+	fd = SAFE_OPEN(fname, O_RDONLY);
 
 	if (do_readahead) {
 		cached_start = get_cached_size();
 		do {
 			TEST(readahead(fd, offset, fsize - offset));
-			if (TEST_RETURN != 0) {
+			if (TST_RET != 0) {
 				check_ret(0);
 				break;
 			}
@@ -232,7 +173,7 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 				if (*cached > cached_start) {
 					max_ra_estimate = (1024 *
 						(*cached - cached_start));
-					tst_resm(TINFO, "max ra estimate: %lu",
+					tst_res(TINFO, "max ra estimate: %lu",
 						max_ra_estimate);
 				}
 				max_ra_estimate = MAX(max_ra_estimate,
@@ -242,27 +183,23 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 			i++;
 			offset += max_ra_estimate;
 		} while ((size_t)offset < fsize);
-		tst_resm(TINFO, "readahead calls made: %zu", i);
+		tst_res(TINFO, "readahead calls made: %zu", i);
 		*cached = get_cached_size();
 
 		/* offset of file shouldn't change after readahead */
-		offset = lseek(fd, 0, SEEK_CUR);
-		if (offset == (off_t) - 1)
-			tst_brkm(TBROK | TERRNO, cleanup, "lseek failed");
+		offset = SAFE_LSEEK(fd, 0, SEEK_CUR);
 		if (offset == 0)

testcases/kernel/syscalls/readahead/readahead02.c static void read_testfile(int do_readahead, const char *fname, size_t fsize, · line 198

Quoted 10 lines
testcases/kernel/syscalls/readahead/readahead02.c +3 -3
-			tst_resm(TPASS, "offset is still at 0 as expected");
+			tst_res(TPASS, "offset is still at 0 as expected");
 		else
-			tst_resm(TFAIL, "offset has changed to: %lu", offset);
+			tst_res(TFAIL, "offset has changed to: %lu", offset);
 	}
 
 	if (gettimeofday(&now, NULL) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "gettimeofday failed");
+		tst_brk(TBROK | TERRNO, "gettimeofday failed");

We should really switch to posix timers here, using wall clock for elapsed time measurements in not a good idea for several reasons.

And we even do have nice API for this, see:

https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2221-measuring-elapsed-time-and-helper-functions

Quoted 152 lines
testcases/kernel/syscalls/readahead/readahead02.c +26 -56
 	time_start_usec = now.tv_sec * 1000000 + now.tv_usec;
 	read_bytes_start = get_bytes_read();
 
-	p = mmap(NULL, fsize, PROT_READ, MAP_SHARED | MAP_POPULATE, fd, 0);
-	if (p == MAP_FAILED)
-		tst_brkm(TBROK | TERRNO, cleanup, "mmap failed");
+	p = SAFE_MMAP(NULL, fsize, PROT_READ, MAP_SHARED | MAP_POPULATE, fd, 0);
 
 	/* for old kernels, where MAP_POPULATE doesn't work, touch each page */
 	tmp = 0;
@@ -270,20 +207,20 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 		tmp = tmp ^ p[i];
 	/* prevent gcc from optimizing out loop above */
 	if (tmp != 0)
-		tst_brkm(TBROK, NULL, "This line should not be reached");
+		tst_brk(TBROK, "This line should not be reached");
 
 	if (!do_readahead)
 		*cached = get_cached_size();
 
-	SAFE_MUNMAP(cleanup, p, fsize);
+	SAFE_MUNMAP(p, fsize);
 
 	*read_bytes = get_bytes_read() - read_bytes_start;
 	if (gettimeofday(&now, NULL) == -1)
-		tst_brkm(TBROK | TERRNO, cleanup, "gettimeofday failed");
+		tst_brk(TBROK | TERRNO, "gettimeofday failed");
 	time_end_usec = now.tv_sec * 1000000 + now.tv_usec;
 	*usec = time_end_usec - time_start_usec;
 
-	SAFE_CLOSE(cleanup, fd);
+	SAFE_CLOSE(fd);
 }
 
 static void test_readahead(void)
@@ -302,7 +239,7 @@ static void test_readahead(void)
 	cached_low = get_cached_size();
 	cached_max = cached_max - cached_low;
 
-	tst_resm(TINFO, "read_testfile(0)");
+	tst_res(TINFO, "read_testfile(0)");
 	read_testfile(0, testfile, testfile_size, &read_bytes, &usec, &cached);
 	if (cached > cached_low)
 		cached = cached - cached_low;
@@ -312,7 +249,7 @@ static void test_readahead(void)
 	sync();
 	drop_caches();
 	cached_low = get_cached_size();
-	tst_resm(TINFO, "read_testfile(1)");
+	tst_res(TINFO, "read_testfile(1)");
 	read_testfile(1, testfile, testfile_size, &read_bytes_ra,
 		      &usec_ra, &cached_ra);
 	if (cached_ra > cached_low)
@@ -320,25 +257,25 @@ static void test_readahead(void)
 	else
 		cached_ra = 0;
 
-	tst_resm(TINFO, "read_testfile(0) took: %ld usec", usec);
-	tst_resm(TINFO, "read_testfile(1) took: %ld usec", usec_ra);
+	tst_res(TINFO, "read_testfile(0) took: %ld usec", usec);
+	tst_res(TINFO, "read_testfile(1) took: %ld usec", usec_ra);
 	if (has_file(proc_io_fname, 0)) {
-		tst_resm(TINFO, "read_testfile(0) read: %ld bytes", read_bytes);
-		tst_resm(TINFO, "read_testfile(1) read: %ld bytes",
-			 read_bytes_ra);
+		tst_res(TINFO, "read_testfile(0) read: %ld bytes", read_bytes);
+		tst_res(TINFO, "read_testfile(1) read: %ld bytes",
+			read_bytes_ra);
 		/* actual number of read bytes depends on total RAM */
 		if (read_bytes_ra < read_bytes)
-			tst_resm(TPASS, "readahead saved some I/O");
+			tst_res(TPASS, "readahead saved some I/O");
 		else
-			tst_resm(TFAIL, "readahead failed to save any I/O");
+			tst_res(TFAIL, "readahead failed to save any I/O");
 	} else {
-		tst_resm(TCONF, "Your system doesn't have /proc/pid/io,"
-			 " unable to determine read bytes during test");
+		tst_res(TCONF, "Your system doesn't have /proc/pid/io,"
+			" unable to determine read bytes during test");
 	}
 
-	tst_resm(TINFO, "cache can hold at least: %ld kB", cached_max);
-	tst_resm(TINFO, "read_testfile(0) used cache: %ld kB", cached);
-	tst_resm(TINFO, "read_testfile(1) used cache: %ld kB", cached_ra);
+	tst_res(TINFO, "cache can hold at least: %ld kB", cached_max);
+	tst_res(TINFO, "read_testfile(0) used cache: %ld kB", cached);
+	tst_res(TINFO, "read_testfile(1) used cache: %ld kB", cached_ra);
 
 	if (cached_max * 1024 >= testfile_size) {
 		/*
@@ -346,83 +283,42 @@ static void test_readahead(void)
 		 * for readahead should be at least testfile_size/2
 		 */
 		if (cached_ra * 1024 > testfile_size / 2)
-			tst_resm(TPASS, "using cache as expected");
+			tst_res(TPASS, "using cache as expected");
 		else
-			tst_resm(TWARN, "using less cache than expected");
+			tst_res(TWARN, "using less cache than expected");
 	} else {
-		tst_resm(TCONF, "Page cache on your system is too small "
-			 "to hold whole testfile.");
+		tst_res(TCONF, "Page cache on your system is too small "
+			"to hold whole testfile.");
 	}
 }
 
-int main(int argc, char *argv[])
-{
-	int lc;
-
-	tst_parse_opts(argc, argv, options, help);
-
-	if (opt_fsize)
-		testfile_size = atoi(opt_fsizestr);
-
-	setup();
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
-		test_readahead();
-	}
-	cleanup();
-	tst_exit();
-}
-
 static void setup(void)
 {
-	tst_require_root();
-	tst_tmpdir();
-	TEST_PAUSE;
+	if (opt_fsizestr)
+		testfile_size = SAFE_STRTOL(opt_fsizestr, 1, INT_MAX);
 
 	has_file(drop_caches_fname, 1);
 	has_file(meminfo_fname, 1);
 
 	/* check if readahead is supported */
-	ltp_syscall(__NR_readahead, 0, 0, 0);
+	tst_syscall(__NR_readahead, 0, 0, 0);
 
 	pagesize = getpagesize();
 
-	if (tst_fs_type(cleanup, ".") == TST_TMPFS_MAGIC) {
-		tst_resm(TINFO, "TMPFS detected, creating loop device");
-
-		fs_type = tst_dev_fs_type();
-		device = tst_acquire_device(cleanup);
-		if (!device) {
-			tst_brkm(TCONF, cleanup,
-				"Failed to obtain block device");
-		}

testcases/kernel/syscalls/readahead/readahead02.c static void test_readahead(void) · line 308

Quoted 10 lines
testcases/kernel/syscalls/readahead/readahead02.c +1 -9
-
-		tst_mkfs(cleanup, device, fs_type, NULL, NULL);
-
-		SAFE_MKDIR(cleanup, MNTPOINT, 0755);
-		SAFE_MOUNT(cleanup, device, MNTPOINT, fs_type, 0, NULL);
-		mount_flag = 1;
-
-		sprintf(testfile, MNTPOINT"/testfile");
-	}
+	sprintf(testfile, "%s/testfile", mntpoint);

This just a static string defined on the top of the test.

Quoted 34 lines
testcases/kernel/syscalls/readahead/readahead02.c +10 -15
 	create_testfile();
 }
 
-static void cleanup(void)
-{
-	unlink(testfile);
-	if (mount_flag && tst_umount(MNTPOINT) < 0)
-		tst_resm(TWARN | TERRNO, "umount device:%s failed", device);
-
-	if (device)
-		tst_release_device(device);
-
-	tst_rmdir();
-}
+static struct tst_test test = {
+	.needs_root = 1,
+	.needs_tmpdir = 1,
+	.mount_device = 1,
+	.mntpoint = mntpoint,
+	.setup = setup,
+	.options = options,
+	.test_all = test_readahead,
+};
 
 #else /* __NR_readahead */
-int main(void)
-{
-	tst_brkm(TCONF, NULL, "System doesn't support __NR_readahead");
-}
+	TST_TEST_TCONF("System doesn't support __NR_readahead");
 #endif
Signature · 1 line
2.17.1
Signature · 2 lines
Cyril Hrubis
[email protected]
#5 lore raw

Re: [LTP] [PATCH v3 2/6] syscalls/readahead02: Convert to newlib and cleanup

Hi!

Quoted 1 line
Quoted 1 line

#if defined(__NR_readahead)

Also we do have fallback definitions in lapi/syscalls.h, so __NR_readahead is never undefined, we can safely drop this ifdef.

Signature · 2 lines
Cyril Hrubis
[email protected]
#6 lore raw

Re: [PATCH v3 2/6] syscalls/readahead02: Convert to newlib and cleanup

In reply to Cyril Hrubis

...

Quoted 19 lines
Quoted 8 lines

static int check_ret(long expected_ret)
{

testcases/kernel/syscalls/readahead/readahead02.c +3 -3
-     if (expected_ret == TEST_RETURN) {
-             tst_resm(TPASS, "expected ret success - "
-                      "returned value = %ld", TEST_RETURN);
+     if (expected_ret == TST_RET) {
+             tst_res(TPASS, "expected ret success - "
+                     "returned value = %ld", TST_RET);

testcases/kernel/syscalls/readahead/readahead02.c line 59

Quoted 10 lines
testcases/kernel/syscalls/readahead/readahead02.c +3 -3
              return 0;
      }
-     tst_resm(TFAIL | TTERRNO, "unexpected failure - "
-              "returned value = %ld, expected: %ld",
-              TEST_RETURN, expected_ret);
+     tst_res(TFAIL | TTERRNO, "unexpected failure - "
+             "returned value = %ld, expected: %ld",
+             TST_RET, expected_ret);
      return 1;
 }
Quoted 2 lines

I would be happier if we got rid of this function.

Done. (In next patch)

Quoted 4 lines

Maybe we should just move the loop from read_file() to a separate function do_readahead() then the indentation would be one level less there and the code could be put directly there.

...

Quoted 11 lines

Review

Quoted 3 lines
      if (gettimeofday(&now, NULL) == -1)
(quoted diff) +1 -1
-             tst_brkm(TBROK | TERRNO, cleanup, "gettimeofday failed");
+             tst_brk(TBROK | TERRNO, "gettimeofday failed");

We should really switch to posix timers here, using wall clock for elapsed time measurements in not a good idea for several reasons.

And we even do have nice API for this, see:

https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2221-measuring-elapsed-time-and-helper-functions

Nice! done in a separate patch.

...

Quoted 6 lines

testcases/kernel/syscalls/readahead/readahead02.c static void test_readahead(void) · line 308

Quoted 3 lines
testcases/kernel/syscalls/readahead/readahead02.c +1 -2
-             sprintf(testfile, MNTPOINT"/testfile");
-     }
+     sprintf(testfile, "%s/testfile", mntpoint);

This just a static string defined on the top of the test.

It is going to be modified per test case in patch
"test readahead() on an overlayfs file"

v4 posted.

Thanks,
Amir.

#7 lore raw

PATCH v3 3/6 syscalls/readahead02: abort test if readahead syscall fails

In reply to Amir Goldstein

There is no reason to continue the test if readahead syscall fails and we can also check and report TCONF if filesystem does not support readahead.

---
 .../kernel/syscalls/readahead/readahead02.c   | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +12 -9
index c739d3ba2..f77e7c66c 100644
@@ -45,16 +45,17 @@ static struct tst_option options[] = {
 	{NULL, NULL, NULL}
 };
 
-static int check_ret(long expected_ret)
+static int check_ret(void)
 {
-	if (expected_ret == TST_RET) {
-		tst_res(TPASS, "expected ret success - "
-			"returned value = %ld", TST_RET);
+	if (TST_RET == 0)
 		return 0;
+	if (TST_ERR == EINVAL) {
+		tst_res(TCONF, "readahead not supported on %s",
+			tst_device->fs_type);
+	} else {
+		tst_res(TFAIL | TTERRNO, "readahead failed on %s",
+			tst_device->fs_type);
 	}
-	tst_res(TFAIL | TTERRNO, "unexpected failure - "
-		"returned value = %ld, expected: %ld",
-		TST_RET, expected_ret);
 	return 1;
 }
 
@@ -163,8 +164,8 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 		do {
 			TEST(readahead(fd, offset, fsize - offset));
 			if (TST_RET != 0) {
-				check_ret(0);
-				break;
+				SAFE_CLOSE(fd);
+				return;
 			}
 
 			/* estimate max readahead size based on first call */
@@ -252,6 +253,8 @@ static void test_readahead(void)
 	tst_res(TINFO, "read_testfile(1)");
 	read_testfile(1, testfile, testfile_size, &read_bytes_ra,
 		      &usec_ra, &cached_ra);
+	if (check_ret())
+		return;
 	if (cached_ra > cached_low)
 		cached_ra = cached_ra - cached_low;
 	else
Signature · 1 line
2.17.1
#8 lore raw

Re: [PATCH v3 3/6] syscalls/readahead02: abort test if readahead syscall fails

Hi!

Quoted 3 lines

There is no reason to continue the test if readahead syscall fails and we can also check and report TCONF if filesystem does not support readahead.

This looks good, but I would like to get rid of the check_ret function as I said in the first case.

So maybe we can leave it in the cleanup but move the actuall code in this patch. Does that sounds good to you?

Quoted 57 lines
---
 .../kernel/syscalls/readahead/readahead02.c   | 21 +++++++++++--------
 1 file changed, 12 insertions(+), 9 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +12 -9
index c739d3ba2..f77e7c66c 100644
@@ -45,16 +45,17 @@ static struct tst_option options[] = {
 	{NULL, NULL, NULL}
 };
 
-static int check_ret(long expected_ret)
+static int check_ret(void)
 {
-	if (expected_ret == TST_RET) {
-		tst_res(TPASS, "expected ret success - "
-			"returned value = %ld", TST_RET);
+	if (TST_RET == 0)
 		return 0;
+	if (TST_ERR == EINVAL) {
+		tst_res(TCONF, "readahead not supported on %s",
+			tst_device->fs_type);
+	} else {
+		tst_res(TFAIL | TTERRNO, "readahead failed on %s",
+			tst_device->fs_type);
 	}
-	tst_res(TFAIL | TTERRNO, "unexpected failure - "
-		"returned value = %ld, expected: %ld",
-		TST_RET, expected_ret);
 	return 1;
 }
 
@@ -163,8 +164,8 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 		do {
 			TEST(readahead(fd, offset, fsize - offset));
 			if (TST_RET != 0) {
-				check_ret(0);
-				break;
+				SAFE_CLOSE(fd);
+				return;
 			}
 
 			/* estimate max readahead size based on first call */
@@ -252,6 +253,8 @@ static void test_readahead(void)
 	tst_res(TINFO, "read_testfile(1)");
 	read_testfile(1, testfile, testfile_size, &read_bytes_ra,
 		      &usec_ra, &cached_ra);
+	if (check_ret())
+		return;
 	if (cached_ra > cached_low)
 		cached_ra = cached_ra - cached_low;
 	else
Signature · 1 line
2.17.1
Signature · 2 lines
Cyril Hrubis
[email protected]
#9 lore raw

Re: [PATCH v3 3/6] syscalls/readahead02: abort test if readahead syscall fails

Quoted 11 lines · Cyril Hrubis

Hi!

Quoted 3 lines

There is no reason to continue the test if readahead syscall fails and we can also check and report TCONF if filesystem does not support readahead.

This looks good, but I would like to get rid of the check_ret function as I said in the first case.

So maybe we can leave it in the cleanup but move the actuall code in this patch. Does that sounds good to you?

Will do.

Thanks,
Amir.

#10 lore raw

PATCH v3 4/6 syscalls/readahead02: test readahead() on an overlayfs file

In reply to Amir Goldstein

Repeat the test case on an overlayfs file.

The new test case is a regression test for kernel commit b833a3660394 ("ovl: add ovl_fadvise()") which fixes a regression of readahead() on an overlay file that was introduced by kernel commit 5b910bd615ba ("ovl: fix GPF in swapfile_activate of file from overlayfs over xfs").

---
 .../kernel/syscalls/readahead/readahead02.c   | 85 ++++++++++++++++---
 1 file changed, 73 insertions(+), 12 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +73 -12
index f77e7c66c..56a8496ff 100644
@@ -14,6 +14,7 @@
 #include <sys/types.h>
 #include <sys/syscall.h>
 #include <sys/mman.h>
+#include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/time.h>
@@ -34,8 +35,14 @@ static const char meminfo_fname[] = "/proc/meminfo";
 static size_t testfile_size = 64 * 1024 * 1024;
 static char *opt_fsizestr;
 static int pagesize;
+static unsigned long cached_max;
+static int ovl_mounted;
 
 #define MNTPOINT        "mntpoint"
+#define OVL_LOWER	MNTPOINT"/lower"
+#define OVL_UPPER	MNTPOINT"/upper"
+#define OVL_WORK	MNTPOINT"/work"
+#define OVL_MNT		MNTPOINT"/ovl"
 #define MIN_SANE_READAHEAD (4u * 1024u)
 
 static const char mntpoint[] = MNTPOINT;
@@ -45,16 +52,28 @@ static struct tst_option options[] = {
 	{NULL, NULL, NULL}
 };
 
-static int check_ret(void)
+static struct tcase {
+	const char *tname;
+	int use_overlay;
+} tcases[] = {
+	{ "readahead on file", 0 },
+	{ "readahead on overlayfs file", 1 },
+};
+
+static int readahead_supported = 1;
+
+static int check_ret(struct tcase *tc)
 {
 	if (TST_RET == 0)
 		return 0;
-	if (TST_ERR == EINVAL) {
+	if (TST_ERR == EINVAL &&
+	    (!tc->use_overlay || !readahead_supported)) {
+		readahead_supported = 0;
 		tst_res(TCONF, "readahead not supported on %s",
 			tst_device->fs_type);
 	} else {
 		tst_res(TFAIL | TTERRNO, "readahead failed on %s",
-			tst_device->fs_type);
+			tc->use_overlay ? "overlayfs" : tst_device->fs_type);
 	}
 	return 1;
 }
@@ -112,12 +131,13 @@ static unsigned long get_cached_size(void)
 	return parse_entry(meminfo_fname, entry);
 }
 
-static void create_testfile(void)
+static void create_testfile(int use_overlay)
 {
 	int fd;
 	char *tmp;
 	size_t i;
 
+	sprintf(testfile, "%s/testfile", use_overlay ? OVL_MNT : MNTPOINT);
 	tst_res(TINFO, "creating test file of size: %zu", testfile_size);
 	tmp = SAFE_MALLOC(pagesize);
 
@@ -224,21 +244,33 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 	SAFE_CLOSE(fd);
 }
 
-static void test_readahead(void)
+static void test_readahead(unsigned int n)
 {
 	unsigned long read_bytes, read_bytes_ra;
 	long usec, usec_ra;
-	unsigned long cached_max, cached_low, cached, cached_ra;
+	unsigned long cached_high, cached_low, cached, cached_ra;
 	char proc_io_fname[128];
+	struct tcase *tc = &tcases[n];
+
+	tst_res(TINFO, "Test #%d: %s", n, tc->tname);
+
 	sprintf(proc_io_fname, "/proc/%u/io", getpid());
 
+	if (tc->use_overlay && !ovl_mounted) {
+		tst_res(TCONF,
+			"overlayfs is not configured in this kernel.");
+		return;
+	}
+
+	create_testfile(tc->use_overlay);
+
 	/* find out how much can cache hold if we read whole file */
 	read_testfile(0, testfile, testfile_size, &read_bytes, &usec, &cached);
-	cached_max = get_cached_size();
+	cached_high = get_cached_size();
 	sync();
 	drop_caches();
 	cached_low = get_cached_size();
-	cached_max = cached_max - cached_low;
+	cached_max = MAX(cached_max, cached_high - cached_low);
 
 	tst_res(TINFO, "read_testfile(0)");
 	read_testfile(0, testfile, testfile_size, &read_bytes, &usec, &cached);
@@ -253,7 +285,7 @@ static void test_readahead(void)
 	tst_res(TINFO, "read_testfile(1)");
 	read_testfile(1, testfile, testfile_size, &read_bytes_ra,
 		      &usec_ra, &cached_ra);
-	if (check_ret())
+	if (check_ret(tc))
 		return;
 	if (cached_ra > cached_low)
 		cached_ra = cached_ra - cached_low;
@@ -295,6 +327,28 @@ static void test_readahead(void)
 	}
 }
 
+static void setup_overlay(void)
+{
+	int ret;
+
+	/* Setup an overlay mount with lower dir and file */
+	SAFE_MKDIR(OVL_LOWER, 0755);
+	SAFE_MKDIR(OVL_UPPER, 0755);
+	SAFE_MKDIR(OVL_WORK, 0755);
+	SAFE_MKDIR(OVL_MNT, 0755);
+	ret = mount("overlay", OVL_MNT, "overlay", 0, "lowerdir="OVL_LOWER
+		    ",upperdir="OVL_UPPER",workdir="OVL_WORK);
+	if (ret < 0) {
+		if (errno == ENODEV) {
+			tst_res(TINFO,
+				"overlayfs is not configured in this kernel.");
+			return;
+		}
+		tst_brk(TBROK | TERRNO, "overlayfs mount failed");
+	}
+	ovl_mounted = 1;
+}
+
 static void setup(void)
 {
 	if (opt_fsizestr)
@@ -308,8 +362,13 @@ static void setup(void)
 
 	pagesize = getpagesize();
 
-	sprintf(testfile, "%s/testfile", mntpoint);
-	create_testfile();
+	setup_overlay();
+}
+
+static void cleanup(void)
+{
+	if (ovl_mounted)
+		SAFE_UMOUNT(OVL_MNT);
 }
 
 static struct tst_test test = {
@@ -318,8 +377,10 @@ static struct tst_test test = {
 	.mount_device = 1,
 	.mntpoint = mntpoint,
 	.setup = setup,
+	.cleanup = cleanup,
 	.options = options,
-	.test_all = test_readahead,
+	.test = test_readahead,
+	.tcnt = ARRAY_SIZE(tcases),
 };
 
 #else /* __NR_readahead */
Signature · 1 line
2.17.1
#11 lore raw

PATCH v3 5/6 syscalls/readahead02: test readahead using posix_fadvise()

In reply to Amir Goldstein

The call to posix_fadvise(POSIX_FADV_WILLNEED) should have the same effect as the call to readahead() syscall.

Repeat the test cases for local file and overlayfs file with posix_fadvise().

The new test case is a regression test for kernel commit b833a3660394 ("ovl: add ovl_fadvise()") which fixes a regression of fadvise() on an overlay file that was introduced by kernel commit 5b910bd615ba ("ovl: fix GPF in swapfile_activate of file from overlayfs over xfs").

---
 .../kernel/syscalls/readahead/readahead02.c   | 41 +++++++++++++++----
 1 file changed, 34 insertions(+), 7 deletions(-)
testcases/kernel/syscalls/readahead/readahead02.c +34 -7
index 56a8496ff..d5cc7677b 100644
@@ -55,24 +55,49 @@ static struct tst_option options[] = {
 static struct tcase {
 	const char *tname;
 	int use_overlay;
+	int use_fadvise;
 } tcases[] = {
-	{ "readahead on file", 0 },
-	{ "readahead on overlayfs file", 1 },
+	{ "readahead on file", 0, 0 },
+	{ "readahead on overlayfs file", 1, 0 },
+	{ "POSIX_FADV_WILLNEED on file", 0, 1 },
+	{ "POSIX_FADV_WILLNEED on overlayfs file", 1, 1 },
 };
 
+static int fadvise_willneed(int fd, off_t offset, size_t len)
+{
+	/* Should have the same effect as readahead() syscall */
+	return posix_fadvise(fd, offset, len, POSIX_FADV_WILLNEED);
+}
+
+static int libc_readahead(int fd, off_t offset, size_t len)
+{
+	return readahead(fd, offset, len);
+}
+
+typedef int (*readahead_func_t)(int, off_t, size_t);
+static readahead_func_t readahead_func = libc_readahead;
+
 static int readahead_supported = 1;
+static int fadvise_supported = 1;
 
 static int check_ret(struct tcase *tc)
 {
 	if (TST_RET == 0)
 		return 0;
-	if (TST_ERR == EINVAL &&
-	    (!tc->use_overlay || !readahead_supported)) {
+	/* posix_fadvise returns error number (not in errno) */
+	if (tc->use_fadvise && (TST_ERR = TST_RET) == EINVAL &&
+	    (!tc->use_overlay || !fadvise_supported)) {
+		fadvise_supported = 0;
+		tst_res(TCONF, "fadvise not supported on %s",
+			tst_device->fs_type);
+	} else if (TST_ERR == EINVAL &&
+		   (!tc->use_overlay || !readahead_supported)) {
 		readahead_supported = 0;
 		tst_res(TCONF, "readahead not supported on %s",
 			tst_device->fs_type);
 	} else {
-		tst_res(TFAIL | TTERRNO, "readahead failed on %s",
+		tst_res(TFAIL | TTERRNO, "%s failed on %s",
+			tc->use_fadvise ? "fadvise" : "readahead",
 			tc->use_overlay ? "overlayfs" : tst_device->fs_type);
 	}
 	return 1;
@@ -152,7 +177,6 @@ static void create_testfile(int use_overlay)
 	free(tmp);
 }
 
-
 /* read_testfile - mmap testfile and read every page.
  * This functions measures how many I/O and time it takes to fully
  * read contents of test file.
@@ -182,7 +206,7 @@ static void read_testfile(int do_readahead, const char *fname, size_t fsize,
 	if (do_readahead) {
 		cached_start = get_cached_size();
 		do {
-			TEST(readahead(fd, offset, fsize - offset));
+			TEST(readahead_func(fd, offset, fsize - offset));
 			if (TST_RET != 0) {
 				SAFE_CLOSE(fd);
 				return;
@@ -264,6 +288,9 @@ static void test_readahead(unsigned int n)
 
 	create_testfile(tc->use_overlay);
 
+	/* Use either readahead() syscall or POSIX_FADV_WILLNEED */
+	readahead_func = tc->use_fadvise ? fadvise_willneed : libc_readahead;
+
 	/* find out how much can cache hold if we read whole file */
 	read_testfile(0, testfile, testfile_size, &read_bytes, &usec, &cached);
 	cached_high = get_cached_size();
Signature · 1 line
2.17.1
#12 lore raw

PATCH v3 6/6 syscalls/readahead02: fail test if readahead did not use any cache

In reply to Amir Goldstein

The heuristic of failing the test only if not saving any io has false negatives with overlayfs readahead() bug, but readahead() with overlayfs always warns on "using less cache then expected", when actually, it is using no cache at all.

Add another condition to fail the test if readahead did not use any cache at all, which always detected the overlayfs bug.

---
 testcases/kernel/syscalls/readahead/readahead02.c | 2 ++
 1 file changed, 2 insertions(+)
testcases/kernel/syscalls/readahead/readahead02.c +2 -0
index d5cc7677b..657461905 100644
@@ -346,6 +346,8 @@ static void test_readahead(unsigned int n)
 		 */
 		if (cached_ra * 1024 > testfile_size / 2)
 			tst_res(TPASS, "using cache as expected");
+		else if (!cached_ra)
+			tst_res(TFAIL, "readahead failed to use any cache");
 		else
 			tst_res(TWARN, "using less cache than expected");
 	} else {
Signature · 1 line
2.17.1
#13 lore raw
In reply to Amir Goldstein
Quoted 3 lines · Amir Goldstein

Cyril,

Ping.

The affected kernel code as well as fix commits are now in v4.19. Pushed rebased branch to:
https://github.com/amir73il/ltp/commits/overlayfs-devel

Also, there is an unrelated (besides the word "fadvise") cleanup branch
https://github.com/amir73il/ltp/commits/fadvise_cleanup
that converts the fadvise tests to newlib.

Thanks,
Amir.

Quoted 44 lines

The following series adds test covergae for readahead() syscall over overlayfs file and adds test coverage for a specific posix_fadvise() syscall advice (POSIX_FADV_WILLNEED).

So far, the posix_fadvise syscall tests in LTP only test for error conditions, but not functionality.

The functionality of the advice POSIX_FADV_WILLNEED is identical to readahead() and since kernel commit 3d8f7615319b ("vfs: implement readahead(2) using POSIX_FADV_WILLNEED"), the implementations are also bound together.

To add test coverage of the advice POSIX_FADV_WILLNEED, I decided not to duplicate the readahead() functional test and add test cases to readahead02 that use the POSIX_FADV_WILLNEED implementation.

Thanks,
Amir.

Changes since v2:
- Runtime check for readahead/fadvise support
- Abort test case immediately if syscall fail

Changes since v1:
- Fix bugs in loop invocation of test (i.e. -i 2)
- Address review comments from Jan Stancek
- Make cached_max a global maximum over all test cases
- Improve reliability of overlayfs readahead test case failure

Amir Goldstein (6):
syscalls/readahead01: Convert to newlib syscalls/readahead02: Convert to newlib and cleanup syscalls/readahead02: abort test if readahead syscall fails syscalls/readahead02: test readahead() on an overlayfs file syscalls/readahead02: test readahead using posix_fadvise() syscalls/readahead02: fail test if readahead did not use any cache

 .../kernel/syscalls/readahead/readahead01.c   | 119 ++----
 .../kernel/syscalls/readahead/readahead02.c   | 339 +++++++++---------
 2 files changed, 198 insertions(+), 260 deletions(-)

--
2.17.1