selftests/mm: check file initialization writes in split_huge_page_test

[PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Vineet Agarwal 1 month ago

create_pagecache_thp_and_fd() fills the backing file for the
pagecache THP tests using repeated write() calls, but the return
value is never checked.

If a write fails or completes only partially, the test may continue
with an incompletely initialized file and produce misleading results.

Check the result of write() and fail the test if the expected number
of bytes was not written.

Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
---
 tools/testing/selftests/mm/split_huge_page_test.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
index 500d07c4938b..eab69b0f59a0 100644
--- a/tools/testing/selftests/mm/split_huge_page_test.c
+++ b/tools/testing/selftests/mm/split_huge_page_test.c
@@ -609,9 +609,16 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
 	assert(fd_size % sizeof(buf) == 0);
 	for (i = 0; i < sizeof(buf); i++)
 		buf[i] = (unsigned char)i;
-	for (i = 0; i < fd_size; i += sizeof(buf))
-		write(*fd, buf, sizeof(buf));
-
+	for (i = 0; i < fd_size; i += sizeof(buf)) {
+		ssize_t written;
+
+		written = write(*fd, buf, sizeof(buf));
+		if (written != sizeof(buf)) {
+			ksft_perror("write testfile");
+			close(*fd);
+			goto err_out_unlink;
+		}
+	}
 	close(*fd);
 	sync();
 	*fd = open("/proc/sys/vm/drop_caches", O_WRONLY);
-- 
2.54.0

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Wei Yang 4 weeks ago

On Tue, May 12, 2026 at 01:19:24PM +0530, Vineet Agarwal wrote:
>create_pagecache_thp_and_fd() fills the backing file for the
>pagecache THP tests using repeated write() calls, but the return
>value is never checked.
>
>If a write fails or completes only partially, the test may continue
>with an incompletely initialized file and produce misleading results.
>
>Check the result of write() and fail the test if the expected number
>of bytes was not written.
>
>Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
>---
> tools/testing/selftests/mm/split_huge_page_test.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
>diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>index 500d07c4938b..eab69b0f59a0 100644
>--- a/tools/testing/selftests/mm/split_huge_page_test.c
>+++ b/tools/testing/selftests/mm/split_huge_page_test.c
>@@ -609,9 +609,16 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
> 	assert(fd_size % sizeof(buf) == 0);
> 	for (i = 0; i < sizeof(buf); i++)
> 		buf[i] = (unsigned char)i;
>-	for (i = 0; i < fd_size; i += sizeof(buf))
>-		write(*fd, buf, sizeof(buf));
>-
>+	for (i = 0; i < fd_size; i += sizeof(buf)) {
>+		ssize_t written;
>+
>+		written = write(*fd, buf, sizeof(buf));
>+		if (written != sizeof(buf)) {
>+			ksft_perror("write testfile");
>+			close(*fd);
>+			goto err_out_unlink;

Maybe we can "goto err_out_close" and remove the close() here?

Apart from this, I found on error writing to /proc/sys/vm/drop_caches in below,
it just goto err_out_unlink, which left fd open.

How about fix that at the same time.

Generally, the change look good.

>+		}
>+	}
> 	close(*fd);
> 	sync();
> 	*fd = open("/proc/sys/vm/drop_caches", O_WRONLY);
>-- 
>2.54.0
>

-- 
Wei Yang
Help you, Help me

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Andrew Morton 3 weeks, 3 days ago

On Fri, 15 May 2026 09:22:11 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> On Tue, May 12, 2026 at 01:19:24PM +0530, Vineet Agarwal wrote:
> >create_pagecache_thp_and_fd() fills the backing file for the
> >pagecache THP tests using repeated write() calls, but the return
> >value is never checked.
> >
> >If a write fails or completes only partially, the test may continue
> >with an incompletely initialized file and produce misleading results.
> >
> >Check the result of write() and fail the test if the expected number
> >of bytes was not written.
> >
> >Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
> >---
> > tools/testing/selftests/mm/split_huge_page_test.c | 13 ++++++++++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> >diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
> >index 500d07c4938b..eab69b0f59a0 100644
> >--- a/tools/testing/selftests/mm/split_huge_page_test.c
> >+++ b/tools/testing/selftests/mm/split_huge_page_test.c
> >@@ -609,9 +609,16 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
> > 	assert(fd_size % sizeof(buf) == 0);
> > 	for (i = 0; i < sizeof(buf); i++)
> > 		buf[i] = (unsigned char)i;
> >-	for (i = 0; i < fd_size; i += sizeof(buf))
> >-		write(*fd, buf, sizeof(buf));
> >-
> >+	for (i = 0; i < fd_size; i += sizeof(buf)) {
> >+		ssize_t written;
> >+
> >+		written = write(*fd, buf, sizeof(buf));
> >+		if (written != sizeof(buf)) {
> >+			ksft_perror("write testfile");
> >+			close(*fd);
> >+			goto err_out_unlink;
> 
> Maybe we can "goto err_out_close" and remove the close() here?

AI review suggested the same:
	https://sashiko.dev/#/patchset/20260512074924.27721-1-agarwal.vineet2006%40gmail.com

And it complained about the handling of short writes in the added code.

> Apart from this, I found on error writing to /proc/sys/vm/drop_caches in below,
> it just goto err_out_unlink, which left fd open.

Not understanding - cab you please describe more completely?

> How about fix that at the same time.
> 
> Generally, the change look good.
>

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Wei Yang 3 weeks, 3 days ago

On Mon, May 18, 2026 at 04:35:53PM -0700, Andrew Morton wrote:
>On Fri, 15 May 2026 09:22:11 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Tue, May 12, 2026 at 01:19:24PM +0530, Vineet Agarwal wrote:
>> >create_pagecache_thp_and_fd() fills the backing file for the
>> >pagecache THP tests using repeated write() calls, but the return
>> >value is never checked.
>> >
>> >If a write fails or completes only partially, the test may continue
>> >with an incompletely initialized file and produce misleading results.
>> >
>> >Check the result of write() and fail the test if the expected number
>> >of bytes was not written.
>> >
>> >Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
>> >---
>> > tools/testing/selftests/mm/split_huge_page_test.c | 13 ++++++++++---
>> > 1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>> >index 500d07c4938b..eab69b0f59a0 100644
>> >--- a/tools/testing/selftests/mm/split_huge_page_test.c
>> >+++ b/tools/testing/selftests/mm/split_huge_page_test.c
>> >@@ -609,9 +609,16 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
>> > 	assert(fd_size % sizeof(buf) == 0);
>> > 	for (i = 0; i < sizeof(buf); i++)
>> > 		buf[i] = (unsigned char)i;
>> >-	for (i = 0; i < fd_size; i += sizeof(buf))
>> >-		write(*fd, buf, sizeof(buf));
>> >-
>> >+	for (i = 0; i < fd_size; i += sizeof(buf)) {
>> >+		ssize_t written;
>> >+
>> >+		written = write(*fd, buf, sizeof(buf));
>> >+		if (written != sizeof(buf)) {
>> >+			ksft_perror("write testfile");
>> >+			close(*fd);
>> >+			goto err_out_unlink;
>> 
>> Maybe we can "goto err_out_close" and remove the close() here?
>
>AI review suggested the same:
>	https://sashiko.dev/#/patchset/20260512074924.27721-1-agarwal.vineet2006%40gmail.com
>
>And it complained about the handling of short writes in the added code.
>
>> Apart from this, I found on error writing to /proc/sys/vm/drop_caches in below,
>> it just goto err_out_unlink, which left fd open.
>
>Not understanding - cab you please describe more completely?
>

I mean do this below.

diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
index 460fa1f606fd..f30402ece608 100644
--- a/tools/testing/selftests/mm/split_huge_page_test.c
+++ b/tools/testing/selftests/mm/split_huge_page_test.c
@@ -469,7 +469,7 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
        }
        if (write(*fd, "3", 1) != 1) {
                ksft_perror("write to drop_caches");
-               goto err_out_unlink;
+               goto err_out_close;
        }
        close(*fd);

On error writing this fd, /proc/sys/vm/drop_caches, we forget to close it.

>> How about fix that at the same time.
>> 
>> Generally, the change look good.
>> 

-- 
Wei Yang
Help you, Help me

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Andrew Morton 3 weeks, 3 days ago

On Tue, 19 May 2026 02:51:41 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:

> >Not understanding - cab you please describe more completely?
> >
> 
> I mean do this below.
> 
> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
> index 460fa1f606fd..f30402ece608 100644
> --- a/tools/testing/selftests/mm/split_huge_page_test.c
> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
> @@ -469,7 +469,7 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
>         }
>         if (write(*fd, "3", 1) != 1) {
>                 ksft_perror("write to drop_caches");
> -               goto err_out_unlink;
> +               goto err_out_close;
>         }
>         close(*fd);
> 
> On error writing this fd, /proc/sys/vm/drop_caches, we forget to close it.

Oh, OK, that's a buglet.  Please send a formal fix sometime?

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by Wei Yang 3 weeks, 2 days ago

On Tue, May 19, 2026 at 12:18:13PM -0700, Andrew Morton wrote:
>On Tue, 19 May 2026 02:51:41 +0000 Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> >Not understanding - cab you please describe more completely?
>> >
>> 
>> I mean do this below.
>> 
>> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>> index 460fa1f606fd..f30402ece608 100644
>> --- a/tools/testing/selftests/mm/split_huge_page_test.c
>> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
>> @@ -469,7 +469,7 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
>>         }
>>         if (write(*fd, "3", 1) != 1) {
>>                 ksft_perror("write to drop_caches");
>> -               goto err_out_unlink;
>> +               goto err_out_close;
>>         }
>>         close(*fd);
>> 
>> On error writing this fd, /proc/sys/vm/drop_caches, we forget to close it.
>
>Oh, OK, that's a buglet.  Please send a formal fix sometime?

Sure, I am glad to.

-- 
Wei Yang
Help you, Help me

Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test

Posted by David Hildenbrand (Arm) 1 month ago

On 5/12/26 09:49, Vineet Agarwal wrote:
> create_pagecache_thp_and_fd() fills the backing file for the
> pagecache THP tests using repeated write() calls, but the return
> value is never checked.
> 
> If a write fails or completes only partially, the test may continue
> with an incompletely initialized file and produce misleading results.
> 
> Check the result of write() and fail the test if the expected number
> of bytes was not written.
> 
> Signed-off-by: Vineet Agarwal <agarwal.vineet2006@gmail.com>
> ---
>  tools/testing/selftests/mm/split_huge_page_test.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
> index 500d07c4938b..eab69b0f59a0 100644
> --- a/tools/testing/selftests/mm/split_huge_page_test.c
> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
> @@ -609,9 +609,16 @@ static int create_pagecache_thp_and_fd(const char *testfile, size_t fd_size,
>  	assert(fd_size % sizeof(buf) == 0);
>  	for (i = 0; i < sizeof(buf); i++)
>  		buf[i] = (unsigned char)i;
> -	for (i = 0; i < fd_size; i += sizeof(buf))
> -		write(*fd, buf, sizeof(buf));
> -
> +	for (i = 0; i < fd_size; i += sizeof(buf)) {
> +		ssize_t written;

Do you really need the temporary variable?

> +
> +		written = write(*fd, buf, sizeof(buf));
> +		if (written != sizeof(buf)) {
> +			ksft_perror("write testfile");
> +			close(*fd);
> +			goto err_out_unlink;
> +		}
> +	}
>  	close(*fd);
>  	sync();
>  	*fd = open("/proc/sys/vm/drop_caches", O_WRONLY);

Apart from that LGTM

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David