[PATCH v5 0/2] fstests: test reads/writes from hugepages-backed buffers

[PATCH v5 0/2] fstests: test reads/writes from hugepages-backed buffers

[Joanne Koong]

There was a recent bug in rc1 [1] that was due to faulty handling for
userspace buffers backed by hugepages.

This patchset adds generic tests for reads/writes from buffers backed by
hugepages.

[1] https://lore.kernel.org/linux-fsdevel/p3iss6hssbvtdutnwmuddvdadubrhfkdoosgmbewvo674f7f3y@cwnwffjqltzw/

Changelog:
v4: 
https://lore.kernel.org/linux-fsdevel/20250118004759.2772065-1-joannelkoong@gmail.com/
- Fix identationand and if logic structure for collapses (Darrick)

v3:
https://lore.kernel.org/linux-fsdevel/20250115183107.3124743-1-joannelkoong@gmail.com/
- Gate '-h' text behind MADVISE_COLLAPSE (Darrick)
- Add periodic hugepages collapsing for memory constrained environments (Darrick)
- Use tabs instead of spaces (Darrick)
- Refactor #ifdef MADVISE_COLLAPSE placements

v2:
https://lore.kernel.org/linux-fsdevel/20241227193311.1799626-1-joannelkoong@gmail.com/
- Gate -h and MADV_COLLAPSE usage to get it compatible on old systems (Zorro)
- Use calloc instead of malloc/memset (Nirjhar)
- Update exit codes in 1st patch
- Add Brian's reviewed-bys

v1:
https://lore.kernel.org/linux-fsdevel/20241218210122.3809198-1-joannelkoong@gmail.com/
- Refactor out buffer initialization (Brian)
- Update commit messages of 1st patch (Brian)
- Use << 10 instead of * 1024 (Nirjhar)
- Replace CONFIG_TRANSPARENT_HUGEPAGE check with checking
  'sys/kernel/mm/transparent_hugepage/' (Darrick)
- Integrate readbdy and writebdy options 
- Update options of generic/759 to include psize/bsize

Joanne Koong (2):
  fsx: support reads/writes from buffers backed by hugepages
  generic: add tests for read/writes from hugepages-backed buffers

 common/rc             |  13 ++++
 ltp/fsx.c             | 166 ++++++++++++++++++++++++++++++++++++++----
 tests/generic/758     |  22 ++++++
 tests/generic/758.out |   4 +
 tests/generic/759     |  26 +++++++
 tests/generic/759.out |   4 +
 6 files changed, 222 insertions(+), 13 deletions(-)
 create mode 100755 tests/generic/758
 create mode 100644 tests/generic/758.out
 create mode 100755 tests/generic/759
 create mode 100644 tests/generic/759.out

-- 
2.47.1

[PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

Add support for reads/writes from buffers backed by hugepages.
This can be enabled through the '-h' flag. This flag should only be used
on systems where THP capabilities are enabled.

This is motivated by a recent bug that was due to faulty handling of
userspace buffers backed by hugepages. This patch is a mitigation
against problems like this in the future.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 153 insertions(+), 13 deletions(-)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 41933354..3be383c6 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -190,6 +190,16 @@ int	o_direct;			/* -Z */
 int	aio = 0;
 int	uring = 0;
 int	mark_nr = 0;
+int	hugepages = 0;                  /* -h flag */
+
+/* Stores info needed to periodically collapse hugepages */
+struct hugepages_collapse_info {
+	void *orig_good_buf;
+	long good_buf_size;
+	void *orig_temp_buf;
+	long temp_buf_size;
+};
+struct hugepages_collapse_info hugepages_info;
 
 int page_size;
 int page_mask;
@@ -2471,7 +2481,7 @@ void
 usage(void)
 {
 	fprintf(stdout, "usage: %s",
-		"fsx [-dfknqxyzBEFHIJKLORWXZ0]\n\
+		"fsx [-dfhknqxyzBEFHIJKLORWXZ0]\n\
 	   [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid]\n\
 	   [-l flen] [-m start:end] [-o oplen] [-p progressinterval]\n\
 	   [-r readbdy] [-s style] [-t truncbdy] [-w writebdy]\n\
@@ -2483,8 +2493,11 @@ usage(void)
 	-d: debug output for all operations\n\
 	-e: pollute post-eof on size changes (default 0)\n\
 	-f: flush and invalidate cache after I/O\n\
-	-g X: write character X instead of random generated data\n\
-	-i logdev: do integrity testing, logdev is the dm log writes device\n\
+	-g X: write character X instead of random generated data\n"
+#ifdef MADV_COLLAPSE
+"	-h hugepages: use buffers backed by hugepages for reads/writes\n"
+#endif
+"	-i logdev: do integrity testing, logdev is the dm log writes device\n\
 	-j logid: prefix debug log messsages with this id\n\
 	-k: do not truncate existing file and use its size as upper bound on file size\n\
 	-l flen: the upper bound on file size (default 262144)\n\
@@ -2833,11 +2846,41 @@ __test_fallocate(int mode, const char *mode_str)
 #endif
 }
 
+/*
+ * Reclaim may break up hugepages, so do a best-effort collapse every once in
+ * a while.
+ */
+static void
+collapse_hugepages(void)
+{
+#ifdef MADV_COLLAPSE
+	int ret;
+
+	/* re-collapse every 16k fsxops after we start */
+	if (!numops || (numops & ((1U << 14) - 1)))
+		return;
+
+	ret = madvise(hugepages_info.orig_good_buf,
+		      hugepages_info.good_buf_size, MADV_COLLAPSE);
+	if (ret)
+		prt("collapsing hugepages for good_buf failed (numops=%llu): %s\n",
+		     numops, strerror(errno));
+	ret = madvise(hugepages_info.orig_temp_buf,
+		      hugepages_info.temp_buf_size, MADV_COLLAPSE);
+	if (ret)
+		prt("collapsing hugepages for temp_buf failed (numops=%llu): %s\n",
+		     numops, strerror(errno));
+#endif
+}
+
 bool
 keep_running(void)
 {
 	int ret;
 
+	if (hugepages)
+	        collapse_hugepages();
+
 	if (deadline.tv_nsec) {
 		struct timespec now;
 
@@ -2856,6 +2899,103 @@ keep_running(void)
 	return numops-- != 0;
 }
 
+static long
+get_hugepage_size(void)
+{
+	const char str[] = "Hugepagesize:";
+	size_t str_len =  sizeof(str) - 1;
+	unsigned int hugepage_size = 0;
+	char buffer[64];
+	FILE *file;
+
+	file = fopen("/proc/meminfo", "r");
+	if (!file) {
+		prterr("get_hugepage_size: fopen /proc/meminfo");
+		return -1;
+	}
+	while (fgets(buffer, sizeof(buffer), file)) {
+		if (strncmp(buffer, str, str_len) == 0) {
+			sscanf(buffer + str_len, "%u", &hugepage_size);
+			break;
+		}
+	}
+	fclose(file);
+	if (!hugepage_size) {
+		prterr("get_hugepage_size: failed to find "
+			"hugepage size in /proc/meminfo\n");
+		return -1;
+	}
+
+	/* convert from KiB to bytes */
+	return hugepage_size << 10;
+}
+
+static void *
+init_hugepages_buf(unsigned len, int hugepage_size, int alignment, long *buf_size)
+{
+	void *buf = NULL;
+#ifdef MADV_COLLAPSE
+	int ret;
+	long size = roundup(len, hugepage_size) + alignment;
+
+	ret = posix_memalign(&buf, hugepage_size, size);
+	if (ret) {
+		prterr("posix_memalign for buf");
+		return NULL;
+	}
+	memset(buf, '\0', size);
+	ret = madvise(buf, size, MADV_COLLAPSE);
+	if (ret) {
+		prterr("madvise collapse for buf");
+		free(buf);
+		return NULL;
+	}
+
+	*buf_size = size;
+#endif
+	return buf;
+}
+
+static void
+init_buffers(void)
+{
+	int i;
+
+	original_buf = (char *) malloc(maxfilelen);
+	for (i = 0; i < maxfilelen; i++)
+		original_buf[i] = random() % 256;
+	if (hugepages) {
+		long hugepage_size = get_hugepage_size();
+		if (hugepage_size == -1) {
+			prterr("get_hugepage_size()");
+			exit(102);
+		}
+		good_buf = init_hugepages_buf(maxfilelen, hugepage_size, writebdy,
+					      &hugepages_info.good_buf_size);
+		if (!good_buf) {
+			prterr("init_hugepages_buf failed for good_buf");
+			exit(103);
+		}
+		hugepages_info.orig_good_buf = good_buf;
+
+		temp_buf = init_hugepages_buf(maxoplen, hugepage_size, readbdy,
+					      &hugepages_info.temp_buf_size);
+		if (!temp_buf) {
+			prterr("init_hugepages_buf failed for temp_buf");
+			exit(103);
+		}
+		hugepages_info.orig_temp_buf = temp_buf;
+	} else {
+		unsigned long good_buf_len = maxfilelen + writebdy;
+		unsigned long temp_buf_len = maxoplen + readbdy;
+
+		good_buf = calloc(1, good_buf_len);
+		temp_buf = calloc(1, temp_buf_len);
+	}
+	good_buf = round_ptr_up(good_buf, writebdy, 0);
+	temp_buf = round_ptr_up(temp_buf, readbdy, 0);
+}
+
 static struct option longopts[] = {
 	{"replay-ops", required_argument, 0, 256},
 	{"record-ops", optional_argument, 0, 255},
@@ -2883,7 +3023,7 @@ main(int argc, char **argv)
 	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
 
 	while ((ch = getopt_long(argc, argv,
-				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
+				 "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
 				 longopts, NULL)) != EOF)
 		switch (ch) {
 		case 'b':
@@ -2916,6 +3056,14 @@ main(int argc, char **argv)
 		case 'g':
 			filldata = *optarg;
 			break;
+		case 'h':
+#ifndef MADV_COLLAPSE
+			fprintf(stderr, "MADV_COLLAPSE not supported. "
+				"Can't support -h\n");
+			exit(86);
+#endif
+			hugepages = 1;
+			break;
 		case 'i':
 			integrity = 1;
 			logdev = strdup(optarg);
@@ -3229,15 +3377,7 @@ main(int argc, char **argv)
 			exit(95);
 		}
 	}
-	original_buf = (char *) malloc(maxfilelen);
-	for (i = 0; i < maxfilelen; i++)
-		original_buf[i] = random() % 256;
-	good_buf = (char *) malloc(maxfilelen + writebdy);
-	good_buf = round_ptr_up(good_buf, writebdy, 0);
-	memset(good_buf, '\0', maxfilelen);
-	temp_buf = (char *) malloc(maxoplen + readbdy);
-	temp_buf = round_ptr_up(temp_buf, readbdy, 0);
-	memset(temp_buf, '\0', maxoplen);
+	init_buffers();
 	if (lite) {	/* zero entire existing file */
 		ssize_t written;
 
-- 
2.47.1

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Zorro Lang]

On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> Add support for reads/writes from buffers backed by hugepages.
> This can be enabled through the '-h' flag. This flag should only be used
> on systems where THP capabilities are enabled.
> 
> This is motivated by a recent bug that was due to faulty handling of
> userspace buffers backed by hugepages. This patch is a mitigation
> against problems like this in the future.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> ---

Those two test cases fail on old system which doesn't support
MADV_COLLAPSE:

   fsx -N 10000 -l 500000 -h
  -fsx -N 10000 -o 8192 -l 500000 -h
  -fsx -N 10000 -o 128000 -l 500000 -h
  +MADV_COLLAPSE not supported. Can't support -h

and

   fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
  -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
  -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
  +mapped writes DISABLED
  +MADV_COLLAPSE not supported. Can't support -h

I'm wondering ...

>  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 153 insertions(+), 13 deletions(-)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 41933354..3be383c6 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -190,6 +190,16 @@ int	o_direct;			/* -Z */
>  int	aio = 0;
>  int	uring = 0;
>  int	mark_nr = 0;
> +int	hugepages = 0;                  /* -h flag */
> +
> +/* Stores info needed to periodically collapse hugepages */
> +struct hugepages_collapse_info {
> +	void *orig_good_buf;
> +	long good_buf_size;
> +	void *orig_temp_buf;
> +	long temp_buf_size;
> +};
> +struct hugepages_collapse_info hugepages_info;
>  
>  int page_size;
>  int page_mask;
> @@ -2471,7 +2481,7 @@ void
>  usage(void)
>  {
>  	fprintf(stdout, "usage: %s",
> -		"fsx [-dfknqxyzBEFHIJKLORWXZ0]\n\
> +		"fsx [-dfhknqxyzBEFHIJKLORWXZ0]\n\
>  	   [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid]\n\
>  	   [-l flen] [-m start:end] [-o oplen] [-p progressinterval]\n\
>  	   [-r readbdy] [-s style] [-t truncbdy] [-w writebdy]\n\
> @@ -2483,8 +2493,11 @@ usage(void)
>  	-d: debug output for all operations\n\
>  	-e: pollute post-eof on size changes (default 0)\n\
>  	-f: flush and invalidate cache after I/O\n\
> -	-g X: write character X instead of random generated data\n\
> -	-i logdev: do integrity testing, logdev is the dm log writes device\n\
> +	-g X: write character X instead of random generated data\n"
> +#ifdef MADV_COLLAPSE
> +"	-h hugepages: use buffers backed by hugepages for reads/writes\n"
> +#endif
> +"	-i logdev: do integrity testing, logdev is the dm log writes device\n\
>  	-j logid: prefix debug log messsages with this id\n\
>  	-k: do not truncate existing file and use its size as upper bound on file size\n\
>  	-l flen: the upper bound on file size (default 262144)\n\
> @@ -2833,11 +2846,41 @@ __test_fallocate(int mode, const char *mode_str)
>  #endif
>  }
>  
> +/*
> + * Reclaim may break up hugepages, so do a best-effort collapse every once in
> + * a while.
> + */
> +static void
> +collapse_hugepages(void)
> +{
> +#ifdef MADV_COLLAPSE
> +	int ret;
> +
> +	/* re-collapse every 16k fsxops after we start */
> +	if (!numops || (numops & ((1U << 14) - 1)))
> +		return;
> +
> +	ret = madvise(hugepages_info.orig_good_buf,
> +		      hugepages_info.good_buf_size, MADV_COLLAPSE);
> +	if (ret)
> +		prt("collapsing hugepages for good_buf failed (numops=%llu): %s\n",
> +		     numops, strerror(errno));
> +	ret = madvise(hugepages_info.orig_temp_buf,
> +		      hugepages_info.temp_buf_size, MADV_COLLAPSE);
> +	if (ret)
> +		prt("collapsing hugepages for temp_buf failed (numops=%llu): %s\n",
> +		     numops, strerror(errno));
> +#endif
> +}
> +
>  bool
>  keep_running(void)
>  {
>  	int ret;
>  
> +	if (hugepages)
> +	        collapse_hugepages();
> +
>  	if (deadline.tv_nsec) {
>  		struct timespec now;
>  
> @@ -2856,6 +2899,103 @@ keep_running(void)
>  	return numops-- != 0;
>  }
>  
> +static long
> +get_hugepage_size(void)
> +{
> +	const char str[] = "Hugepagesize:";
> +	size_t str_len =  sizeof(str) - 1;
> +	unsigned int hugepage_size = 0;
> +	char buffer[64];
> +	FILE *file;
> +
> +	file = fopen("/proc/meminfo", "r");
> +	if (!file) {
> +		prterr("get_hugepage_size: fopen /proc/meminfo");
> +		return -1;
> +	}
> +	while (fgets(buffer, sizeof(buffer), file)) {
> +		if (strncmp(buffer, str, str_len) == 0) {
> +			sscanf(buffer + str_len, "%u", &hugepage_size);
> +			break;
> +		}
> +	}
> +	fclose(file);
> +	if (!hugepage_size) {
> +		prterr("get_hugepage_size: failed to find "
> +			"hugepage size in /proc/meminfo\n");
> +		return -1;
> +	}
> +
> +	/* convert from KiB to bytes */
> +	return hugepage_size << 10;
> +}
> +
> +static void *
> +init_hugepages_buf(unsigned len, int hugepage_size, int alignment, long *buf_size)
> +{
> +	void *buf = NULL;
> +#ifdef MADV_COLLAPSE
> +	int ret;
> +	long size = roundup(len, hugepage_size) + alignment;
> +
> +	ret = posix_memalign(&buf, hugepage_size, size);
> +	if (ret) {
> +		prterr("posix_memalign for buf");
> +		return NULL;
> +	}
> +	memset(buf, '\0', size);
> +	ret = madvise(buf, size, MADV_COLLAPSE);
> +	if (ret) {
> +		prterr("madvise collapse for buf");
> +		free(buf);
> +		return NULL;
> +	}
> +
> +	*buf_size = size;
> +#endif
> +	return buf;
> +}
> +
> +static void
> +init_buffers(void)
> +{
> +	int i;
> +
> +	original_buf = (char *) malloc(maxfilelen);
> +	for (i = 0; i < maxfilelen; i++)
> +		original_buf[i] = random() % 256;
> +	if (hugepages) {
> +		long hugepage_size = get_hugepage_size();
> +		if (hugepage_size == -1) {
> +			prterr("get_hugepage_size()");
> +			exit(102);
> +		}
> +		good_buf = init_hugepages_buf(maxfilelen, hugepage_size, writebdy,
> +					      &hugepages_info.good_buf_size);
> +		if (!good_buf) {
> +			prterr("init_hugepages_buf failed for good_buf");
> +			exit(103);
> +		}
> +		hugepages_info.orig_good_buf = good_buf;
> +
> +		temp_buf = init_hugepages_buf(maxoplen, hugepage_size, readbdy,
> +					      &hugepages_info.temp_buf_size);
> +		if (!temp_buf) {
> +			prterr("init_hugepages_buf failed for temp_buf");
> +			exit(103);
> +		}
> +		hugepages_info.orig_temp_buf = temp_buf;
> +	} else {
> +		unsigned long good_buf_len = maxfilelen + writebdy;
> +		unsigned long temp_buf_len = maxoplen + readbdy;
> +
> +		good_buf = calloc(1, good_buf_len);
> +		temp_buf = calloc(1, temp_buf_len);
> +	}
> +	good_buf = round_ptr_up(good_buf, writebdy, 0);
> +	temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> +}
> +
>  static struct option longopts[] = {
>  	{"replay-ops", required_argument, 0, 256},
>  	{"record-ops", optional_argument, 0, 255},
> @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
>  	setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
>  
>  	while ((ch = getopt_long(argc, argv,
> -				 "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> +				 "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
>  				 longopts, NULL)) != EOF)
>  		switch (ch) {
>  		case 'b':
> @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
>  		case 'g':
>  			filldata = *optarg;
>  			break;
> +		case 'h':
> +#ifndef MADV_COLLAPSE
> +			fprintf(stderr, "MADV_COLLAPSE not supported. "
> +				"Can't support -h\n");
> +			exit(86);
> +#endif
> +			hugepages = 1;
> +			break;

...
if we could change this part to:

#ifdef MADV_COLLAPSE
	hugepages = 1;
#endif
	break;

to avoid the test failures on old systems.

Or any better ideas from you :)

Thanks,
Zorro

>  		case 'i':
>  			integrity = 1;
>  			logdev = strdup(optarg);
> @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
>  			exit(95);
>  		}
>  	}
> -	original_buf = (char *) malloc(maxfilelen);
> -	for (i = 0; i < maxfilelen; i++)
> -		original_buf[i] = random() % 256;
> -	good_buf = (char *) malloc(maxfilelen + writebdy);
> -	good_buf = round_ptr_up(good_buf, writebdy, 0);
> -	memset(good_buf, '\0', maxfilelen);
> -	temp_buf = (char *) malloc(maxoplen + readbdy);
> -	temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> -	memset(temp_buf, '\0', maxoplen);
> +	init_buffers();
>  	if (lite) {	/* zero entire existing file */
>  		ssize_t written;
>  
> -- 
> 2.47.1
> 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > Add support for reads/writes from buffers backed by hugepages.
> > This can be enabled through the '-h' flag. This flag should only be used
> > on systems where THP capabilities are enabled.
> >
> > This is motivated by a recent bug that was due to faulty handling of
> > userspace buffers backed by hugepages. This patch is a mitigation
> > against problems like this in the future.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > ---
>
> Those two test cases fail on old system which doesn't support
> MADV_COLLAPSE:
>
>    fsx -N 10000 -l 500000 -h
>   -fsx -N 10000 -o 8192 -l 500000 -h
>   -fsx -N 10000 -o 128000 -l 500000 -h
>   +MADV_COLLAPSE not supported. Can't support -h
>
> and
>
>    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
>   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
>   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
>   +mapped writes DISABLED
>   +MADV_COLLAPSE not supported. Can't support -h
>
> I'm wondering ...
>
> >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 153 insertions(+), 13 deletions(-)
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 41933354..3be383c6 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -190,6 +190,16 @@ int      o_direct;                       /* -Z */
> >  int  aio = 0;
> >  int  uring = 0;
> >  int  mark_nr = 0;
> > +int  hugepages = 0;                  /* -h flag */
> > +
> > +/* Stores info needed to periodically collapse hugepages */
> > +struct hugepages_collapse_info {
> > +     void *orig_good_buf;
> > +     long good_buf_size;
> > +     void *orig_temp_buf;
> > +     long temp_buf_size;
> > +};
> > +struct hugepages_collapse_info hugepages_info;
> >
> >  int page_size;
> >  int page_mask;
> > @@ -2471,7 +2481,7 @@ void
> >  usage(void)
> >  {
> >       fprintf(stdout, "usage: %s",
> > -             "fsx [-dfknqxyzBEFHIJKLORWXZ0]\n\
> > +             "fsx [-dfhknqxyzBEFHIJKLORWXZ0]\n\
> >          [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid]\n\
> >          [-l flen] [-m start:end] [-o oplen] [-p progressinterval]\n\
> >          [-r readbdy] [-s style] [-t truncbdy] [-w writebdy]\n\
> > @@ -2483,8 +2493,11 @@ usage(void)
> >       -d: debug output for all operations\n\
> >       -e: pollute post-eof on size changes (default 0)\n\
> >       -f: flush and invalidate cache after I/O\n\
> > -     -g X: write character X instead of random generated data\n\
> > -     -i logdev: do integrity testing, logdev is the dm log writes device\n\
> > +     -g X: write character X instead of random generated data\n"
> > +#ifdef MADV_COLLAPSE
> > +"    -h hugepages: use buffers backed by hugepages for reads/writes\n"
> > +#endif
> > +"    -i logdev: do integrity testing, logdev is the dm log writes device\n\
> >       -j logid: prefix debug log messsages with this id\n\
> >       -k: do not truncate existing file and use its size as upper bound on file size\n\
> >       -l flen: the upper bound on file size (default 262144)\n\
> > @@ -2833,11 +2846,41 @@ __test_fallocate(int mode, const char *mode_str)
> >  #endif
> >  }
> >
> > +/*
> > + * Reclaim may break up hugepages, so do a best-effort collapse every once in
> > + * a while.
> > + */
> > +static void
> > +collapse_hugepages(void)
> > +{
> > +#ifdef MADV_COLLAPSE
> > +     int ret;
> > +
> > +     /* re-collapse every 16k fsxops after we start */
> > +     if (!numops || (numops & ((1U << 14) - 1)))
> > +             return;
> > +
> > +     ret = madvise(hugepages_info.orig_good_buf,
> > +                   hugepages_info.good_buf_size, MADV_COLLAPSE);
> > +     if (ret)
> > +             prt("collapsing hugepages for good_buf failed (numops=%llu): %s\n",
> > +                  numops, strerror(errno));
> > +     ret = madvise(hugepages_info.orig_temp_buf,
> > +                   hugepages_info.temp_buf_size, MADV_COLLAPSE);
> > +     if (ret)
> > +             prt("collapsing hugepages for temp_buf failed (numops=%llu): %s\n",
> > +                  numops, strerror(errno));
> > +#endif
> > +}
> > +
> >  bool
> >  keep_running(void)
> >  {
> >       int ret;
> >
> > +     if (hugepages)
> > +             collapse_hugepages();
> > +
> >       if (deadline.tv_nsec) {
> >               struct timespec now;
> >
> > @@ -2856,6 +2899,103 @@ keep_running(void)
> >       return numops-- != 0;
> >  }
> >
> > +static long
> > +get_hugepage_size(void)
> > +{
> > +     const char str[] = "Hugepagesize:";
> > +     size_t str_len =  sizeof(str) - 1;
> > +     unsigned int hugepage_size = 0;
> > +     char buffer[64];
> > +     FILE *file;
> > +
> > +     file = fopen("/proc/meminfo", "r");
> > +     if (!file) {
> > +             prterr("get_hugepage_size: fopen /proc/meminfo");
> > +             return -1;
> > +     }
> > +     while (fgets(buffer, sizeof(buffer), file)) {
> > +             if (strncmp(buffer, str, str_len) == 0) {
> > +                     sscanf(buffer + str_len, "%u", &hugepage_size);
> > +                     break;
> > +             }
> > +     }
> > +     fclose(file);
> > +     if (!hugepage_size) {
> > +             prterr("get_hugepage_size: failed to find "
> > +                     "hugepage size in /proc/meminfo\n");
> > +             return -1;
> > +     }
> > +
> > +     /* convert from KiB to bytes */
> > +     return hugepage_size << 10;
> > +}
> > +
> > +static void *
> > +init_hugepages_buf(unsigned len, int hugepage_size, int alignment, long *buf_size)
> > +{
> > +     void *buf = NULL;
> > +#ifdef MADV_COLLAPSE
> > +     int ret;
> > +     long size = roundup(len, hugepage_size) + alignment;
> > +
> > +     ret = posix_memalign(&buf, hugepage_size, size);
> > +     if (ret) {
> > +             prterr("posix_memalign for buf");
> > +             return NULL;
> > +     }
> > +     memset(buf, '\0', size);
> > +     ret = madvise(buf, size, MADV_COLLAPSE);
> > +     if (ret) {
> > +             prterr("madvise collapse for buf");
> > +             free(buf);
> > +             return NULL;
> > +     }
> > +
> > +     *buf_size = size;
> > +#endif
> > +     return buf;
> > +}
> > +
> > +static void
> > +init_buffers(void)
> > +{
> > +     int i;
> > +
> > +     original_buf = (char *) malloc(maxfilelen);
> > +     for (i = 0; i < maxfilelen; i++)
> > +             original_buf[i] = random() % 256;
> > +     if (hugepages) {
> > +             long hugepage_size = get_hugepage_size();
> > +             if (hugepage_size == -1) {
> > +                     prterr("get_hugepage_size()");
> > +                     exit(102);
> > +             }
> > +             good_buf = init_hugepages_buf(maxfilelen, hugepage_size, writebdy,
> > +                                           &hugepages_info.good_buf_size);
> > +             if (!good_buf) {
> > +                     prterr("init_hugepages_buf failed for good_buf");
> > +                     exit(103);
> > +             }
> > +             hugepages_info.orig_good_buf = good_buf;
> > +
> > +             temp_buf = init_hugepages_buf(maxoplen, hugepage_size, readbdy,
> > +                                           &hugepages_info.temp_buf_size);
> > +             if (!temp_buf) {
> > +                     prterr("init_hugepages_buf failed for temp_buf");
> > +                     exit(103);
> > +             }
> > +             hugepages_info.orig_temp_buf = temp_buf;
> > +     } else {
> > +             unsigned long good_buf_len = maxfilelen + writebdy;
> > +             unsigned long temp_buf_len = maxoplen + readbdy;
> > +
> > +             good_buf = calloc(1, good_buf_len);
> > +             temp_buf = calloc(1, temp_buf_len);
> > +     }
> > +     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > +     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > +}
> > +
> >  static struct option longopts[] = {
> >       {"replay-ops", required_argument, 0, 256},
> >       {"record-ops", optional_argument, 0, 255},
> > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> >
> >       while ((ch = getopt_long(argc, argv,
> > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> >                                longopts, NULL)) != EOF)
> >               switch (ch) {
> >               case 'b':
> > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> >               case 'g':
> >                       filldata = *optarg;
> >                       break;
> > +             case 'h':
> > +#ifndef MADV_COLLAPSE
> > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > +                             "Can't support -h\n");
> > +                     exit(86);
> > +#endif
> > +                     hugepages = 1;
> > +                     break;
>
> ...
> if we could change this part to:
>
> #ifdef MADV_COLLAPSE
>         hugepages = 1;
> #endif
>         break;
>
> to avoid the test failures on old systems.
>
> Or any better ideas from you :)

Hi Zorro,

It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
do you think about skipping generic/758 and generic/759 if the kernel
version is older than 6.1? That to me seems more preferable than the
paste above, as the paste above would execute the test as if it did
test hugepages when in reality it didn't, which would be misleading.


Thanks,
Joanne

>
> Thanks,
> Zorro
>
> >               case 'i':
> >                       integrity = 1;
> >                       logdev = strdup(optarg);
> > @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
> >                       exit(95);
> >               }
> >       }
> > -     original_buf = (char *) malloc(maxfilelen);
> > -     for (i = 0; i < maxfilelen; i++)
> > -             original_buf[i] = random() % 256;
> > -     good_buf = (char *) malloc(maxfilelen + writebdy);
> > -     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > -     memset(good_buf, '\0', maxfilelen);
> > -     temp_buf = (char *) malloc(maxoplen + readbdy);
> > -     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > -     memset(temp_buf, '\0', maxoplen);
> > +     init_buffers();
> >       if (lite) {     /* zero entire existing file */
> >               ssize_t written;
> >
> > --
> > 2.47.1
> >
>

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > Add support for reads/writes from buffers backed by hugepages.
> > > This can be enabled through the '-h' flag. This flag should only be used
> > > on systems where THP capabilities are enabled.
> > >
> > > This is motivated by a recent bug that was due to faulty handling of
> > > userspace buffers backed by hugepages. This patch is a mitigation
> > > against problems like this in the future.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> >
> > Those two test cases fail on old system which doesn't support
> > MADV_COLLAPSE:
> >
> >    fsx -N 10000 -l 500000 -h
> >   -fsx -N 10000 -o 8192 -l 500000 -h
> >   -fsx -N 10000 -o 128000 -l 500000 -h
> >   +MADV_COLLAPSE not supported. Can't support -h
> >
> > and
> >
> >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   +mapped writes DISABLED
> >   +MADV_COLLAPSE not supported. Can't support -h
> >
> > I'm wondering ...
> >
> > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > index 41933354..3be383c6 100644
> > > --- a/ltp/fsx.c
> > > +++ b/ltp/fsx.c
> > > @@ -190,6 +190,16 @@ int      o_direct;                       /* -Z */
> > >  int  aio = 0;
> > >  int  uring = 0;
> > >  int  mark_nr = 0;
> > > +int  hugepages = 0;                  /* -h flag */
> > > +
> > > +/* Stores info needed to periodically collapse hugepages */
> > > +struct hugepages_collapse_info {
> > > +     void *orig_good_buf;
> > > +     long good_buf_size;
> > > +     void *orig_temp_buf;
> > > +     long temp_buf_size;
> > > +};
> > > +struct hugepages_collapse_info hugepages_info;
> > >
> > >  int page_size;
> > >  int page_mask;
> > > @@ -2471,7 +2481,7 @@ void
> > >  usage(void)
> > >  {
> > >       fprintf(stdout, "usage: %s",
> > > -             "fsx [-dfknqxyzBEFHIJKLORWXZ0]\n\
> > > +             "fsx [-dfhknqxyzBEFHIJKLORWXZ0]\n\
> > >          [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid]\n\
> > >          [-l flen] [-m start:end] [-o oplen] [-p progressinterval]\n\
> > >          [-r readbdy] [-s style] [-t truncbdy] [-w writebdy]\n\
> > > @@ -2483,8 +2493,11 @@ usage(void)
> > >       -d: debug output for all operations\n\
> > >       -e: pollute post-eof on size changes (default 0)\n\
> > >       -f: flush and invalidate cache after I/O\n\
> > > -     -g X: write character X instead of random generated data\n\
> > > -     -i logdev: do integrity testing, logdev is the dm log writes device\n\
> > > +     -g X: write character X instead of random generated data\n"
> > > +#ifdef MADV_COLLAPSE
> > > +"    -h hugepages: use buffers backed by hugepages for reads/writes\n"
> > > +#endif
> > > +"    -i logdev: do integrity testing, logdev is the dm log writes device\n\
> > >       -j logid: prefix debug log messsages with this id\n\
> > >       -k: do not truncate existing file and use its size as upper bound on file size\n\
> > >       -l flen: the upper bound on file size (default 262144)\n\
> > > @@ -2833,11 +2846,41 @@ __test_fallocate(int mode, const char *mode_str)
> > >  #endif
> > >  }
> > >
> > > +/*
> > > + * Reclaim may break up hugepages, so do a best-effort collapse every once in
> > > + * a while.
> > > + */
> > > +static void
> > > +collapse_hugepages(void)
> > > +{
> > > +#ifdef MADV_COLLAPSE
> > > +     int ret;
> > > +
> > > +     /* re-collapse every 16k fsxops after we start */
> > > +     if (!numops || (numops & ((1U << 14) - 1)))
> > > +             return;
> > > +
> > > +     ret = madvise(hugepages_info.orig_good_buf,
> > > +                   hugepages_info.good_buf_size, MADV_COLLAPSE);
> > > +     if (ret)
> > > +             prt("collapsing hugepages for good_buf failed (numops=%llu): %s\n",
> > > +                  numops, strerror(errno));
> > > +     ret = madvise(hugepages_info.orig_temp_buf,
> > > +                   hugepages_info.temp_buf_size, MADV_COLLAPSE);
> > > +     if (ret)
> > > +             prt("collapsing hugepages for temp_buf failed (numops=%llu): %s\n",
> > > +                  numops, strerror(errno));
> > > +#endif
> > > +}
> > > +
> > >  bool
> > >  keep_running(void)
> > >  {
> > >       int ret;
> > >
> > > +     if (hugepages)
> > > +             collapse_hugepages();
> > > +
> > >       if (deadline.tv_nsec) {
> > >               struct timespec now;
> > >
> > > @@ -2856,6 +2899,103 @@ keep_running(void)
> > >       return numops-- != 0;
> > >  }
> > >
> > > +static long
> > > +get_hugepage_size(void)
> > > +{
> > > +     const char str[] = "Hugepagesize:";
> > > +     size_t str_len =  sizeof(str) - 1;
> > > +     unsigned int hugepage_size = 0;
> > > +     char buffer[64];
> > > +     FILE *file;
> > > +
> > > +     file = fopen("/proc/meminfo", "r");
> > > +     if (!file) {
> > > +             prterr("get_hugepage_size: fopen /proc/meminfo");
> > > +             return -1;
> > > +     }
> > > +     while (fgets(buffer, sizeof(buffer), file)) {
> > > +             if (strncmp(buffer, str, str_len) == 0) {
> > > +                     sscanf(buffer + str_len, "%u", &hugepage_size);
> > > +                     break;
> > > +             }
> > > +     }
> > > +     fclose(file);
> > > +     if (!hugepage_size) {
> > > +             prterr("get_hugepage_size: failed to find "
> > > +                     "hugepage size in /proc/meminfo\n");
> > > +             return -1;
> > > +     }
> > > +
> > > +     /* convert from KiB to bytes */
> > > +     return hugepage_size << 10;
> > > +}
> > > +
> > > +static void *
> > > +init_hugepages_buf(unsigned len, int hugepage_size, int alignment, long *buf_size)
> > > +{
> > > +     void *buf = NULL;
> > > +#ifdef MADV_COLLAPSE
> > > +     int ret;
> > > +     long size = roundup(len, hugepage_size) + alignment;
> > > +
> > > +     ret = posix_memalign(&buf, hugepage_size, size);
> > > +     if (ret) {
> > > +             prterr("posix_memalign for buf");
> > > +             return NULL;
> > > +     }
> > > +     memset(buf, '\0', size);
> > > +     ret = madvise(buf, size, MADV_COLLAPSE);
> > > +     if (ret) {
> > > +             prterr("madvise collapse for buf");
> > > +             free(buf);
> > > +             return NULL;
> > > +     }
> > > +
> > > +     *buf_size = size;
> > > +#endif
> > > +     return buf;
> > > +}
> > > +
> > > +static void
> > > +init_buffers(void)
> > > +{
> > > +     int i;
> > > +
> > > +     original_buf = (char *) malloc(maxfilelen);
> > > +     for (i = 0; i < maxfilelen; i++)
> > > +             original_buf[i] = random() % 256;
> > > +     if (hugepages) {
> > > +             long hugepage_size = get_hugepage_size();
> > > +             if (hugepage_size == -1) {
> > > +                     prterr("get_hugepage_size()");
> > > +                     exit(102);
> > > +             }
> > > +             good_buf = init_hugepages_buf(maxfilelen, hugepage_size, writebdy,
> > > +                                           &hugepages_info.good_buf_size);
> > > +             if (!good_buf) {
> > > +                     prterr("init_hugepages_buf failed for good_buf");
> > > +                     exit(103);
> > > +             }
> > > +             hugepages_info.orig_good_buf = good_buf;
> > > +
> > > +             temp_buf = init_hugepages_buf(maxoplen, hugepage_size, readbdy,
> > > +                                           &hugepages_info.temp_buf_size);
> > > +             if (!temp_buf) {
> > > +                     prterr("init_hugepages_buf failed for temp_buf");
> > > +                     exit(103);
> > > +             }
> > > +             hugepages_info.orig_temp_buf = temp_buf;
> > > +     } else {
> > > +             unsigned long good_buf_len = maxfilelen + writebdy;
> > > +             unsigned long temp_buf_len = maxoplen + readbdy;
> > > +
> > > +             good_buf = calloc(1, good_buf_len);
> > > +             temp_buf = calloc(1, temp_buf_len);
> > > +     }
> > > +     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > +     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > +}
> > > +
> > >  static struct option longopts[] = {
> > >       {"replay-ops", required_argument, 0, 256},
> > >       {"record-ops", optional_argument, 0, 255},
> > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > >
> > >       while ((ch = getopt_long(argc, argv,
> > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > >                                longopts, NULL)) != EOF)
> > >               switch (ch) {
> > >               case 'b':
> > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > >               case 'g':
> > >                       filldata = *optarg;
> > >                       break;
> > > +             case 'h':
> > > +#ifndef MADV_COLLAPSE
> > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > +                             "Can't support -h\n");
> > > +                     exit(86);
> > > +#endif
> > > +                     hugepages = 1;
> > > +                     break;
> >
> > ...
> > if we could change this part to:
> >
> > #ifdef MADV_COLLAPSE
> >         hugepages = 1;
> > #endif
> >         break;
> >
> > to avoid the test failures on old systems.
> >
> > Or any better ideas from you :)
> 
> Hi Zorro,
> 
> It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> do you think about skipping generic/758 and generic/759 if the kernel
> version is older than 6.1? That to me seems more preferable than the
> paste above, as the paste above would execute the test as if it did
> test hugepages when in reality it didn't, which would be misleading.

Now that I've gotten to try this out --

There's a couple of things going on here.  The first is that generic/759
and 760 need to check if invoking fsx -h causes it to spit out the
"MADV_COLLAPSE not supported" error and _notrun the test.

The second thing is that userspace programs can ensure the existence of
MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
which requires that the underlying C library headers are new enough to
include a definition.  glibc 2.37 is new enough, but even things like
Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
might not follow glibc's practice of wrapping and/or redefining symbols
in a way that you hope is the same as...

The second way is through linux/mman.h, which comes from the kernel
headers package; and the third way is for the program to define it
itself if nobody else does.

So I think the easiest way to fix the fsx.c build is to include
linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
details when I reviewed your patch; I'm a little attention constrained
ATM trying to get a large pile of bugfixes and redesigns reviewed so
for-next can finally move forward again.

--D

> 
> Thanks,
> Joanne
> 
> >
> > Thanks,
> > Zorro
> >
> > >               case 'i':
> > >                       integrity = 1;
> > >                       logdev = strdup(optarg);
> > > @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
> > >                       exit(95);
> > >               }
> > >       }
> > > -     original_buf = (char *) malloc(maxfilelen);
> > > -     for (i = 0; i < maxfilelen; i++)
> > > -             original_buf[i] = random() % 256;
> > > -     good_buf = (char *) malloc(maxfilelen + writebdy);
> > > -     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > -     memset(good_buf, '\0', maxfilelen);
> > > -     temp_buf = (char *) malloc(maxoplen + readbdy);
> > > -     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > -     memset(temp_buf, '\0', maxoplen);
> > > +     init_buffers();
> > >       if (lite) {     /* zero entire existing file */
> > >               ssize_t written;
> > >
> > > --
> > > 2.47.1
> > >
> >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > Add support for reads/writes from buffers backed by hugepages.
> > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > on systems where THP capabilities are enabled.
> > > >
> > > > This is motivated by a recent bug that was due to faulty handling of
> > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > against problems like this in the future.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > >
> > > Those two test cases fail on old system which doesn't support
> > > MADV_COLLAPSE:
> > >
> > >    fsx -N 10000 -l 500000 -h
> > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > >   +MADV_COLLAPSE not supported. Can't support -h
> > >
> > > and
> > >
> > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   +mapped writes DISABLED
> > >   +MADV_COLLAPSE not supported. Can't support -h
> > >
> > > I'm wondering ...
> > >
> > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > index 41933354..3be383c6 100644
> > > > --- a/ltp/fsx.c
> > > > +++ b/ltp/fsx.c
> > > >  static struct option longopts[] = {
> > > >       {"replay-ops", required_argument, 0, 256},
> > > >       {"record-ops", optional_argument, 0, 255},
> > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > >
> > > >       while ((ch = getopt_long(argc, argv,
> > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > >                                longopts, NULL)) != EOF)
> > > >               switch (ch) {
> > > >               case 'b':
> > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > >               case 'g':
> > > >                       filldata = *optarg;
> > > >                       break;
> > > > +             case 'h':
> > > > +#ifndef MADV_COLLAPSE
> > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > +                             "Can't support -h\n");
> > > > +                     exit(86);
> > > > +#endif
> > > > +                     hugepages = 1;
> > > > +                     break;
> > >
> > > ...
> > > if we could change this part to:
> > >
> > > #ifdef MADV_COLLAPSE
> > >         hugepages = 1;
> > > #endif
> > >         break;
> > >
> > > to avoid the test failures on old systems.
> > >
> > > Or any better ideas from you :)
> >
> > Hi Zorro,
> >
> > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > do you think about skipping generic/758 and generic/759 if the kernel
> > version is older than 6.1? That to me seems more preferable than the
> > paste above, as the paste above would execute the test as if it did
> > test hugepages when in reality it didn't, which would be misleading.
>
> Now that I've gotten to try this out --
>
> There's a couple of things going on here.  The first is that generic/759
> and 760 need to check if invoking fsx -h causes it to spit out the
> "MADV_COLLAPSE not supported" error and _notrun the test.
>
> The second thing is that userspace programs can ensure the existence of
> MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> which requires that the underlying C library headers are new enough to
> include a definition.  glibc 2.37 is new enough, but even things like
> Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> might not follow glibc's practice of wrapping and/or redefining symbols
> in a way that you hope is the same as...
>
> The second way is through linux/mman.h, which comes from the kernel
> headers package; and the third way is for the program to define it
> itself if nobody else does.
>
> So I think the easiest way to fix the fsx.c build is to include
> linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these

Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?

For generic/758 and 759, does it suffice to gate this on whether the
kernel version if 6.1+ and _notrun if not? My understanding is that
any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
headers package and support the feature.


Thanks,
Joanne

> details when I reviewed your patch; I'm a little attention constrained
> ATM trying to get a large pile of bugfixes and redesigns reviewed so
> for-next can finally move forward again.
>
> --D
>
> >
> > Thanks,
> > Joanne
> >
> > >
> > > Thanks,
> > > Zorro
> > >
> > > >               case 'i':
> > > >                       integrity = 1;
> > > >                       logdev = strdup(optarg);
> > > > @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
> > > >                       exit(95);
> > > >               }
> > > >       }
> > > > -     original_buf = (char *) malloc(maxfilelen);
> > > > -     for (i = 0; i < maxfilelen; i++)
> > > > -             original_buf[i] = random() % 256;
> > > > -     good_buf = (char *) malloc(maxfilelen + writebdy);
> > > > -     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > > -     memset(good_buf, '\0', maxfilelen);
> > > > -     temp_buf = (char *) malloc(maxoplen + readbdy);
> > > > -     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > > -     memset(temp_buf, '\0', maxoplen);
> > > > +     init_buffers();
> > > >       if (lite) {     /* zero entire existing file */
> > > >               ssize_t written;
> > > >
> > > > --
> > > > 2.47.1
> > > >
> > >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > >
> > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > on systems where THP capabilities are enabled.
> > > > >
> > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > against problems like this in the future.
> > > > >
> > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > >
> > > > Those two test cases fail on old system which doesn't support
> > > > MADV_COLLAPSE:
> > > >
> > > >    fsx -N 10000 -l 500000 -h
> > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > >
> > > > and
> > > >
> > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > >   +mapped writes DISABLED
> > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > >
> > > > I'm wondering ...
> > > >
> > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > index 41933354..3be383c6 100644
> > > > > --- a/ltp/fsx.c
> > > > > +++ b/ltp/fsx.c
> > > > >  static struct option longopts[] = {
> > > > >       {"replay-ops", required_argument, 0, 256},
> > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > >
> > > > >       while ((ch = getopt_long(argc, argv,
> > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > >                                longopts, NULL)) != EOF)
> > > > >               switch (ch) {
> > > > >               case 'b':
> > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > >               case 'g':
> > > > >                       filldata = *optarg;
> > > > >                       break;
> > > > > +             case 'h':
> > > > > +#ifndef MADV_COLLAPSE
> > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > +                             "Can't support -h\n");
> > > > > +                     exit(86);
> > > > > +#endif
> > > > > +                     hugepages = 1;
> > > > > +                     break;
> > > >
> > > > ...
> > > > if we could change this part to:
> > > >
> > > > #ifdef MADV_COLLAPSE
> > > >         hugepages = 1;
> > > > #endif
> > > >         break;
> > > >
> > > > to avoid the test failures on old systems.
> > > >
> > > > Or any better ideas from you :)
> > >
> > > Hi Zorro,
> > >
> > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > do you think about skipping generic/758 and generic/759 if the kernel
> > > version is older than 6.1? That to me seems more preferable than the
> > > paste above, as the paste above would execute the test as if it did
> > > test hugepages when in reality it didn't, which would be misleading.
> >
> > Now that I've gotten to try this out --
> >
> > There's a couple of things going on here.  The first is that generic/759
> > and 760 need to check if invoking fsx -h causes it to spit out the
> > "MADV_COLLAPSE not supported" error and _notrun the test.
> >
> > The second thing is that userspace programs can ensure the existence of
> > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > which requires that the underlying C library headers are new enough to
> > include a definition.  glibc 2.37 is new enough, but even things like
> > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > might not follow glibc's practice of wrapping and/or redefining symbols
> > in a way that you hope is the same as...
> >
> > The second way is through linux/mman.h, which comes from the kernel
> > headers package; and the third way is for the program to define it
> > itself if nobody else does.
> >
> > So I think the easiest way to fix the fsx.c build is to include
> > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> 
> Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?

Yes, because glibc provides the mmap() function that wraps
syscall(__NR_mmap, ...);

> For generic/758 and 759, does it suffice to gate this on whether the
> kernel version if 6.1+ and _notrun if not? My understanding is that
> any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> headers package and support the feature.

No, because some (most?) vendors backport new features into existing
kernels without revving the version number of that kernel.

Maybe the following fixes things?

--D

generic/759,760: fix MADV_COLLAPSE detection and inclusion

On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
MADV_COLLAPSE flag might not be defined in any of the header files
pulled in by sys/mman.h, which means that the fsx binary might not get
built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
the test will fail with:

>  QA output created by 760
>  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> +mapped writes DISABLED
> +MADV_COLLAPSE not supported. Can't support -h

Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
then fix fsx.c to include the mman.h from the kernel headers (aka
linux/mman.h) so that we can actually test IOs to and from THPs if the
kernel is newer than the rest of userspace.

Cc: <fstests@vger.kernel.org> # v2025.02.02
Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 ltp/fsx.c         |    1 +
 tests/generic/759 |    3 +++
 tests/generic/760 |    3 +++
 3 files changed, 7 insertions(+)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 634c496ffe9317..cf9502a74c17a7 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -20,6 +20,7 @@
 #include <strings.h>
 #include <sys/file.h>
 #include <sys/mman.h>
+#include <linux/mman.h>
 #include <sys/uio.h>
 #include <stdbool.h>
 #ifdef HAVE_ERR_H
diff --git a/tests/generic/759 b/tests/generic/759
index 6c74478aa8a0e0..3549c5ed6a9299 100755
--- a/tests/generic/759
+++ b/tests/generic/759
@@ -14,6 +14,9 @@ _begin_fstest rw auto quick
 _require_test
 _require_thp
 
+$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
+	_notrun "fsx binary does not support MADV_COLLAPSE"
+
 run_fsx -N 10000            -l 500000 -h
 run_fsx -N 10000  -o 8192   -l 500000 -h
 run_fsx -N 10000  -o 128000 -l 500000 -h
diff --git a/tests/generic/760 b/tests/generic/760
index c71a196222ad3b..2fbd28502ae678 100755
--- a/tests/generic/760
+++ b/tests/generic/760
@@ -15,6 +15,9 @@ _require_test
 _require_odirect
 _require_thp
 
+$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
+	_notrun "fsx binary does not support MADV_COLLAPSE"
+
 psize=`$here/src/feature -s`
 bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > >
> > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > on systems where THP capabilities are enabled.
> > > > > >
> > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > against problems like this in the future.
> > > > > >
> > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > ---
> > > > >
> > > > > Those two test cases fail on old system which doesn't support
> > > > > MADV_COLLAPSE:
> > > > >
> > > > >    fsx -N 10000 -l 500000 -h
> > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > >
> > > > > and
> > > > >
> > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > >   +mapped writes DISABLED
> > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > >
> > > > > I'm wondering ...
> > > > >
> > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > index 41933354..3be383c6 100644
> > > > > > --- a/ltp/fsx.c
> > > > > > +++ b/ltp/fsx.c
> > > > > >  static struct option longopts[] = {
> > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > >
> > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > >                                longopts, NULL)) != EOF)
> > > > > >               switch (ch) {
> > > > > >               case 'b':
> > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > >               case 'g':
> > > > > >                       filldata = *optarg;
> > > > > >                       break;
> > > > > > +             case 'h':
> > > > > > +#ifndef MADV_COLLAPSE
> > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > +                             "Can't support -h\n");
> > > > > > +                     exit(86);
> > > > > > +#endif
> > > > > > +                     hugepages = 1;
> > > > > > +                     break;
> > > > >
> > > > > ...
> > > > > if we could change this part to:
> > > > >
> > > > > #ifdef MADV_COLLAPSE
> > > > >         hugepages = 1;
> > > > > #endif
> > > > >         break;
> > > > >
> > > > > to avoid the test failures on old systems.
> > > > >
> > > > > Or any better ideas from you :)
> > > >
> > > > Hi Zorro,
> > > >
> > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > version is older than 6.1? That to me seems more preferable than the
> > > > paste above, as the paste above would execute the test as if it did
> > > > test hugepages when in reality it didn't, which would be misleading.
> > >
> > > Now that I've gotten to try this out --
> > >
> > > There's a couple of things going on here.  The first is that generic/759
> > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > >
> > > The second thing is that userspace programs can ensure the existence of
> > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > which requires that the underlying C library headers are new enough to
> > > include a definition.  glibc 2.37 is new enough, but even things like
> > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > in a way that you hope is the same as...
> > >
> > > The second way is through linux/mman.h, which comes from the kernel
> > > headers package; and the third way is for the program to define it
> > > itself if nobody else does.
> > >
> > > So I think the easiest way to fix the fsx.c build is to include
> > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> >
> > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
>
> Yes, because glibc provides the mmap() function that wraps
> syscall(__NR_mmap, ...);
>
> > For generic/758 and 759, does it suffice to gate this on whether the
> > kernel version if 6.1+ and _notrun if not? My understanding is that
> > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > headers package and support the feature.
>
> No, because some (most?) vendors backport new features into existing
> kernels without revving the version number of that kernel.

Oh okay, I see. That makes sense, thanks for the explanation.

>
> Maybe the following fixes things?
>
> --D
>
> generic/759,760: fix MADV_COLLAPSE detection and inclusion
>
> On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> MADV_COLLAPSE flag might not be defined in any of the header files
> pulled in by sys/mman.h, which means that the fsx binary might not get
> built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> the test will fail with:
>
> >  QA output created by 760
> >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > +mapped writes DISABLED
> > +MADV_COLLAPSE not supported. Can't support -h
>
> Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> then fix fsx.c to include the mman.h from the kernel headers (aka
> linux/mman.h) so that we can actually test IOs to and from THPs if the
> kernel is newer than the rest of userspace.
>
> Cc: <fstests@vger.kernel.org> # v2025.02.02
> Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  ltp/fsx.c         |    1 +
>  tests/generic/759 |    3 +++
>  tests/generic/760 |    3 +++
>  3 files changed, 7 insertions(+)
>
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 634c496ffe9317..cf9502a74c17a7 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -20,6 +20,7 @@
>  #include <strings.h>
>  #include <sys/file.h>
>  #include <sys/mman.h>
> +#include <linux/mman.h>
>  #include <sys/uio.h>
>  #include <stdbool.h>
>  #ifdef HAVE_ERR_H
> diff --git a/tests/generic/759 b/tests/generic/759
> index 6c74478aa8a0e0..3549c5ed6a9299 100755
> --- a/tests/generic/759
> +++ b/tests/generic/759
> @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
>  _require_test
>  _require_thp
>
> +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> +       _notrun "fsx binary does not support MADV_COLLAPSE"
> +
>  run_fsx -N 10000            -l 500000 -h
>  run_fsx -N 10000  -o 8192   -l 500000 -h
>  run_fsx -N 10000  -o 128000 -l 500000 -h
> diff --git a/tests/generic/760 b/tests/generic/760
> index c71a196222ad3b..2fbd28502ae678 100755
> --- a/tests/generic/760
> +++ b/tests/generic/760
> @@ -15,6 +15,9 @@ _require_test
>  _require_odirect
>  _require_thp
>
> +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> +       _notrun "fsx binary does not support MADV_COLLAPSE"
> +

I tried this out locally and it works for me:

generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
Ran: generic/759
Not run: generic/759
Passed all 1 tests

SECTION       -- fuse
=========================
Ran: generic/759
Not run: generic/759
Passed all 1 tests


Thanks,
Joanne

>  psize=`$here/src/feature -s`
>  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
>

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > >
> > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > on systems where THP capabilities are enabled.
> > > > > > >
> > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > against problems like this in the future.
> > > > > > >
> > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > ---
> > > > > >
> > > > > > Those two test cases fail on old system which doesn't support
> > > > > > MADV_COLLAPSE:
> > > > > >
> > > > > >    fsx -N 10000 -l 500000 -h
> > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > >
> > > > > > and
> > > > > >
> > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > >   +mapped writes DISABLED
> > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > >
> > > > > > I'm wondering ...
> > > > > >
> > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > >
> > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > index 41933354..3be383c6 100644
> > > > > > > --- a/ltp/fsx.c
> > > > > > > +++ b/ltp/fsx.c
> > > > > > >  static struct option longopts[] = {
> > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > >
> > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > >                                longopts, NULL)) != EOF)
> > > > > > >               switch (ch) {
> > > > > > >               case 'b':
> > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > >               case 'g':
> > > > > > >                       filldata = *optarg;
> > > > > > >                       break;
> > > > > > > +             case 'h':
> > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > +                             "Can't support -h\n");
> > > > > > > +                     exit(86);
> > > > > > > +#endif
> > > > > > > +                     hugepages = 1;
> > > > > > > +                     break;
> > > > > >
> > > > > > ...
> > > > > > if we could change this part to:
> > > > > >
> > > > > > #ifdef MADV_COLLAPSE
> > > > > >         hugepages = 1;
> > > > > > #endif
> > > > > >         break;
> > > > > >
> > > > > > to avoid the test failures on old systems.
> > > > > >
> > > > > > Or any better ideas from you :)
> > > > >
> > > > > Hi Zorro,
> > > > >
> > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > paste above, as the paste above would execute the test as if it did
> > > > > test hugepages when in reality it didn't, which would be misleading.
> > > >
> > > > Now that I've gotten to try this out --
> > > >
> > > > There's a couple of things going on here.  The first is that generic/759
> > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > >
> > > > The second thing is that userspace programs can ensure the existence of
> > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > which requires that the underlying C library headers are new enough to
> > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > in a way that you hope is the same as...
> > > >
> > > > The second way is through linux/mman.h, which comes from the kernel
> > > > headers package; and the third way is for the program to define it
> > > > itself if nobody else does.
> > > >
> > > > So I think the easiest way to fix the fsx.c build is to include
> > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > >
> > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> >
> > Yes, because glibc provides the mmap() function that wraps
> > syscall(__NR_mmap, ...);
> >
> > > For generic/758 and 759, does it suffice to gate this on whether the
> > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > headers package and support the feature.
> >
> > No, because some (most?) vendors backport new features into existing
> > kernels without revving the version number of that kernel.
> 
> Oh okay, I see. That makes sense, thanks for the explanation.
> 
> >
> > Maybe the following fixes things?
> >
> > --D
> >
> > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> >
> > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > MADV_COLLAPSE flag might not be defined in any of the header files
> > pulled in by sys/mman.h, which means that the fsx binary might not get
> > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > the test will fail with:
> >
> > >  QA output created by 760
> > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > +mapped writes DISABLED
> > > +MADV_COLLAPSE not supported. Can't support -h
> >
> > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > then fix fsx.c to include the mman.h from the kernel headers (aka
> > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > kernel is newer than the rest of userspace.
> >
> > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> >  ltp/fsx.c         |    1 +
> >  tests/generic/759 |    3 +++
> >  tests/generic/760 |    3 +++
> >  3 files changed, 7 insertions(+)
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 634c496ffe9317..cf9502a74c17a7 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -20,6 +20,7 @@
> >  #include <strings.h>
> >  #include <sys/file.h>
> >  #include <sys/mman.h>
> > +#include <linux/mman.h>
> >  #include <sys/uio.h>
> >  #include <stdbool.h>
> >  #ifdef HAVE_ERR_H
> > diff --git a/tests/generic/759 b/tests/generic/759
> > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > --- a/tests/generic/759
> > +++ b/tests/generic/759
> > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> >  _require_test
> >  _require_thp
> >
> > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > +
> >  run_fsx -N 10000            -l 500000 -h
> >  run_fsx -N 10000  -o 8192   -l 500000 -h
> >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > diff --git a/tests/generic/760 b/tests/generic/760
> > index c71a196222ad3b..2fbd28502ae678 100755
> > --- a/tests/generic/760
> > +++ b/tests/generic/760
> > @@ -15,6 +15,9 @@ _require_test
> >  _require_odirect
> >  _require_thp
> >
> > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > +
> 
> I tried this out locally and it works for me:
> 
> generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> Ran: generic/759
> Not run: generic/759
> Passed all 1 tests
> 
> SECTION       -- fuse
> =========================
> Ran: generic/759
> Not run: generic/759
> Passed all 1 tests

Does the test actually run if you /do/ have kernel/libc headers that
define MADV_COLLAPSE?  And if so, does that count as a Tested-by?

--D

> 
> Thanks,
> Joanne
> 
> >  psize=`$here/src/feature -s`
> >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> >
> 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > >
> > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > >
> > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > against problems like this in the future.
> > > > > > > >
> > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > ---
> > > > > > >
> > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > MADV_COLLAPSE:
> > > > > > >
> > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > >
> > > > > > > and
> > > > > > >
> > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > >   +mapped writes DISABLED
> > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > >
> > > > > > > I'm wondering ...
> > > > > > >
> > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > --- a/ltp/fsx.c
> > > > > > > > +++ b/ltp/fsx.c
> > > > > > > >  static struct option longopts[] = {
> > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > >
> > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > >               switch (ch) {
> > > > > > > >               case 'b':
> > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > >               case 'g':
> > > > > > > >                       filldata = *optarg;
> > > > > > > >                       break;
> > > > > > > > +             case 'h':
> > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > +                             "Can't support -h\n");
> > > > > > > > +                     exit(86);
> > > > > > > > +#endif
> > > > > > > > +                     hugepages = 1;
> > > > > > > > +                     break;
> > > > > > >
> > > > > > > ...
> > > > > > > if we could change this part to:
> > > > > > >
> > > > > > > #ifdef MADV_COLLAPSE
> > > > > > >         hugepages = 1;
> > > > > > > #endif
> > > > > > >         break;
> > > > > > >
> > > > > > > to avoid the test failures on old systems.
> > > > > > >
> > > > > > > Or any better ideas from you :)
> > > > > >
> > > > > > Hi Zorro,
> > > > > >
> > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > >
> > > > > Now that I've gotten to try this out --
> > > > >
> > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > >
> > > > > The second thing is that userspace programs can ensure the existence of
> > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > which requires that the underlying C library headers are new enough to
> > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > in a way that you hope is the same as...
> > > > >
> > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > headers package; and the third way is for the program to define it
> > > > > itself if nobody else does.
> > > > >
> > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > >
> > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > >
> > > Yes, because glibc provides the mmap() function that wraps
> > > syscall(__NR_mmap, ...);
> > >
> > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > headers package and support the feature.
> > >
> > > No, because some (most?) vendors backport new features into existing
> > > kernels without revving the version number of that kernel.
> >
> > Oh okay, I see. That makes sense, thanks for the explanation.
> >
> > >
> > > Maybe the following fixes things?
> > >
> > > --D
> > >
> > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > >
> > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > the test will fail with:
> > >
> > > >  QA output created by 760
> > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > +mapped writes DISABLED
> > > > +MADV_COLLAPSE not supported. Can't support -h
> > >
> > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > kernel is newer than the rest of userspace.
> > >
> > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > ---
> > >  ltp/fsx.c         |    1 +
> > >  tests/generic/759 |    3 +++
> > >  tests/generic/760 |    3 +++
> > >  3 files changed, 7 insertions(+)
> > >
> > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > --- a/ltp/fsx.c
> > > +++ b/ltp/fsx.c
> > > @@ -20,6 +20,7 @@
> > >  #include <strings.h>
> > >  #include <sys/file.h>
> > >  #include <sys/mman.h>
> > > +#include <linux/mman.h>
> > >  #include <sys/uio.h>
> > >  #include <stdbool.h>
> > >  #ifdef HAVE_ERR_H
> > > diff --git a/tests/generic/759 b/tests/generic/759
> > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > --- a/tests/generic/759
> > > +++ b/tests/generic/759
> > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > >  _require_test
> > >  _require_thp
> > >
> > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > +
> > >  run_fsx -N 10000            -l 500000 -h
> > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > diff --git a/tests/generic/760 b/tests/generic/760
> > > index c71a196222ad3b..2fbd28502ae678 100755
> > > --- a/tests/generic/760
> > > +++ b/tests/generic/760
> > > @@ -15,6 +15,9 @@ _require_test
> > >  _require_odirect
> > >  _require_thp
> > >
> > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > +
> >
> > I tried this out locally and it works for me:
> >
> > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > Ran: generic/759
> > Not run: generic/759
> > Passed all 1 tests
> >
> > SECTION       -- fuse
> > =========================
> > Ran: generic/759
> > Not run: generic/759
> > Passed all 1 tests
>
> Does the test actually run if you /do/ have kernel/libc headers that
> define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
>

I'm not sure if I fully understand the subtext of your question but
yes, the test runs on my system when MADV_COLLAPSE is defined in the
kernel/libc headers.
For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
I tried this out by modifying the ifdef/ifndef checks in fsx to
MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
grep -q 'MADV_COLLAPSE not supported'' line catches that.


> --D
>
> >
> > Thanks,
> > Joanne
> >
> > >  psize=`$here/src/feature -s`
> > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > >
> >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 01:40:39PM -0800, Joanne Koong wrote:
> On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > > >
> > > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > > against problems like this in the future.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > > ---
> > > > > > > >
> > > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > > MADV_COLLAPSE:
> > > > > > > >
> > > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > >
> > > > > > > > and
> > > > > > > >
> > > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > >   +mapped writes DISABLED
> > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > >
> > > > > > > > I'm wondering ...
> > > > > > > >
> > > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > > --- a/ltp/fsx.c
> > > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > >  static struct option longopts[] = {
> > > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > > >
> > > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > > >               switch (ch) {
> > > > > > > > >               case 'b':
> > > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > > >               case 'g':
> > > > > > > > >                       filldata = *optarg;
> > > > > > > > >                       break;
> > > > > > > > > +             case 'h':
> > > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > > +                             "Can't support -h\n");
> > > > > > > > > +                     exit(86);
> > > > > > > > > +#endif
> > > > > > > > > +                     hugepages = 1;
> > > > > > > > > +                     break;
> > > > > > > >
> > > > > > > > ...
> > > > > > > > if we could change this part to:
> > > > > > > >
> > > > > > > > #ifdef MADV_COLLAPSE
> > > > > > > >         hugepages = 1;
> > > > > > > > #endif
> > > > > > > >         break;
> > > > > > > >
> > > > > > > > to avoid the test failures on old systems.
> > > > > > > >
> > > > > > > > Or any better ideas from you :)
> > > > > > >
> > > > > > > Hi Zorro,
> > > > > > >
> > > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > > >
> > > > > > Now that I've gotten to try this out --
> > > > > >
> > > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > > >
> > > > > > The second thing is that userspace programs can ensure the existence of
> > > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > > which requires that the underlying C library headers are new enough to
> > > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > > in a way that you hope is the same as...
> > > > > >
> > > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > > headers package; and the third way is for the program to define it
> > > > > > itself if nobody else does.
> > > > > >
> > > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > > >
> > > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > > >
> > > > Yes, because glibc provides the mmap() function that wraps
> > > > syscall(__NR_mmap, ...);
> > > >
> > > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > > headers package and support the feature.
> > > >
> > > > No, because some (most?) vendors backport new features into existing
> > > > kernels without revving the version number of that kernel.
> > >
> > > Oh okay, I see. That makes sense, thanks for the explanation.
> > >
> > > >
> > > > Maybe the following fixes things?
> > > >
> > > > --D
> > > >
> > > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > > >
> > > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > > the test will fail with:
> > > >
> > > > >  QA output created by 760
> > > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > +mapped writes DISABLED
> > > > > +MADV_COLLAPSE not supported. Can't support -h
> > > >
> > > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > > kernel is newer than the rest of userspace.
> > > >
> > > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > ---
> > > >  ltp/fsx.c         |    1 +
> > > >  tests/generic/759 |    3 +++
> > > >  tests/generic/760 |    3 +++
> > > >  3 files changed, 7 insertions(+)
> > > >
> > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > > --- a/ltp/fsx.c
> > > > +++ b/ltp/fsx.c
> > > > @@ -20,6 +20,7 @@
> > > >  #include <strings.h>
> > > >  #include <sys/file.h>
> > > >  #include <sys/mman.h>
> > > > +#include <linux/mman.h>
> > > >  #include <sys/uio.h>
> > > >  #include <stdbool.h>
> > > >  #ifdef HAVE_ERR_H
> > > > diff --git a/tests/generic/759 b/tests/generic/759
> > > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > > --- a/tests/generic/759
> > > > +++ b/tests/generic/759
> > > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > > >  _require_test
> > > >  _require_thp
> > > >
> > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > +
> > > >  run_fsx -N 10000            -l 500000 -h
> > > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > > diff --git a/tests/generic/760 b/tests/generic/760
> > > > index c71a196222ad3b..2fbd28502ae678 100755
> > > > --- a/tests/generic/760
> > > > +++ b/tests/generic/760
> > > > @@ -15,6 +15,9 @@ _require_test
> > > >  _require_odirect
> > > >  _require_thp
> > > >
> > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > +
> > >
> > > I tried this out locally and it works for me:
> > >
> > > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > > Ran: generic/759
> > > Not run: generic/759
> > > Passed all 1 tests
> > >
> > > SECTION       -- fuse
> > > =========================
> > > Ran: generic/759
> > > Not run: generic/759
> > > Passed all 1 tests
> >
> > Does the test actually run if you /do/ have kernel/libc headers that
> > define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
> >
> 
> I'm not sure if I fully understand the subtext of your question but
> yes, the test runs on my system when MADV_COLLAPSE is defined in the
> kernel/libc headers.

Yep, you understood me correctly. :)

> For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
> I tried this out by modifying the ifdef/ifndef checks in fsx to
> MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
> grep -q 'MADV_COLLAPSE not supported'' line catches that.

<nod> Sounds good then; I'll add this to my test clod and run it
overnight.

--D

> 
> > --D
> >
> > >
> > > Thanks,
> > > Joanne
> > >
> > > >  psize=`$here/src/feature -s`
> > > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > > >
> > >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Mon, Feb 03, 2025 at 01:54:32PM -0800, Darrick J. Wong wrote:
> On Mon, Feb 03, 2025 at 01:40:39PM -0800, Joanne Koong wrote:
> > On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > > > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > >
> > > > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > > > >
> > > > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > > > against problems like this in the future.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > > > MADV_COLLAPSE:
> > > > > > > > >
> > > > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > >
> > > > > > > > > and
> > > > > > > > >
> > > > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > >   +mapped writes DISABLED
> > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > >
> > > > > > > > > I'm wondering ...
> > > > > > > > >
> > > > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > > > --- a/ltp/fsx.c
> > > > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > > >  static struct option longopts[] = {
> > > > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > > > >
> > > > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > > > >               switch (ch) {
> > > > > > > > > >               case 'b':
> > > > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > > > >               case 'g':
> > > > > > > > > >                       filldata = *optarg;
> > > > > > > > > >                       break;
> > > > > > > > > > +             case 'h':
> > > > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > > > +                             "Can't support -h\n");
> > > > > > > > > > +                     exit(86);
> > > > > > > > > > +#endif
> > > > > > > > > > +                     hugepages = 1;
> > > > > > > > > > +                     break;
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > > if we could change this part to:
> > > > > > > > >
> > > > > > > > > #ifdef MADV_COLLAPSE
> > > > > > > > >         hugepages = 1;
> > > > > > > > > #endif
> > > > > > > > >         break;
> > > > > > > > >
> > > > > > > > > to avoid the test failures on old systems.
> > > > > > > > >
> > > > > > > > > Or any better ideas from you :)
> > > > > > > >
> > > > > > > > Hi Zorro,
> > > > > > > >
> > > > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > > > >
> > > > > > > Now that I've gotten to try this out --
> > > > > > >
> > > > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > > > >
> > > > > > > The second thing is that userspace programs can ensure the existence of
> > > > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > > > which requires that the underlying C library headers are new enough to
> > > > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > > > in a way that you hope is the same as...
> > > > > > >
> > > > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > > > headers package; and the third way is for the program to define it
> > > > > > > itself if nobody else does.
> > > > > > >
> > > > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > > > >
> > > > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > > > >
> > > > > Yes, because glibc provides the mmap() function that wraps
> > > > > syscall(__NR_mmap, ...);
> > > > >
> > > > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > > > headers package and support the feature.
> > > > >
> > > > > No, because some (most?) vendors backport new features into existing
> > > > > kernels without revving the version number of that kernel.
> > > >
> > > > Oh okay, I see. That makes sense, thanks for the explanation.
> > > >
> > > > >
> > > > > Maybe the following fixes things?
> > > > >
> > > > > --D
> > > > >
> > > > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > > > >
> > > > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > > > the test will fail with:
> > > > >
> > > > > >  QA output created by 760
> > > > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > +mapped writes DISABLED
> > > > > > +MADV_COLLAPSE not supported. Can't support -h
> > > > >
> > > > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > > > kernel is newer than the rest of userspace.
> > > > >
> > > > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > > ---
> > > > >  ltp/fsx.c         |    1 +
> > > > >  tests/generic/759 |    3 +++
> > > > >  tests/generic/760 |    3 +++
> > > > >  3 files changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > > > --- a/ltp/fsx.c
> > > > > +++ b/ltp/fsx.c
> > > > > @@ -20,6 +20,7 @@
> > > > >  #include <strings.h>
> > > > >  #include <sys/file.h>
> > > > >  #include <sys/mman.h>
> > > > > +#include <linux/mman.h>
> > > > >  #include <sys/uio.h>
> > > > >  #include <stdbool.h>
> > > > >  #ifdef HAVE_ERR_H
> > > > > diff --git a/tests/generic/759 b/tests/generic/759
> > > > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > > > --- a/tests/generic/759
> > > > > +++ b/tests/generic/759
> > > > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > > > >  _require_test
> > > > >  _require_thp
> > > > >
> > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > +
> > > > >  run_fsx -N 10000            -l 500000 -h
> > > > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > > > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > > > diff --git a/tests/generic/760 b/tests/generic/760
> > > > > index c71a196222ad3b..2fbd28502ae678 100755
> > > > > --- a/tests/generic/760
> > > > > +++ b/tests/generic/760
> > > > > @@ -15,6 +15,9 @@ _require_test
> > > > >  _require_odirect
> > > > >  _require_thp
> > > > >
> > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > +
> > > >
> > > > I tried this out locally and it works for me:
> > > >
> > > > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > > > Ran: generic/759
> > > > Not run: generic/759
> > > > Passed all 1 tests
> > > >
> > > > SECTION       -- fuse
> > > > =========================
> > > > Ran: generic/759
> > > > Not run: generic/759
> > > > Passed all 1 tests
> > >
> > > Does the test actually run if you /do/ have kernel/libc headers that
> > > define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
> > >
> > 
> > I'm not sure if I fully understand the subtext of your question but
> > yes, the test runs on my system when MADV_COLLAPSE is defined in the
> > kernel/libc headers.
> 
> Yep, you understood me correctly. :)
> 
> > For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
> > I tried this out by modifying the ifdef/ifndef checks in fsx to
> > MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
> > grep -q 'MADV_COLLAPSE not supported'' line catches that.
> 
> <nod> Sounds good then; I'll add this to my test clod and run it
> overnight.

The arm64 vms with 64k base pages spat out this:

--- /run/fstests/bin/tests/generic/759.out	2025-02-02 08:36:28.007248055 -0800
+++ /var/tmp/fstests/generic/759.out.bad	2025-02-03 16:51:34.862964640 -0800
@@ -1,4 +1,5 @@
 QA output created by 759
 fsx -N 10000 -l 500000 -h
-fsx -N 10000 -o 8192 -l 500000 -h
-fsx -N 10000 -o 128000 -l 500000 -h
+Seed set to 1
+madvise collapse for buf: Cannot allocate memory
+init_hugepages_buf failed for good_buf: Cannot allocate memory

Note that it was trying to create a 512M page(!) on a VM with 8G of
memory.  Normally one doesn't use large base page size in low memory
environments, but this /is/ fstestsland. 8-)

From commit 7d8faaf155454f ("mm/madvise: introduce MADV_COLLAPSE sync
hugepage collapse") :

	ENOMEM	Memory allocation failed or VMA not found
	EBUSY	Memcg charging failed
	EAGAIN	Required resource temporarily unavailable.  Try again
		might succeed.
	EINVAL	Other error: No PMD found, subpage doesn't have Present
		bit set, "Special" page no backed by struct page, VMA
		incorrectly sized, address not page-aligned, ...

It sounds like ENOMEM/EBUSY/EINVAL should result in
_notrun "Could not generate hugepage" ?  What are your thoughts?

--D

> --D
> 
> > 
> > > --D
> > >
> > > >
> > > > Thanks,
> > > > Joanne
> > > >
> > > > >  psize=`$here/src/feature -s`
> > > > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > > > >
> > > >
> 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Mon, Feb 3, 2025 at 8:21 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Mon, Feb 03, 2025 at 01:54:32PM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 03, 2025 at 01:40:39PM -0800, Joanne Koong wrote:
> > > On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > > > > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > > > > >
> > > > > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > > > > against problems like this in the future.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > > > > ---
> > > > > > > > > >
> > > > > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > > > > MADV_COLLAPSE:
> > > > > > > > > >
> > > > > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > >
> > > > > > > > > > and
> > > > > > > > > >
> > > > > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > >   +mapped writes DISABLED
> > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > >
> > > > > > > > > > I'm wondering ...
> > > > > > > > > >
> > > > > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > > > > --- a/ltp/fsx.c
> > > > > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > > > >  static struct option longopts[] = {
> > > > > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > > > > >
> > > > > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > > > > >               switch (ch) {
> > > > > > > > > > >               case 'b':
> > > > > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > > > > >               case 'g':
> > > > > > > > > > >                       filldata = *optarg;
> > > > > > > > > > >                       break;
> > > > > > > > > > > +             case 'h':
> > > > > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > > > > +                             "Can't support -h\n");
> > > > > > > > > > > +                     exit(86);
> > > > > > > > > > > +#endif
> > > > > > > > > > > +                     hugepages = 1;
> > > > > > > > > > > +                     break;
> > > > > > > > > >
> > > > > > > > > > ...
> > > > > > > > > > if we could change this part to:
> > > > > > > > > >
> > > > > > > > > > #ifdef MADV_COLLAPSE
> > > > > > > > > >         hugepages = 1;
> > > > > > > > > > #endif
> > > > > > > > > >         break;
> > > > > > > > > >
> > > > > > > > > > to avoid the test failures on old systems.
> > > > > > > > > >
> > > > > > > > > > Or any better ideas from you :)
> > > > > > > > >
> > > > > > > > > Hi Zorro,
> > > > > > > > >
> > > > > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > > > > >
> > > > > > > > Now that I've gotten to try this out --
> > > > > > > >
> > > > > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > > > > >
> > > > > > > > The second thing is that userspace programs can ensure the existence of
> > > > > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > > > > which requires that the underlying C library headers are new enough to
> > > > > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > > > > in a way that you hope is the same as...
> > > > > > > >
> > > > > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > > > > headers package; and the third way is for the program to define it
> > > > > > > > itself if nobody else does.
> > > > > > > >
> > > > > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > > > > >
> > > > > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > > > > >
> > > > > > Yes, because glibc provides the mmap() function that wraps
> > > > > > syscall(__NR_mmap, ...);
> > > > > >
> > > > > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > > > > headers package and support the feature.
> > > > > >
> > > > > > No, because some (most?) vendors backport new features into existing
> > > > > > kernels without revving the version number of that kernel.
> > > > >
> > > > > Oh okay, I see. That makes sense, thanks for the explanation.
> > > > >
> > > > > >
> > > > > > Maybe the following fixes things?
> > > > > >
> > > > > > --D
> > > > > >
> > > > > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > > > > >
> > > > > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > > > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > > > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > > > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > > > > the test will fail with:
> > > > > >
> > > > > > >  QA output created by 760
> > > > > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > +mapped writes DISABLED
> > > > > > > +MADV_COLLAPSE not supported. Can't support -h
> > > > > >
> > > > > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > > > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > > > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > > > > kernel is newer than the rest of userspace.
> > > > > >
> > > > > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > > > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > > > ---
> > > > > >  ltp/fsx.c         |    1 +
> > > > > >  tests/generic/759 |    3 +++
> > > > > >  tests/generic/760 |    3 +++
> > > > > >  3 files changed, 7 insertions(+)
> > > > > >
> > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > > > > --- a/ltp/fsx.c
> > > > > > +++ b/ltp/fsx.c
> > > > > > @@ -20,6 +20,7 @@
> > > > > >  #include <strings.h>
> > > > > >  #include <sys/file.h>
> > > > > >  #include <sys/mman.h>
> > > > > > +#include <linux/mman.h>
> > > > > >  #include <sys/uio.h>
> > > > > >  #include <stdbool.h>
> > > > > >  #ifdef HAVE_ERR_H
> > > > > > diff --git a/tests/generic/759 b/tests/generic/759
> > > > > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > > > > --- a/tests/generic/759
> > > > > > +++ b/tests/generic/759
> > > > > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > > > > >  _require_test
> > > > > >  _require_thp
> > > > > >
> > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > +
> > > > > >  run_fsx -N 10000            -l 500000 -h
> > > > > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > > > > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > > > > diff --git a/tests/generic/760 b/tests/generic/760
> > > > > > index c71a196222ad3b..2fbd28502ae678 100755
> > > > > > --- a/tests/generic/760
> > > > > > +++ b/tests/generic/760
> > > > > > @@ -15,6 +15,9 @@ _require_test
> > > > > >  _require_odirect
> > > > > >  _require_thp
> > > > > >
> > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > +
> > > > >
> > > > > I tried this out locally and it works for me:
> > > > >
> > > > > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > > > > Ran: generic/759
> > > > > Not run: generic/759
> > > > > Passed all 1 tests
> > > > >
> > > > > SECTION       -- fuse
> > > > > =========================
> > > > > Ran: generic/759
> > > > > Not run: generic/759
> > > > > Passed all 1 tests
> > > >
> > > > Does the test actually run if you /do/ have kernel/libc headers that
> > > > define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
> > > >
> > >
> > > I'm not sure if I fully understand the subtext of your question but
> > > yes, the test runs on my system when MADV_COLLAPSE is defined in the
> > > kernel/libc headers.
> >
> > Yep, you understood me correctly. :)
> >
> > > For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
> > > I tried this out by modifying the ifdef/ifndef checks in fsx to
> > > MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
> > > grep -q 'MADV_COLLAPSE not supported'' line catches that.
> >
> > <nod> Sounds good then; I'll add this to my test clod and run it
> > overnight.
>
> The arm64 vms with 64k base pages spat out this:
>
> --- /run/fstests/bin/tests/generic/759.out      2025-02-02 08:36:28.007248055 -0800
> +++ /var/tmp/fstests/generic/759.out.bad        2025-02-03 16:51:34.862964640 -0800
> @@ -1,4 +1,5 @@
>  QA output created by 759
>  fsx -N 10000 -l 500000 -h
> -fsx -N 10000 -o 8192 -l 500000 -h
> -fsx -N 10000 -o 128000 -l 500000 -h
> +Seed set to 1
> +madvise collapse for buf: Cannot allocate memory
> +init_hugepages_buf failed for good_buf: Cannot allocate memory
>
> Note that it was trying to create a 512M page(!) on a VM with 8G of
> memory.  Normally one doesn't use large base page size in low memory
> environments, but this /is/ fstestsland. 8-)
>
> From commit 7d8faaf155454f ("mm/madvise: introduce MADV_COLLAPSE sync
> hugepage collapse") :
>
>         ENOMEM  Memory allocation failed or VMA not found
>         EBUSY   Memcg charging failed
>         EAGAIN  Required resource temporarily unavailable.  Try again
>                 might succeed.
>         EINVAL  Other error: No PMD found, subpage doesn't have Present
>                 bit set, "Special" page no backed by struct page, VMA
>                 incorrectly sized, address not page-aligned, ...
>
> It sounds like ENOMEM/EBUSY/EINVAL should result in
> _notrun "Could not generate hugepage" ?  What are your thoughts?
>

Thanks for running it overnight. This sounds good to me, but will this
be robust against false negatives? For example, if it succeeds when
we're doing the

$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not
supported' &&  _notrun "fsx binary does not support MADV_COLLAPSE"

check, does that guarantee that the actual run won't error out with
ENOMEM/EBUSY/EINVAL? It seems like there could be the case where it
passes the check but then when it actually runs, the system state
memory state may have changed and now memcg charging or the memory
allocation fails? EAGAIN seems a bit iffy to me - hopefully this
doesn't happen often but if it does, it would likely be a false
negative fail for the generic test?

Maybe instead of "$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q
'MADV_COLLAPSE not supported' &&  _notrun "fsx binary does not support
MADV_COLLAPSE"", we should run the test and then if the output to the
.out file is "MADV_COLLAPSE not supported", then we deem that a
pass/success? Not sure if this is possible in the fstest
infrastructure though for checking against two possible outputs in the
.out file. What are your thoughts?


Thanks,
Joanne

> --D
>
> > --D
> >
> > >
> > > > --D
> > > >
> > > > >
> > > > > Thanks,
> > > > > Joanne
> > > > >
> > > > > >  psize=`$here/src/feature -s`
> > > > > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > > > > >
> > > > >
> >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Darrick J. Wong]

On Tue, Feb 04, 2025 at 12:50:04PM -0800, Joanne Koong wrote:
> On Mon, Feb 3, 2025 at 8:21 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > On Mon, Feb 03, 2025 at 01:54:32PM -0800, Darrick J. Wong wrote:
> > > On Mon, Feb 03, 2025 at 01:40:39PM -0800, Joanne Koong wrote:
> > > > On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > >
> > > > > On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > > > > > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > > > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > > > > > >
> > > > > > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > > > > > against problems like this in the future.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > > > > > MADV_COLLAPSE:
> > > > > > > > > > >
> > > > > > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > > >
> > > > > > > > > > > and
> > > > > > > > > > >
> > > > > > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > >   +mapped writes DISABLED
> > > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > > >
> > > > > > > > > > > I'm wondering ...
> > > > > > > > > > >
> > > > > > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > > > > > --- a/ltp/fsx.c
> > > > > > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > > > > >  static struct option longopts[] = {
> > > > > > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > > > > > >
> > > > > > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > > > > > >               switch (ch) {
> > > > > > > > > > > >               case 'b':
> > > > > > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > > > > > >               case 'g':
> > > > > > > > > > > >                       filldata = *optarg;
> > > > > > > > > > > >                       break;
> > > > > > > > > > > > +             case 'h':
> > > > > > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > > > > > +                             "Can't support -h\n");
> > > > > > > > > > > > +                     exit(86);
> > > > > > > > > > > > +#endif
> > > > > > > > > > > > +                     hugepages = 1;
> > > > > > > > > > > > +                     break;
> > > > > > > > > > >
> > > > > > > > > > > ...
> > > > > > > > > > > if we could change this part to:
> > > > > > > > > > >
> > > > > > > > > > > #ifdef MADV_COLLAPSE
> > > > > > > > > > >         hugepages = 1;
> > > > > > > > > > > #endif
> > > > > > > > > > >         break;
> > > > > > > > > > >
> > > > > > > > > > > to avoid the test failures on old systems.
> > > > > > > > > > >
> > > > > > > > > > > Or any better ideas from you :)
> > > > > > > > > >
> > > > > > > > > > Hi Zorro,
> > > > > > > > > >
> > > > > > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > > > > > >
> > > > > > > > > Now that I've gotten to try this out --
> > > > > > > > >
> > > > > > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > > > > > >
> > > > > > > > > The second thing is that userspace programs can ensure the existence of
> > > > > > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > > > > > which requires that the underlying C library headers are new enough to
> > > > > > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > > > > > in a way that you hope is the same as...
> > > > > > > > >
> > > > > > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > > > > > headers package; and the third way is for the program to define it
> > > > > > > > > itself if nobody else does.
> > > > > > > > >
> > > > > > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > > > > > >
> > > > > > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > > > > > >
> > > > > > > Yes, because glibc provides the mmap() function that wraps
> > > > > > > syscall(__NR_mmap, ...);
> > > > > > >
> > > > > > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > > > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > > > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > > > > > headers package and support the feature.
> > > > > > >
> > > > > > > No, because some (most?) vendors backport new features into existing
> > > > > > > kernels without revving the version number of that kernel.
> > > > > >
> > > > > > Oh okay, I see. That makes sense, thanks for the explanation.
> > > > > >
> > > > > > >
> > > > > > > Maybe the following fixes things?
> > > > > > >
> > > > > > > --D
> > > > > > >
> > > > > > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > > > > > >
> > > > > > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > > > > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > > > > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > > > > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > > > > > the test will fail with:
> > > > > > >
> > > > > > > >  QA output created by 760
> > > > > > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > +mapped writes DISABLED
> > > > > > > > +MADV_COLLAPSE not supported. Can't support -h
> > > > > > >
> > > > > > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > > > > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > > > > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > > > > > kernel is newer than the rest of userspace.
> > > > > > >
> > > > > > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > > > > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > > > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > > > > ---
> > > > > > >  ltp/fsx.c         |    1 +
> > > > > > >  tests/generic/759 |    3 +++
> > > > > > >  tests/generic/760 |    3 +++
> > > > > > >  3 files changed, 7 insertions(+)
> > > > > > >
> > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > > > > > --- a/ltp/fsx.c
> > > > > > > +++ b/ltp/fsx.c
> > > > > > > @@ -20,6 +20,7 @@
> > > > > > >  #include <strings.h>
> > > > > > >  #include <sys/file.h>
> > > > > > >  #include <sys/mman.h>
> > > > > > > +#include <linux/mman.h>
> > > > > > >  #include <sys/uio.h>
> > > > > > >  #include <stdbool.h>
> > > > > > >  #ifdef HAVE_ERR_H
> > > > > > > diff --git a/tests/generic/759 b/tests/generic/759
> > > > > > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > > > > > --- a/tests/generic/759
> > > > > > > +++ b/tests/generic/759
> > > > > > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > > > > > >  _require_test
> > > > > > >  _require_thp
> > > > > > >
> > > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > > +
> > > > > > >  run_fsx -N 10000            -l 500000 -h
> > > > > > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > > > > > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > > > > > diff --git a/tests/generic/760 b/tests/generic/760
> > > > > > > index c71a196222ad3b..2fbd28502ae678 100755
> > > > > > > --- a/tests/generic/760
> > > > > > > +++ b/tests/generic/760
> > > > > > > @@ -15,6 +15,9 @@ _require_test
> > > > > > >  _require_odirect
> > > > > > >  _require_thp
> > > > > > >
> > > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > > +
> > > > > >
> > > > > > I tried this out locally and it works for me:
> > > > > >
> > > > > > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > > > > > Ran: generic/759
> > > > > > Not run: generic/759
> > > > > > Passed all 1 tests
> > > > > >
> > > > > > SECTION       -- fuse
> > > > > > =========================
> > > > > > Ran: generic/759
> > > > > > Not run: generic/759
> > > > > > Passed all 1 tests
> > > > >
> > > > > Does the test actually run if you /do/ have kernel/libc headers that
> > > > > define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
> > > > >
> > > >
> > > > I'm not sure if I fully understand the subtext of your question but
> > > > yes, the test runs on my system when MADV_COLLAPSE is defined in the
> > > > kernel/libc headers.
> > >
> > > Yep, you understood me correctly. :)
> > >
> > > > For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
> > > > I tried this out by modifying the ifdef/ifndef checks in fsx to
> > > > MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
> > > > grep -q 'MADV_COLLAPSE not supported'' line catches that.
> > >
> > > <nod> Sounds good then; I'll add this to my test clod and run it
> > > overnight.
> >
> > The arm64 vms with 64k base pages spat out this:
> >
> > --- /run/fstests/bin/tests/generic/759.out      2025-02-02 08:36:28.007248055 -0800
> > +++ /var/tmp/fstests/generic/759.out.bad        2025-02-03 16:51:34.862964640 -0800
> > @@ -1,4 +1,5 @@
> >  QA output created by 759
> >  fsx -N 10000 -l 500000 -h
> > -fsx -N 10000 -o 8192 -l 500000 -h
> > -fsx -N 10000 -o 128000 -l 500000 -h
> > +Seed set to 1
> > +madvise collapse for buf: Cannot allocate memory
> > +init_hugepages_buf failed for good_buf: Cannot allocate memory
> >
> > Note that it was trying to create a 512M page(!) on a VM with 8G of
> > memory.  Normally one doesn't use large base page size in low memory
> > environments, but this /is/ fstestsland. 8-)
> >
> > From commit 7d8faaf155454f ("mm/madvise: introduce MADV_COLLAPSE sync
> > hugepage collapse") :
> >
> >         ENOMEM  Memory allocation failed or VMA not found
> >         EBUSY   Memcg charging failed
> >         EAGAIN  Required resource temporarily unavailable.  Try again
> >                 might succeed.
> >         EINVAL  Other error: No PMD found, subpage doesn't have Present
> >                 bit set, "Special" page no backed by struct page, VMA
> >                 incorrectly sized, address not page-aligned, ...
> >
> > It sounds like ENOMEM/EBUSY/EINVAL should result in
> > _notrun "Could not generate hugepage" ?  What are your thoughts?
> >
> 
> Thanks for running it overnight. This sounds good to me, but will this
> be robust against false negatives? For example, if it succeeds when
> we're doing the
> 
> $here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not
> supported' &&  _notrun "fsx binary does not support MADV_COLLAPSE"
> 
> check, does that guarantee that the actual run won't error out with
> ENOMEM/EBUSY/EINVAL?

Nope.  That code only checks that the fsx binary was built with
MADV_COLLAPSE support, not that any of it actually works.

>                      It seems like there could be the case where it
> passes the check but then when it actually runs, the system state
> memory state may have changed and now memcg charging or the memory
> allocation fails?

Indeed, I think the error I saw is exactly this happening.

In the end I turned the fsx -h invocation into a helper that checks for
any of those three errors (ENOMEM/BUSY/INVAL) and _notruns the test if
we can't even get a hugepage at the start.

# Run fsx with -h(ugepage buffers).  If we can't set up a hugepage then skip
# the test, but if any other error occurs then exit the test.
_run_hugepage_fsx() {
	_run_fsx "$@" -h &> $tmp.hugepage_fsx
	local res=$?
	if [ $res -eq 103 ]; then
		# According to the MADV_COLLAPSE manpage, these three errors
		# can happen if the kernel could not collapse a collection of
		# pages into a single huge page.
		grep -q -E ' for hugebuf: (Cannot allocate memory|Device or resource busy|Resource temporarily unavailable)' $tmp.hugepage_fsx && \
			_notrun "Could not set up huge page for test"
	fi
	cat $tmp.hugepage_fsx
	rm -f $tmp.hugepage_fsx
	test $res -ne 0 && exit 1
	return 0
}

Note that it won't prevent or paper over subsequent MADV_COLLAPSE
failures once the process is up and running, though as long as nothing
else in fsx fails or corrupts the file, the collapse failures won't be
reported.  (As is the case now).

Anyway I'll send patches for both issues.

> allocation fails? EAGAIN seems a bit iffy to me - hopefully this
> doesn't happen often but if it does, it would likely be a false
> negative fail for the generic test?
> 
> Maybe instead of "$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q
> 'MADV_COLLAPSE not supported' &&  _notrun "fsx binary does not support
> MADV_COLLAPSE"", we should run the test and then if the output to the
> .out file is "MADV_COLLAPSE not supported", then we deem that a
> pass/success? Not sure if this is possible in the fstest
> infrastructure though for checking against two possible outputs in the
> .out file. What are your thoughts?

You ... can switch the .out(put) dynamically, but the mechanism to do it
is underdocumented and quirky; and it makes understanding what the test
does rather difficult.  See _link_out_file in xfs/614 if you want to
open that Pandora's box.

--D

> 
> 
> Thanks,
> Joanne
> 
> > --D
> >
> > > --D
> > >
> > > >
> > > > > --D
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Joanne
> > > > > >
> > > > > > >  psize=`$here/src/feature -s`
> > > > > > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > > > > > >
> > > > > >
> > >
> 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Tue, Feb 4, 2025 at 1:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Feb 04, 2025 at 12:50:04PM -0800, Joanne Koong wrote:
> > On Mon, Feb 3, 2025 at 8:21 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > On Mon, Feb 03, 2025 at 01:54:32PM -0800, Darrick J. Wong wrote:
> > > > On Mon, Feb 03, 2025 at 01:40:39PM -0800, Joanne Koong wrote:
> > > > > On Mon, Feb 3, 2025 at 12:01 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Feb 03, 2025 at 11:57:23AM -0800, Joanne Koong wrote:
> > > > > > > On Mon, Feb 3, 2025 at 11:41 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 03, 2025 at 11:23:20AM -0800, Joanne Koong wrote:
> > > > > > > > > On Mon, Feb 3, 2025 at 10:59 AM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > > > > > > > > > > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > > > > > > > > > > Add support for reads/writes from buffers backed by hugepages.
> > > > > > > > > > > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > > > > > > > > > > on systems where THP capabilities are enabled.
> > > > > > > > > > > > >
> > > > > > > > > > > > > This is motivated by a recent bug that was due to faulty handling of
> > > > > > > > > > > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > > > > > > > > > > against problems like this in the future.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > > > > > > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > > > > > > > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > > > > > > > > ---
> > > > > > > > > > > >
> > > > > > > > > > > > Those two test cases fail on old system which doesn't support
> > > > > > > > > > > > MADV_COLLAPSE:
> > > > > > > > > > > >
> > > > > > > > > > > >    fsx -N 10000 -l 500000 -h
> > > > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > > > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > > > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > > > >
> > > > > > > > > > > > and
> > > > > > > > > > > >
> > > > > > > > > > > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > > > >   +mapped writes DISABLED
> > > > > > > > > > > >   +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > > > > > >
> > > > > > > > > > > > I'm wondering ...
> > > > > > > > > > > >
> > > > > > > > > > > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > > > > > > > > > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > > > > > > index 41933354..3be383c6 100644
> > > > > > > > > > > > > --- a/ltp/fsx.c
> > > > > > > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > > > > > >  static struct option longopts[] = {
> > > > > > > > > > > > >       {"replay-ops", required_argument, 0, 256},
> > > > > > > > > > > > >       {"record-ops", optional_argument, 0, 255},
> > > > > > > > > > > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > > > > > > > > > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > > > > > > > > > > >
> > > > > > > > > > > > >       while ((ch = getopt_long(argc, argv,
> > > > > > > > > > > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > > > > > > > > > >                                longopts, NULL)) != EOF)
> > > > > > > > > > > > >               switch (ch) {
> > > > > > > > > > > > >               case 'b':
> > > > > > > > > > > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > > > > > > > > > > >               case 'g':
> > > > > > > > > > > > >                       filldata = *optarg;
> > > > > > > > > > > > >                       break;
> > > > > > > > > > > > > +             case 'h':
> > > > > > > > > > > > > +#ifndef MADV_COLLAPSE
> > > > > > > > > > > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > > > > > > > > > > +                             "Can't support -h\n");
> > > > > > > > > > > > > +                     exit(86);
> > > > > > > > > > > > > +#endif
> > > > > > > > > > > > > +                     hugepages = 1;
> > > > > > > > > > > > > +                     break;
> > > > > > > > > > > >
> > > > > > > > > > > > ...
> > > > > > > > > > > > if we could change this part to:
> > > > > > > > > > > >
> > > > > > > > > > > > #ifdef MADV_COLLAPSE
> > > > > > > > > > > >         hugepages = 1;
> > > > > > > > > > > > #endif
> > > > > > > > > > > >         break;
> > > > > > > > > > > >
> > > > > > > > > > > > to avoid the test failures on old systems.
> > > > > > > > > > > >
> > > > > > > > > > > > Or any better ideas from you :)
> > > > > > > > > > >
> > > > > > > > > > > Hi Zorro,
> > > > > > > > > > >
> > > > > > > > > > > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > > > > > > > > > > do you think about skipping generic/758 and generic/759 if the kernel
> > > > > > > > > > > version is older than 6.1? That to me seems more preferable than the
> > > > > > > > > > > paste above, as the paste above would execute the test as if it did
> > > > > > > > > > > test hugepages when in reality it didn't, which would be misleading.
> > > > > > > > > >
> > > > > > > > > > Now that I've gotten to try this out --
> > > > > > > > > >
> > > > > > > > > > There's a couple of things going on here.  The first is that generic/759
> > > > > > > > > > and 760 need to check if invoking fsx -h causes it to spit out the
> > > > > > > > > > "MADV_COLLAPSE not supported" error and _notrun the test.
> > > > > > > > > >
> > > > > > > > > > The second thing is that userspace programs can ensure the existence of
> > > > > > > > > > MADV_COLLAPSE in multiple ways.  The first way is through sys/mman.h,
> > > > > > > > > > which requires that the underlying C library headers are new enough to
> > > > > > > > > > include a definition.  glibc 2.37 is new enough, but even things like
> > > > > > > > > > Debian 12 and RHEL 9 aren't new enough to have that.  Other C libraries
> > > > > > > > > > might not follow glibc's practice of wrapping and/or redefining symbols
> > > > > > > > > > in a way that you hope is the same as...
> > > > > > > > > >
> > > > > > > > > > The second way is through linux/mman.h, which comes from the kernel
> > > > > > > > > > headers package; and the third way is for the program to define it
> > > > > > > > > > itself if nobody else does.
> > > > > > > > > >
> > > > > > > > > > So I think the easiest way to fix the fsx.c build is to include
> > > > > > > > > > linux/mman.h in addition to sys/mman.h.  Sorry I didn't notice these
> > > > > > > > >
> > > > > > > > > Thanks for your input. Do we still need sys/mman.h if linux/mman.h is added?
> > > > > > > >
> > > > > > > > Yes, because glibc provides the mmap() function that wraps
> > > > > > > > syscall(__NR_mmap, ...);
> > > > > > > >
> > > > > > > > > For generic/758 and 759, does it suffice to gate this on whether the
> > > > > > > > > kernel version if 6.1+ and _notrun if not? My understanding is that
> > > > > > > > > any kernel version 6.1 or newer will have MADV_COLLAPSE in its kernel
> > > > > > > > > headers package and support the feature.
> > > > > > > >
> > > > > > > > No, because some (most?) vendors backport new features into existing
> > > > > > > > kernels without revving the version number of that kernel.
> > > > > > >
> > > > > > > Oh okay, I see. That makes sense, thanks for the explanation.
> > > > > > >
> > > > > > > >
> > > > > > > > Maybe the following fixes things?
> > > > > > > >
> > > > > > > > --D
> > > > > > > >
> > > > > > > > generic/759,760: fix MADV_COLLAPSE detection and inclusion
> > > > > > > >
> > > > > > > > On systems with "old" C libraries such as glibc 2.36 in Debian 12, the
> > > > > > > > MADV_COLLAPSE flag might not be defined in any of the header files
> > > > > > > > pulled in by sys/mman.h, which means that the fsx binary might not get
> > > > > > > > built with any of the MADV_COLLAPSE code.  If the kernel supports THP,
> > > > > > > > the test will fail with:
> > > > > > > >
> > > > > > > > >  QA output created by 760
> > > > > > > > >  fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > > > > > > > > +mapped writes DISABLED
> > > > > > > > > +MADV_COLLAPSE not supported. Can't support -h
> > > > > > > >
> > > > > > > > Fix both tests to detect fsx binaries that don't support MADV_COLLAPSE,
> > > > > > > > then fix fsx.c to include the mman.h from the kernel headers (aka
> > > > > > > > linux/mman.h) so that we can actually test IOs to and from THPs if the
> > > > > > > > kernel is newer than the rest of userspace.
> > > > > > > >
> > > > > > > > Cc: <fstests@vger.kernel.org> # v2025.02.02
> > > > > > > > Fixes: 627289232371e3 ("generic: add tests for read/writes from hugepages-backed buffers")
> > > > > > > > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > > > > > > > ---
> > > > > > > >  ltp/fsx.c         |    1 +
> > > > > > > >  tests/generic/759 |    3 +++
> > > > > > > >  tests/generic/760 |    3 +++
> > > > > > > >  3 files changed, 7 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > > > > > index 634c496ffe9317..cf9502a74c17a7 100644
> > > > > > > > --- a/ltp/fsx.c
> > > > > > > > +++ b/ltp/fsx.c
> > > > > > > > @@ -20,6 +20,7 @@
> > > > > > > >  #include <strings.h>
> > > > > > > >  #include <sys/file.h>
> > > > > > > >  #include <sys/mman.h>
> > > > > > > > +#include <linux/mman.h>
> > > > > > > >  #include <sys/uio.h>
> > > > > > > >  #include <stdbool.h>
> > > > > > > >  #ifdef HAVE_ERR_H
> > > > > > > > diff --git a/tests/generic/759 b/tests/generic/759
> > > > > > > > index 6c74478aa8a0e0..3549c5ed6a9299 100755
> > > > > > > > --- a/tests/generic/759
> > > > > > > > +++ b/tests/generic/759
> > > > > > > > @@ -14,6 +14,9 @@ _begin_fstest rw auto quick
> > > > > > > >  _require_test
> > > > > > > >  _require_thp
> > > > > > > >
> > > > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > > > +
> > > > > > > >  run_fsx -N 10000            -l 500000 -h
> > > > > > > >  run_fsx -N 10000  -o 8192   -l 500000 -h
> > > > > > > >  run_fsx -N 10000  -o 128000 -l 500000 -h
> > > > > > > > diff --git a/tests/generic/760 b/tests/generic/760
> > > > > > > > index c71a196222ad3b..2fbd28502ae678 100755
> > > > > > > > --- a/tests/generic/760
> > > > > > > > +++ b/tests/generic/760
> > > > > > > > @@ -15,6 +15,9 @@ _require_test
> > > > > > > >  _require_odirect
> > > > > > > >  _require_thp
> > > > > > > >
> > > > > > > > +$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not supported' && \
> > > > > > > > +       _notrun "fsx binary does not support MADV_COLLAPSE"
> > > > > > > > +
> > > > > > >
> > > > > > > I tried this out locally and it works for me:
> > > > > > >
> > > > > > > generic/759 8s ... [not run] fsx binary does not support MADV_COLLAPSE
> > > > > > > Ran: generic/759
> > > > > > > Not run: generic/759
> > > > > > > Passed all 1 tests
> > > > > > >
> > > > > > > SECTION       -- fuse
> > > > > > > =========================
> > > > > > > Ran: generic/759
> > > > > > > Not run: generic/759
> > > > > > > Passed all 1 tests
> > > > > >
> > > > > > Does the test actually run if you /do/ have kernel/libc headers that
> > > > > > define MADV_COLLAPSE?  And if so, does that count as a Tested-by?
> > > > > >
> > > > >
> > > > > I'm not sure if I fully understand the subtext of your question but
> > > > > yes, the test runs on my system when MADV_COLLAPSE is defined in the
> > > > > kernel/libc headers.
> > > >
> > > > Yep, you understood me correctly. :)
> > > >
> > > > > For sanity checking the inverse case, (eg MADV_COLLAPSE not defined),
> > > > > I tried this out by modifying the ifdef/ifndef checks in fsx to
> > > > > MADV_COLLAPSE_ to see if the '$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 |
> > > > > grep -q 'MADV_COLLAPSE not supported'' line catches that.
> > > >
> > > > <nod> Sounds good then; I'll add this to my test clod and run it
> > > > overnight.
> > >
> > > The arm64 vms with 64k base pages spat out this:
> > >
> > > --- /run/fstests/bin/tests/generic/759.out      2025-02-02 08:36:28.007248055 -0800
> > > +++ /var/tmp/fstests/generic/759.out.bad        2025-02-03 16:51:34.862964640 -0800
> > > @@ -1,4 +1,5 @@
> > >  QA output created by 759
> > >  fsx -N 10000 -l 500000 -h
> > > -fsx -N 10000 -o 8192 -l 500000 -h
> > > -fsx -N 10000 -o 128000 -l 500000 -h
> > > +Seed set to 1
> > > +madvise collapse for buf: Cannot allocate memory
> > > +init_hugepages_buf failed for good_buf: Cannot allocate memory
> > >
> > > Note that it was trying to create a 512M page(!) on a VM with 8G of
> > > memory.  Normally one doesn't use large base page size in low memory
> > > environments, but this /is/ fstestsland. 8-)
> > >
> > > From commit 7d8faaf155454f ("mm/madvise: introduce MADV_COLLAPSE sync
> > > hugepage collapse") :
> > >
> > >         ENOMEM  Memory allocation failed or VMA not found
> > >         EBUSY   Memcg charging failed
> > >         EAGAIN  Required resource temporarily unavailable.  Try again
> > >                 might succeed.
> > >         EINVAL  Other error: No PMD found, subpage doesn't have Present
> > >                 bit set, "Special" page no backed by struct page, VMA
> > >                 incorrectly sized, address not page-aligned, ...
> > >
> > > It sounds like ENOMEM/EBUSY/EINVAL should result in
> > > _notrun "Could not generate hugepage" ?  What are your thoughts?
> > >
> >
> > Thanks for running it overnight. This sounds good to me, but will this
> > be robust against false negatives? For example, if it succeeds when
> > we're doing the
> >
> > $here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q 'MADV_COLLAPSE not
> > supported' &&  _notrun "fsx binary does not support MADV_COLLAPSE"
> >
> > check, does that guarantee that the actual run won't error out with
> > ENOMEM/EBUSY/EINVAL?
>
> Nope.  That code only checks that the fsx binary was built with
> MADV_COLLAPSE support, not that any of it actually works.
>
> >                      It seems like there could be the case where it
> > passes the check but then when it actually runs, the system state
> > memory state may have changed and now memcg charging or the memory
> > allocation fails?
>
> Indeed, I think the error I saw is exactly this happening.
>
> In the end I turned the fsx -h invocation into a helper that checks for
> any of those three errors (ENOMEM/BUSY/INVAL) and _notruns the test if
> we can't even get a hugepage at the start.
>
> # Run fsx with -h(ugepage buffers).  If we can't set up a hugepage then skip
> # the test, but if any other error occurs then exit the test.
> _run_hugepage_fsx() {
>         _run_fsx "$@" -h &> $tmp.hugepage_fsx
>         local res=$?
>         if [ $res -eq 103 ]; then
>                 # According to the MADV_COLLAPSE manpage, these three errors
>                 # can happen if the kernel could not collapse a collection of
>                 # pages into a single huge page.
>                 grep -q -E ' for hugebuf: (Cannot allocate memory|Device or resource busy|Resource temporarily unavailable)' $tmp.hugepage_fsx && \
>                         _notrun "Could not set up huge page for test"
>         fi
>         cat $tmp.hugepage_fsx
>         rm -f $tmp.hugepage_fsx
>         test $res -ne 0 && exit 1
>         return 0
> }

Awesome, thank you!

>
> Note that it won't prevent or paper over subsequent MADV_COLLAPSE
> failures once the process is up and running, though as long as nothing
> else in fsx fails or corrupts the file, the collapse failures won't be
> reported.  (As is the case now).
>
> Anyway I'll send patches for both issues.
>
> > allocation fails? EAGAIN seems a bit iffy to me - hopefully this
> > doesn't happen often but if it does, it would likely be a false
> > negative fail for the generic test?
> >
> > Maybe instead of "$here/ltp/fsx -N 0 -h $TEST_DIR 2>&1 | grep -q
> > 'MADV_COLLAPSE not supported' &&  _notrun "fsx binary does not support
> > MADV_COLLAPSE"", we should run the test and then if the output to the
> > .out file is "MADV_COLLAPSE not supported", then we deem that a
> > pass/success? Not sure if this is possible in the fstest
> > infrastructure though for checking against two possible outputs in the
> > .out file. What are your thoughts?
>
> You ... can switch the .out(put) dynamically, but the mechanism to do it
> is underdocumented and quirky; and it makes understanding what the test
> does rather difficult.  See _link_out_file in xfs/614 if you want to
> open that Pandora's box.
>
> --D
>
> >
> >
> > Thanks,
> > Joanne
> >
> > > --D
> > >
> > > > --D
> > > >
> > > > >
> > > > > > --D
> > > > > >
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Joanne
> > > > > > >
> > > > > > > >  psize=`$here/src/feature -s`
> > > > > > > >  bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
> > > > > > > >
> > > > > > >
> > > >
> >

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Zorro Lang]

On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > Add support for reads/writes from buffers backed by hugepages.
> > > This can be enabled through the '-h' flag. This flag should only be used
> > > on systems where THP capabilities are enabled.
> > >
> > > This is motivated by a recent bug that was due to faulty handling of
> > > userspace buffers backed by hugepages. This patch is a mitigation
> > > against problems like this in the future.
> > >
> > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> >
> > Those two test cases fail on old system which doesn't support
> > MADV_COLLAPSE:
> >
> >    fsx -N 10000 -l 500000 -h
> >   -fsx -N 10000 -o 8192 -l 500000 -h
> >   -fsx -N 10000 -o 128000 -l 500000 -h
> >   +MADV_COLLAPSE not supported. Can't support -h
> >
> > and
> >
> >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> >   +mapped writes DISABLED
> >   +MADV_COLLAPSE not supported. Can't support -h
> >
> > I'm wondering ...
> >
> > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > index 41933354..3be383c6 100644
> > > --- a/ltp/fsx.c
> > > +++ b/ltp/fsx.c
> > > @@ -190,6 +190,16 @@ int      o_direct;                       /* -Z */
> > >  int  aio = 0;
> > >  int  uring = 0;
> > >  int  mark_nr = 0;
> > > +int  hugepages = 0;                  /* -h flag */
> > > +
> > > +/* Stores info needed to periodically collapse hugepages */
> > > +struct hugepages_collapse_info {
> > > +     void *orig_good_buf;
> > > +     long good_buf_size;
> > > +     void *orig_temp_buf;
> > > +     long temp_buf_size;
> > > +};
> > > +struct hugepages_collapse_info hugepages_info;
> > >
> > >  int page_size;
> > >  int page_mask;
> > > @@ -2471,7 +2481,7 @@ void
> > >  usage(void)
> > >  {
> > >       fprintf(stdout, "usage: %s",
> > > -             "fsx [-dfknqxyzBEFHIJKLORWXZ0]\n\
> > > +             "fsx [-dfhknqxyzBEFHIJKLORWXZ0]\n\
> > >          [-b opnum] [-c Prob] [-g filldata] [-i logdev] [-j logid]\n\
> > >          [-l flen] [-m start:end] [-o oplen] [-p progressinterval]\n\
> > >          [-r readbdy] [-s style] [-t truncbdy] [-w writebdy]\n\
> > > @@ -2483,8 +2493,11 @@ usage(void)
> > >       -d: debug output for all operations\n\
> > >       -e: pollute post-eof on size changes (default 0)\n\
> > >       -f: flush and invalidate cache after I/O\n\
> > > -     -g X: write character X instead of random generated data\n\
> > > -     -i logdev: do integrity testing, logdev is the dm log writes device\n\
> > > +     -g X: write character X instead of random generated data\n"
> > > +#ifdef MADV_COLLAPSE
> > > +"    -h hugepages: use buffers backed by hugepages for reads/writes\n"
> > > +#endif
> > > +"    -i logdev: do integrity testing, logdev is the dm log writes device\n\
> > >       -j logid: prefix debug log messsages with this id\n\
> > >       -k: do not truncate existing file and use its size as upper bound on file size\n\
> > >       -l flen: the upper bound on file size (default 262144)\n\
> > > @@ -2833,11 +2846,41 @@ __test_fallocate(int mode, const char *mode_str)
> > >  #endif
> > >  }
> > >
> > > +/*
> > > + * Reclaim may break up hugepages, so do a best-effort collapse every once in
> > > + * a while.
> > > + */
> > > +static void
> > > +collapse_hugepages(void)
> > > +{
> > > +#ifdef MADV_COLLAPSE
> > > +     int ret;
> > > +
> > > +     /* re-collapse every 16k fsxops after we start */
> > > +     if (!numops || (numops & ((1U << 14) - 1)))
> > > +             return;
> > > +
> > > +     ret = madvise(hugepages_info.orig_good_buf,
> > > +                   hugepages_info.good_buf_size, MADV_COLLAPSE);
> > > +     if (ret)
> > > +             prt("collapsing hugepages for good_buf failed (numops=%llu): %s\n",
> > > +                  numops, strerror(errno));
> > > +     ret = madvise(hugepages_info.orig_temp_buf,
> > > +                   hugepages_info.temp_buf_size, MADV_COLLAPSE);
> > > +     if (ret)
> > > +             prt("collapsing hugepages for temp_buf failed (numops=%llu): %s\n",
> > > +                  numops, strerror(errno));
> > > +#endif
> > > +}
> > > +
> > >  bool
> > >  keep_running(void)
> > >  {
> > >       int ret;
> > >
> > > +     if (hugepages)
> > > +             collapse_hugepages();
> > > +
> > >       if (deadline.tv_nsec) {
> > >               struct timespec now;
> > >
> > > @@ -2856,6 +2899,103 @@ keep_running(void)
> > >       return numops-- != 0;
> > >  }
> > >
> > > +static long
> > > +get_hugepage_size(void)
> > > +{
> > > +     const char str[] = "Hugepagesize:";
> > > +     size_t str_len =  sizeof(str) - 1;
> > > +     unsigned int hugepage_size = 0;
> > > +     char buffer[64];
> > > +     FILE *file;
> > > +
> > > +     file = fopen("/proc/meminfo", "r");
> > > +     if (!file) {
> > > +             prterr("get_hugepage_size: fopen /proc/meminfo");
> > > +             return -1;
> > > +     }
> > > +     while (fgets(buffer, sizeof(buffer), file)) {
> > > +             if (strncmp(buffer, str, str_len) == 0) {
> > > +                     sscanf(buffer + str_len, "%u", &hugepage_size);
> > > +                     break;
> > > +             }
> > > +     }
> > > +     fclose(file);
> > > +     if (!hugepage_size) {
> > > +             prterr("get_hugepage_size: failed to find "
> > > +                     "hugepage size in /proc/meminfo\n");
> > > +             return -1;
> > > +     }
> > > +
> > > +     /* convert from KiB to bytes */
> > > +     return hugepage_size << 10;
> > > +}
> > > +
> > > +static void *
> > > +init_hugepages_buf(unsigned len, int hugepage_size, int alignment, long *buf_size)
> > > +{
> > > +     void *buf = NULL;
> > > +#ifdef MADV_COLLAPSE
> > > +     int ret;
> > > +     long size = roundup(len, hugepage_size) + alignment;
> > > +
> > > +     ret = posix_memalign(&buf, hugepage_size, size);
> > > +     if (ret) {
> > > +             prterr("posix_memalign for buf");
> > > +             return NULL;
> > > +     }
> > > +     memset(buf, '\0', size);
> > > +     ret = madvise(buf, size, MADV_COLLAPSE);
> > > +     if (ret) {
> > > +             prterr("madvise collapse for buf");
> > > +             free(buf);
> > > +             return NULL;
> > > +     }
> > > +
> > > +     *buf_size = size;
> > > +#endif
> > > +     return buf;
> > > +}
> > > +
> > > +static void
> > > +init_buffers(void)
> > > +{
> > > +     int i;
> > > +
> > > +     original_buf = (char *) malloc(maxfilelen);
> > > +     for (i = 0; i < maxfilelen; i++)
> > > +             original_buf[i] = random() % 256;
> > > +     if (hugepages) {
> > > +             long hugepage_size = get_hugepage_size();
> > > +             if (hugepage_size == -1) {
> > > +                     prterr("get_hugepage_size()");
> > > +                     exit(102);
> > > +             }
> > > +             good_buf = init_hugepages_buf(maxfilelen, hugepage_size, writebdy,
> > > +                                           &hugepages_info.good_buf_size);
> > > +             if (!good_buf) {
> > > +                     prterr("init_hugepages_buf failed for good_buf");
> > > +                     exit(103);
> > > +             }
> > > +             hugepages_info.orig_good_buf = good_buf;
> > > +
> > > +             temp_buf = init_hugepages_buf(maxoplen, hugepage_size, readbdy,
> > > +                                           &hugepages_info.temp_buf_size);
> > > +             if (!temp_buf) {
> > > +                     prterr("init_hugepages_buf failed for temp_buf");
> > > +                     exit(103);
> > > +             }
> > > +             hugepages_info.orig_temp_buf = temp_buf;
> > > +     } else {
> > > +             unsigned long good_buf_len = maxfilelen + writebdy;
> > > +             unsigned long temp_buf_len = maxoplen + readbdy;
> > > +
> > > +             good_buf = calloc(1, good_buf_len);
> > > +             temp_buf = calloc(1, temp_buf_len);
> > > +     }
> > > +     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > +     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > +}
> > > +
> > >  static struct option longopts[] = {
> > >       {"replay-ops", required_argument, 0, 256},
> > >       {"record-ops", optional_argument, 0, 255},
> > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > >
> > >       while ((ch = getopt_long(argc, argv,
> > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > >                                longopts, NULL)) != EOF)
> > >               switch (ch) {
> > >               case 'b':
> > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > >               case 'g':
> > >                       filldata = *optarg;
> > >                       break;
> > > +             case 'h':
> > > +#ifndef MADV_COLLAPSE
> > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > +                             "Can't support -h\n");
> > > +                     exit(86);
> > > +#endif
> > > +                     hugepages = 1;
> > > +                     break;
> >
> > ...
> > if we could change this part to:
> >
> > #ifdef MADV_COLLAPSE
> >         hugepages = 1;
> > #endif
> >         break;
> >
> > to avoid the test failures on old systems.
> >
> > Or any better ideas from you :)
> 
> Hi Zorro,
> 
> It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> do you think about skipping generic/758 and generic/759 if the kernel
> version is older than 6.1? That to me seems more preferable than the
> paste above, as the paste above would execute the test as if it did
> test hugepages when in reality it didn't, which would be misleading.

Hi Joanne,

Sorry I'm still on my holiday, reply a bit late and hastily. At first,
your patch has been merged, the merged case numbers are g/759 and g/760,
you can rebase to latest for-next branch and write new fix patch :)

Then, you're right, above code change is bit rough;) Maybe there's a way
to _notrun if MADV_COLLAPSE isn't supported?

Thanks,
Zorro

> 
> 
> Thanks,
> Joanne
> 
> >
> > Thanks,
> > Zorro
> >
> > >               case 'i':
> > >                       integrity = 1;
> > >                       logdev = strdup(optarg);
> > > @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
> > >                       exit(95);
> > >               }
> > >       }
> > > -     original_buf = (char *) malloc(maxfilelen);
> > > -     for (i = 0; i < maxfilelen; i++)
> > > -             original_buf[i] = random() % 256;
> > > -     good_buf = (char *) malloc(maxfilelen + writebdy);
> > > -     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > -     memset(good_buf, '\0', maxfilelen);
> > > -     temp_buf = (char *) malloc(maxoplen + readbdy);
> > > -     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > -     memset(temp_buf, '\0', maxoplen);
> > > +     init_buffers();
> > >       if (lite) {     /* zero entire existing file */
> > >               ssize_t written;
> > >
> > > --
> > > 2.47.1
> > >
> >
> 

Re: [PATCH v5 1/2] fsx: support reads/writes from buffers backed by hugepages

[Joanne Koong]

On Tue, Feb 4, 2025 at 9:05 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Mon, Feb 03, 2025 at 10:04:04AM -0800, Joanne Koong wrote:
> > On Sun, Feb 2, 2025 at 6:25 AM Zorro Lang <zlang@redhat.com> wrote:
> > >
> > > On Tue, Jan 21, 2025 at 01:56:40PM -0800, Joanne Koong wrote:
> > > > Add support for reads/writes from buffers backed by hugepages.
> > > > This can be enabled through the '-h' flag. This flag should only be used
> > > > on systems where THP capabilities are enabled.
> > > >
> > > > This is motivated by a recent bug that was due to faulty handling of
> > > > userspace buffers backed by hugepages. This patch is a mitigation
> > > > against problems like this in the future.
> > > >
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > >
> > > Those two test cases fail on old system which doesn't support
> > > MADV_COLLAPSE:
> > >
> > >    fsx -N 10000 -l 500000 -h
> > >   -fsx -N 10000 -o 8192 -l 500000 -h
> > >   -fsx -N 10000 -o 128000 -l 500000 -h
> > >   +MADV_COLLAPSE not supported. Can't support -h
> > >
> > > and
> > >
> > >    fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   -fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   -fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
> > >   +mapped writes DISABLED
> > >   +MADV_COLLAPSE not supported. Can't support -h
> > >
> > > I'm wondering ...
> > >
> > > >  ltp/fsx.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 153 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > > > index 41933354..3be383c6 100644
> > > > --- a/ltp/fsx.c
> > > > +++ b/ltp/fsx.c
> > > > @@ -190,6 +190,16 @@ int      o_direct;                       /* -Z */
> > > > +
> > > >  static struct option longopts[] = {
> > > >       {"replay-ops", required_argument, 0, 256},
> > > >       {"record-ops", optional_argument, 0, 255},
> > > > @@ -2883,7 +3023,7 @@ main(int argc, char **argv)
> > > >       setvbuf(stdout, (char *)0, _IOLBF, 0); /* line buffered stdout */
> > > >
> > > >       while ((ch = getopt_long(argc, argv,
> > > > -                              "0b:c:de:fg:i:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > > +                              "0b:c:de:fg:hi:j:kl:m:no:p:qr:s:t:uw:xyABD:EFJKHzCILN:OP:RS:UWXZ",
> > > >                                longopts, NULL)) != EOF)
> > > >               switch (ch) {
> > > >               case 'b':
> > > > @@ -2916,6 +3056,14 @@ main(int argc, char **argv)
> > > >               case 'g':
> > > >                       filldata = *optarg;
> > > >                       break;
> > > > +             case 'h':
> > > > +#ifndef MADV_COLLAPSE
> > > > +                     fprintf(stderr, "MADV_COLLAPSE not supported. "
> > > > +                             "Can't support -h\n");
> > > > +                     exit(86);
> > > > +#endif
> > > > +                     hugepages = 1;
> > > > +                     break;
> > >
> > > ...
> > > if we could change this part to:
> > >
> > > #ifdef MADV_COLLAPSE
> > >         hugepages = 1;
> > > #endif
> > >         break;
> > >
> > > to avoid the test failures on old systems.
> > >
> > > Or any better ideas from you :)
> >
> > Hi Zorro,
> >
> > It looks like MADV_COLLAPSE was introduced in kernel version 6.1. What
> > do you think about skipping generic/758 and generic/759 if the kernel
> > version is older than 6.1? That to me seems more preferable than the
> > paste above, as the paste above would execute the test as if it did
> > test hugepages when in reality it didn't, which would be misleading.
>
> Hi Joanne,
>
> Sorry I'm still on my holiday, reply a bit late and hastily. At first,
> your patch has been merged, the merged case numbers are g/759 and g/760,
> you can rebase to latest for-next branch and write new fix patch :)

Hi Zorro,

No worries at all. Thanks for your work on this.

>
> Then, you're right, above code change is bit rough;) Maybe there's a way
> to _notrun if MADV_COLLAPSE isn't supported?

I'll (or Darrick if we go with his solution) make sure to submit a fix
for this so that it doesn't break the older systems.

Thanks,
Joanne

>
> Thanks,
> Zorro
>
> >
> >
> > Thanks,
> > Joanne
> >
> > >
> > > Thanks,
> > > Zorro
> > >
> > > >               case 'i':
> > > >                       integrity = 1;
> > > >                       logdev = strdup(optarg);
> > > > @@ -3229,15 +3377,7 @@ main(int argc, char **argv)
> > > >                       exit(95);
> > > >               }
> > > >       }
> > > > -     original_buf = (char *) malloc(maxfilelen);
> > > > -     for (i = 0; i < maxfilelen; i++)
> > > > -             original_buf[i] = random() % 256;
> > > > -     good_buf = (char *) malloc(maxfilelen + writebdy);
> > > > -     good_buf = round_ptr_up(good_buf, writebdy, 0);
> > > > -     memset(good_buf, '\0', maxfilelen);
> > > > -     temp_buf = (char *) malloc(maxoplen + readbdy);
> > > > -     temp_buf = round_ptr_up(temp_buf, readbdy, 0);
> > > > -     memset(temp_buf, '\0', maxoplen);
> > > > +     init_buffers();
> > > >       if (lite) {     /* zero entire existing file */
> > > >               ssize_t written;
> > > >
> > > > --
> > > > 2.47.1
> > > >
> > >
> >
>

[PATCH v5 2/2] generic: add tests for read/writes from hugepages-backed buffers

[Joanne Koong]

Add generic tests 758 and 759 for testing reads/writes from buffers
backed by hugepages.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 common/rc             | 13 +++++++++++++
 tests/generic/758     | 22 ++++++++++++++++++++++
 tests/generic/758.out |  4 ++++
 tests/generic/759     | 26 ++++++++++++++++++++++++++
 tests/generic/759.out |  4 ++++
 5 files changed, 69 insertions(+)
 create mode 100755 tests/generic/758
 create mode 100644 tests/generic/758.out
 create mode 100755 tests/generic/759
 create mode 100644 tests/generic/759.out

diff --git a/common/rc b/common/rc
index 1b2e4508..0c44d096 100644
--- a/common/rc
+++ b/common/rc
@@ -3016,6 +3016,19 @@ _require_xfs_io_command()
 	fi
 }
 
+# check that the system supports transparent hugepages
+_require_thp()
+{
+	if [ ! -e /sys/kernel/mm/transparent_hugepage/enabled ]; then
+		_notrun "system doesn't support transparent hugepages"
+	fi
+
+	thp_status=$(cat /sys/kernel/mm/transparent_hugepage/enabled)
+	if [[ $thp_status == *"[never]"* ]]; then
+		_notrun "system doesn't have transparent hugepages enabled"
+	fi
+}
+
 # check that kernel and filesystem support direct I/O, and check if "$1" size
 # aligned (optional) is supported
 _require_odirect()
diff --git a/tests/generic/758 b/tests/generic/758
new file mode 100755
index 00000000..e7cd8cdc
--- /dev/null
+++ b/tests/generic/758
@@ -0,0 +1,22 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# FS QA Test No. 758
+#
+# fsx exercising reads/writes from userspace buffers
+# backed by hugepages
+#
+. ./common/preamble
+_begin_fstest rw auto quick
+
+. ./common/filter
+
+_require_test
+_require_thp
+
+run_fsx -N 10000            -l 500000 -h
+run_fsx -N 10000  -o 8192   -l 500000 -h
+run_fsx -N 10000  -o 128000 -l 500000 -h
+
+status=0
+exit
diff --git a/tests/generic/758.out b/tests/generic/758.out
new file mode 100644
index 00000000..af04bb14
--- /dev/null
+++ b/tests/generic/758.out
@@ -0,0 +1,4 @@
+QA output created by 758
+fsx -N 10000 -l 500000 -h
+fsx -N 10000 -o 8192 -l 500000 -h
+fsx -N 10000 -o 128000 -l 500000 -h
diff --git a/tests/generic/759 b/tests/generic/759
new file mode 100755
index 00000000..514e7603
--- /dev/null
+++ b/tests/generic/759
@@ -0,0 +1,26 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# FS QA Test No. 759
+#
+# fsx exercising direct IO reads/writes from userspace buffers
+# backed by hugepages
+#
+. ./common/preamble
+_begin_fstest rw auto quick
+
+. ./common/filter
+
+_require_test
+_require_odirect
+_require_thp
+
+psize=`$here/src/feature -s`
+bsize=`$here/src/min_dio_alignment $TEST_DIR $TEST_DEV`
+
+run_fsx -N 10000            -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
+run_fsx -N 10000  -o 8192   -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
+run_fsx -N 10000  -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
+
+status=0
+exit
diff --git a/tests/generic/759.out b/tests/generic/759.out
new file mode 100644
index 00000000..86bb66ef
--- /dev/null
+++ b/tests/generic/759.out
@@ -0,0 +1,4 @@
+QA output created by 759
+fsx -N 10000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
+fsx -N 10000 -o 8192 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
+fsx -N 10000 -o 128000 -l 500000 -r PSIZE -t BSIZE -w BSIZE -Z -R -W -h
-- 
2.47.1