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

[PATCH] selftests: 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 could be as large as twice the
io_opt, and thus the hardcoded file size no longer works.

Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 .../selftests/mincore/mincore_selftest.c      | 57 +++++++++++++++----
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
index 5a1e85ff5d32..cf0c86697403 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"
@@ -195,10 +200,42 @@ TEST(check_file_mmap)
 	int fd;
 	int i;
 	int ra_pages = 0;
+	long ra_size, filesize;
+	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 filesize large enough to contain the readahead window.
+	 */
+	ra_size *= 512;
+	filesize = ra_size * 2;
 
 	page_size = sysconf(_SC_PAGESIZE);
-	vec_size = FILE_SIZE / page_size;
-	if (FILE_SIZE % page_size)
+	vec_size = filesize / page_size;
+	if (filesize % 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, filesize);
 	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, filesize, 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, filesize, vec);
 	ASSERT_EQ(0, retval);
 	for (i = 0; i < vec_size; i++) {
 		ASSERT_EQ(0, vec[i]) {
@@ -240,14 +277,14 @@ 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[filesize / 2] = 1;
+	retval = mincore(addr, filesize, vec);
 	ASSERT_EQ(0, retval);
-	ASSERT_EQ(1, vec[FILE_SIZE / 2 / page_size]) {
+	ASSERT_EQ(1, vec[filesize / 2 / page_size]) {
 		TH_LOG("Page not found in memory after use");
 	}
 
-	i = FILE_SIZE / 2 / page_size + 1;
+	i = filesize / 2 / page_size + 1;
 	while (i < vec_size && vec[i]) {
 		ra_pages++;
 		i++;
@@ -271,7 +308,7 @@ TEST(check_file_mmap)
 		}
 	}
 
-	munmap(addr, FILE_SIZE);
+	munmap(addr, filesize);
 	close(fd);
 	free(vec);
 }
-- 
2.27.0

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

[JeffleXu]

On 3/30/21 8:40 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 could be as large as twice the
> io_opt, and thus the hardcoded file size no longer works.
> 
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>

Forgot to mention that otherwise, "Read-ahead pages reached the end of
the file" is reported in check_file_mmap().

> ---
>  .../selftests/mincore/mincore_selftest.c      | 57 +++++++++++++++----
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
> index 5a1e85ff5d32..cf0c86697403 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"
> @@ -195,10 +200,42 @@ TEST(check_file_mmap)
>  	int fd;
>  	int i;
>  	int ra_pages = 0;
> +	long ra_size, filesize;
> +	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 filesize large enough to contain the readahead window.
> +	 */
> +	ra_size *= 512;
> +	filesize = ra_size * 2;
>  
>  	page_size = sysconf(_SC_PAGESIZE);
> -	vec_size = FILE_SIZE / page_size;
> -	if (FILE_SIZE % page_size)
> +	vec_size = filesize / page_size;
> +	if (filesize % 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, filesize);
>  	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, filesize, 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, filesize, vec);
>  	ASSERT_EQ(0, retval);
>  	for (i = 0; i < vec_size; i++) {
>  		ASSERT_EQ(0, vec[i]) {
> @@ -240,14 +277,14 @@ 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[filesize / 2] = 1;
> +	retval = mincore(addr, filesize, vec);
>  	ASSERT_EQ(0, retval);
> -	ASSERT_EQ(1, vec[FILE_SIZE / 2 / page_size]) {
> +	ASSERT_EQ(1, vec[filesize / 2 / page_size]) {
>  		TH_LOG("Page not found in memory after use");
>  	}
>  
> -	i = FILE_SIZE / 2 / page_size + 1;
> +	i = filesize / 2 / page_size + 1;
>  	while (i < vec_size && vec[i]) {
>  		ra_pages++;
>  		i++;
> @@ -271,7 +308,7 @@ TEST(check_file_mmap)
>  		}
>  	}
>  
> -	munmap(addr, FILE_SIZE);
> +	munmap(addr, filesize);
>  	close(fd);
>  	free(vec);
>  }
> 

-- 
Thanks,
Jeffle

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

[Shuah Khan]

On 3/30/21 6:45 AM, JeffleXu wrote:
> 
> 
> On 3/30/21 8:40 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 could be as large as twice the
>> io_opt, and thus the hardcoded file size no longer works.
>>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> 
> Forgot to mention that otherwise, "Read-ahead pages reached the end of
> the file" is reported in check_file_mmap().
> 

Please update the change log and send v2 including the change history.

>> ---
>>   .../selftests/mincore/mincore_selftest.c      | 57 +++++++++++++++----
>>   1 file changed, 47 insertions(+), 10 deletions(-)
>>

Please include test name in the subject line

>> diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
>> index 5a1e85ff5d32..cf0c86697403 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"
>> @@ -195,10 +200,42 @@ TEST(check_file_mmap)
>>   	int fd;
>>   	int i;
>>   	int ra_pages = 0;
>> +	long ra_size, filesize;
>> +	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 filesize large enough to contain the readahead window.
>> +	 */
>> +	ra_size *= 512;
>> +	filesize = ra_size * 2;
>>   
>>   	page_size = sysconf(_SC_PAGESIZE);
>> -	vec_size = FILE_SIZE / page_size;
>> -	if (FILE_SIZE % page_size)
>> +	vec_size = filesize / page_size;
>> +	if (filesize % 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, filesize);
>>   	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, filesize, 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, filesize, vec);
>>   	ASSERT_EQ(0, retval);
>>   	for (i = 0; i < vec_size; i++) {
>>   		ASSERT_EQ(0, vec[i]) {
>> @@ -240,14 +277,14 @@ 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[filesize / 2] = 1;
>> +	retval = mincore(addr, filesize, vec);
>>   	ASSERT_EQ(0, retval);
>> -	ASSERT_EQ(1, vec[FILE_SIZE / 2 / page_size]) {
>> +	ASSERT_EQ(1, vec[filesize / 2 / page_size]) {
>>   		TH_LOG("Page not found in memory after use");
>>   	}
>>   
>> -	i = FILE_SIZE / 2 / page_size + 1;
>> +	i = filesize / 2 / page_size + 1;
>>   	while (i < vec_size && vec[i]) {
>>   		ra_pages++;
>>   		i++;
>> @@ -271,7 +308,7 @@ TEST(check_file_mmap)
>>   		}
>>   	}
>>   
>> -	munmap(addr, FILE_SIZE);
>> +	munmap(addr, filesize);
>>   	close(fd);
>>   	free(vec);
>>   }
>>
> 

thanks,
-- Shuah