[PATCH v4] selftests/mincore: get readahead size for check_file_mmap()

[PATCH v4] selftests/mincore: get readahead size for check_file_mmap()

[Jeffle Xu]

The readahead size used to be 2MB, thus it's reasonable to set the file
size as 4MB when checking check_file_mmap().

However since commit c2e4cd57cfa1 ("block: lift setting the readahead
size into the block layer"), readahead size could be as large as twice
the io_opt, and thus the hardcoded file size no longer works.
check_file_mmap() may report "Read-ahead pages reached the end of the
file" when the readahead size actually exceeds the file size in this
case.

To fix this issue, read the exact readahead window size via BLKRAGET
ioctl. Since now we have the readahead window size, take a more
fine-grained check. It is worth noting that this fine-grained check may
be broken as the sync readahead algorithm of kernel changes. It may be
acceptable since the algorithm of readahead ranging should be quite
stable, and we could tune the test case accorddingly if the algorithm
indeed changes.

Reported-by: James Wang <jnwang@linux.alibaba.com>
Acked-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
changes since v3:
- make the check more fine-grained since we have the exact readahead
  window size now, as suggested by Ricardo Cañuelo

chnages since v2:
- add 'Reported-by'

chnages since v1:
- add the test name "mincore" in the subject line
- add the error message in commit message
- rename @filesize to @file_size to keep a more consistent naming
  convention
---
 .../selftests/mincore/mincore_selftest.c      | 96 +++++++++++++------
 1 file changed, 68 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
index 5a1e85ff5d32..369b35af4b4f 100644
--- a/tools/testing/selftests/mincore/mincore_selftest.c
+++ b/tools/testing/selftests/mincore/mincore_selftest.c
@@ -15,6 +15,11 @@
 #include <string.h>
 #include <fcntl.h>
 #include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <sys/sysmacros.h>
+#include <sys/mount.h>
 
 #include "../kselftest.h"
 #include "../kselftest_harness.h"
@@ -193,12 +198,44 @@ TEST(check_file_mmap)
 	int retval;
 	int page_size;
 	int fd;
-	int i;
+	int i, start, end;
 	int ra_pages = 0;
+	long ra_size, file_size;
+	struct stat stats;
+	dev_t devt;
+	unsigned int major, minor;
+	char devpath[32];
+
+	retval = stat(".", &stats);
+	ASSERT_EQ(0, retval) {
+		TH_LOG("Can't stat pwd: %s", strerror(errno));
+	}
+
+	devt = stats.st_dev;
+	major = major(devt);
+	minor = minor(devt);
+	snprintf(devpath, sizeof(devpath), "/dev/block/%u:%u", major, minor);
+
+	fd = open(devpath, O_RDONLY);
+	ASSERT_NE(-1, fd) {
+		TH_LOG("Can't open underlying disk %s", strerror(errno));
+	}
+
+	retval = ioctl(fd, BLKRAGET, &ra_size);
+	ASSERT_EQ(0, retval) {
+		TH_LOG("Error ioctl with the underlying disk: %s", strerror(errno));
+	}
+
+	/*
+	 * BLKRAGET ioctl returns the readahead size in sectors (512 bytes).
+	 * Make file_size large enough to contain the readahead window.
+	 */
+	ra_size *= 512;
+	file_size = ra_size * 2;
 
 	page_size = sysconf(_SC_PAGESIZE);
-	vec_size = FILE_SIZE / page_size;
-	if (FILE_SIZE % page_size)
+	vec_size = file_size / page_size;
+	if (file_size % page_size)
 		vec_size++;
 
 	vec = calloc(vec_size, sizeof(unsigned char));
@@ -213,7 +250,7 @@ TEST(check_file_mmap)
 			strerror(errno));
 	}
 	errno = 0;
-	retval = fallocate(fd, 0, 0, FILE_SIZE);
+	retval = fallocate(fd, 0, 0, file_size);
 	ASSERT_EQ(0, retval) {
 		TH_LOG("Error allocating space for the temporary file: %s",
 			strerror(errno));
@@ -223,12 +260,12 @@ TEST(check_file_mmap)
 	 * Map the whole file, the pages shouldn't be fetched yet.
 	 */
 	errno = 0;
-	addr = mmap(NULL, FILE_SIZE, PROT_READ | PROT_WRITE,
+	addr = mmap(NULL, file_size, PROT_READ | PROT_WRITE,
 			MAP_SHARED, fd, 0);
 	ASSERT_NE(MAP_FAILED, addr) {
 		TH_LOG("mmap error: %s", strerror(errno));
 	}
-	retval = mincore(addr, FILE_SIZE, vec);
+	retval = mincore(addr, file_size, vec);
 	ASSERT_EQ(0, retval);
 	for (i = 0; i < vec_size; i++) {
 		ASSERT_EQ(0, vec[i]) {
@@ -240,38 +277,41 @@ TEST(check_file_mmap)
 	 * Touch a page in the middle of the mapping. We expect the next
 	 * few pages (the readahead window) to be populated too.
 	 */
-	addr[FILE_SIZE / 2] = 1;
-	retval = mincore(addr, FILE_SIZE, vec);
+	addr[file_size / 2] = 1;
+	retval = mincore(addr, file_size, vec);
 	ASSERT_EQ(0, retval);
-	ASSERT_EQ(1, vec[FILE_SIZE / 2 / page_size]) {
-		TH_LOG("Page not found in memory after use");
-	}
 
-	i = FILE_SIZE / 2 / page_size + 1;
-	while (i < vec_size && vec[i]) {
-		ra_pages++;
-		i++;
-	}
-	EXPECT_GT(ra_pages, 0) {
-		TH_LOG("No read-ahead pages found in memory");
-	}
+	/*
+	 * Readahead window is [start, end). So far the sync readahead
+	 * algorithm takes the page that triggers the page fault as the
+	 * midpoint.
+	 */
+	ra_pages = ra_size / page_size;
+	start = file_size / 2 / page_size - ra_pages / 2;
+	end = start + ra_pages;
 
-	EXPECT_LT(i, vec_size) {
-		TH_LOG("Read-ahead pages reached the end of the file");
+	/*
+	 * Check there's no hole in the readahead window.
+	 */
+	for (i = start; i < end; i++) {
+		ASSERT_EQ(1, vec[i]) {
+			TH_LOG("Hole found in read-ahead window");
+		}
 	}
+
 	/*
-	 * End of the readahead window. The rest of the pages shouldn't
-	 * be in memory.
+	 * Check there's no page beyond the readahead window.
 	 */
-	if (i < vec_size) {
-		while (i < vec_size && !vec[i])
-			i++;
-		EXPECT_EQ(vec_size, i) {
+	for (i = 0; i < vec_size; i++) {
+		if (i == start)
+			i = end;
+
+		EXPECT_EQ(0, vec[i]) {
 			TH_LOG("Unexpected page in memory beyond readahead window");
 		}
 	}
 
-	munmap(addr, FILE_SIZE);
+	munmap(addr, file_size);
 	close(fd);
 	free(vec);
 }
-- 
2.27.0

Re: [PATCH v4] selftests/mincore: get readahead size for check_file_mmap()

[JeffleXu]

ping...

On 4/19/21 1:46 PM, Jeffle Xu wrote:
> The readahead size used to be 2MB, thus it's reasonable to set the file
> size as 4MB when checking check_file_mmap().
> 
> However since commit c2e4cd57cfa1 ("block: lift setting the readahead
> size into the block layer"), readahead size could be as large as twice
> the io_opt, and thus the hardcoded file size no longer works.
> check_file_mmap() may report "Read-ahead pages reached the end of the
> file" when the readahead size actually exceeds the file size in this
> case.
> 
> To fix this issue, read the exact readahead window size via BLKRAGET
> ioctl. Since now we have the readahead window size, take a more
> fine-grained check. It is worth noting that this fine-grained check may
> be broken as the sync readahead algorithm of kernel changes. It may be
> acceptable since the algorithm of readahead ranging should be quite
> stable, and we could tune the test case accorddingly if the algorithm
> indeed changes.
> 
> Reported-by: James Wang <jnwang@linux.alibaba.com>
> Acked-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
> changes since v3:
> - make the check more fine-grained since we have the exact readahead
>   window size now, as suggested by Ricardo Cañuelo
> 
> chnages since v2:
> - add 'Reported-by'
> 
> chnages since v1:
> - add the test name "mincore" in the subject line
> - add the error message in commit message
> - rename @filesize to @file_size to keep a more consistent naming
>   convention
> ---
>  .../selftests/mincore/mincore_selftest.c      | 96 +++++++++++++------
>  1 file changed, 68 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
> index 5a1e85ff5d32..369b35af4b4f 100644
> --- a/tools/testing/selftests/mincore/mincore_selftest.c
> +++ b/tools/testing/selftests/mincore/mincore_selftest.c
> @@ -15,6 +15,11 @@
>  #include <string.h>
>  #include <fcntl.h>
>  #include <string.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <sys/sysmacros.h>
> +#include <sys/mount.h>
>  
>  #include "../kselftest.h"
>  #include "../kselftest_harness.h"
> @@ -193,12 +198,44 @@ TEST(check_file_mmap)
>  	int retval;
>  	int page_size;
>  	int fd;
> -	int i;
> +	int i, start, end;
>  	int ra_pages = 0;
> +	long ra_size, file_size;
> +	struct stat stats;
> +	dev_t devt;
> +	unsigned int major, minor;
> +	char devpath[32];
> +
> +	retval = stat(".", &stats);
> +	ASSERT_EQ(0, retval) {
> +		TH_LOG("Can't stat pwd: %s", strerror(errno));
> +	}
> +
> +	devt = stats.st_dev;
> +	major = major(devt);
> +	minor = minor(devt);
> +	snprintf(devpath, sizeof(devpath), "/dev/block/%u:%u", major, minor);
> +
> +	fd = open(devpath, O_RDONLY);
> +	ASSERT_NE(-1, fd) {
> +		TH_LOG("Can't open underlying disk %s", strerror(errno));
> +	}
> +
> +	retval = ioctl(fd, BLKRAGET, &ra_size);
> +	ASSERT_EQ(0, retval) {
> +		TH_LOG("Error ioctl with the underlying disk: %s", strerror(errno));
> +	}
> +
> +	/*
> +	 * BLKRAGET ioctl returns the readahead size in sectors (512 bytes).
> +	 * Make file_size large enough to contain the readahead window.
> +	 */
> +	ra_size *= 512;
> +	file_size = ra_size * 2;
>  
>  	page_size = sysconf(_SC_PAGESIZE);
> -	vec_size = FILE_SIZE / page_size;
> -	if (FILE_SIZE % page_size)
> +	vec_size = file_size / page_size;
> +	if (file_size % page_size)
>  		vec_size++;
>  
>  	vec = calloc(vec_size, sizeof(unsigned char));
> @@ -213,7 +250,7 @@ TEST(check_file_mmap)
>  			strerror(errno));
>  	}
>  	errno = 0;
> -	retval = fallocate(fd, 0, 0, FILE_SIZE);
> +	retval = fallocate(fd, 0, 0, file_size);
>  	ASSERT_EQ(0, retval) {
>  		TH_LOG("Error allocating space for the temporary file: %s",
>  			strerror(errno));
> @@ -223,12 +260,12 @@ TEST(check_file_mmap)
>  	 * Map the whole file, the pages shouldn't be fetched yet.
>  	 */
>  	errno = 0;
> -	addr = mmap(NULL, FILE_SIZE, PROT_READ | PROT_WRITE,
> +	addr = mmap(NULL, file_size, PROT_READ | PROT_WRITE,
>  			MAP_SHARED, fd, 0);
>  	ASSERT_NE(MAP_FAILED, addr) {
>  		TH_LOG("mmap error: %s", strerror(errno));
>  	}
> -	retval = mincore(addr, FILE_SIZE, vec);
> +	retval = mincore(addr, file_size, vec);
>  	ASSERT_EQ(0, retval);
>  	for (i = 0; i < vec_size; i++) {
>  		ASSERT_EQ(0, vec[i]) {
> @@ -240,38 +277,41 @@ TEST(check_file_mmap)
>  	 * Touch a page in the middle of the mapping. We expect the next
>  	 * few pages (the readahead window) to be populated too.
>  	 */
> -	addr[FILE_SIZE / 2] = 1;
> -	retval = mincore(addr, FILE_SIZE, vec);
> +	addr[file_size / 2] = 1;
> +	retval = mincore(addr, file_size, vec);
>  	ASSERT_EQ(0, retval);
> -	ASSERT_EQ(1, vec[FILE_SIZE / 2 / page_size]) {
> -		TH_LOG("Page not found in memory after use");
> -	}
>  
> -	i = FILE_SIZE / 2 / page_size + 1;
> -	while (i < vec_size && vec[i]) {
> -		ra_pages++;
> -		i++;
> -	}
> -	EXPECT_GT(ra_pages, 0) {
> -		TH_LOG("No read-ahead pages found in memory");
> -	}
> +	/*
> +	 * Readahead window is [start, end). So far the sync readahead
> +	 * algorithm takes the page that triggers the page fault as the
> +	 * midpoint.
> +	 */
> +	ra_pages = ra_size / page_size;
> +	start = file_size / 2 / page_size - ra_pages / 2;
> +	end = start + ra_pages;
>  
> -	EXPECT_LT(i, vec_size) {
> -		TH_LOG("Read-ahead pages reached the end of the file");
> +	/*
> +	 * Check there's no hole in the readahead window.
> +	 */
> +	for (i = start; i < end; i++) {
> +		ASSERT_EQ(1, vec[i]) {
> +			TH_LOG("Hole found in read-ahead window");
> +		}
>  	}
> +
>  	/*
> -	 * End of the readahead window. The rest of the pages shouldn't
> -	 * be in memory.
> +	 * Check there's no page beyond the readahead window.
>  	 */
> -	if (i < vec_size) {
> -		while (i < vec_size && !vec[i])
> -			i++;
> -		EXPECT_EQ(vec_size, i) {
> +	for (i = 0; i < vec_size; i++) {
> +		if (i == start)
> +			i = end;
> +
> +		EXPECT_EQ(0, vec[i]) {
>  			TH_LOG("Unexpected page in memory beyond readahead window");
>  		}
>  	}
>  
> -	munmap(addr, FILE_SIZE);
> +	munmap(addr, file_size);
>  	close(fd);
>  	free(vec);
>  }
> 

-- 
Thanks,
Jeffle