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

[Wei Yang <richard.weiyang@gmail.com>]

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