Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] selftests/mincore: get readahead size for check_file_mmap()
@ 2021-04-19  5:46 Jeffle Xu
  2021-04-30  5:26 ` JeffleXu
  0 siblings, 1 reply; 2+ messages in thread
From: Jeffle Xu @ 2021-04-19  5:46 UTC (permalink / raw)
  To: ricardo.canuelo, shuah; +Cc: linux-kselftest, Jeffle Xu, James Wang

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v4] selftests/mincore: get readahead size for check_file_mmap()
  2021-04-19  5:46 [PATCH v4] selftests/mincore: get readahead size for check_file_mmap() Jeffle Xu
@ 2021-04-30  5:26 ` JeffleXu
  0 siblings, 0 replies; 2+ messages in thread
From: JeffleXu @ 2021-04-30  5:26 UTC (permalink / raw)
  To: ricardo.canuelo, shuah; +Cc: linux-kselftest, James Wang

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19  5:46 [PATCH v4] selftests/mincore: get readahead size for check_file_mmap() Jeffle Xu
2021-04-30  5:26 ` JeffleXu

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git