* [PATCH] selftests/mm: check file initialization writes in split_huge_page_test @ 2026-05-12 7:49 Vineet Agarwal 2026-05-12 7:57 ` David Hildenbrand (Arm) 2026-05-15 9:22 ` Wei Yang 0 siblings, 2 replies; 7+ messages in thread From: Vineet Agarwal @ 2026-05-12 7:49 UTC (permalink / raw) To: akpm; +Cc: david, ljs, linux-mm, linux-kselftest, linux-kernel, Vineet Agarwal 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-12 7:49 [PATCH] selftests/mm: check file initialization writes in split_huge_page_test Vineet Agarwal @ 2026-05-12 7:57 ` David Hildenbrand (Arm) 2026-05-15 9:22 ` Wei Yang 1 sibling, 0 replies; 7+ messages in thread From: David Hildenbrand (Arm) @ 2026-05-12 7:57 UTC (permalink / raw) To: Vineet Agarwal, akpm; +Cc: ljs, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-12 7:49 [PATCH] selftests/mm: check file initialization writes in split_huge_page_test Vineet Agarwal 2026-05-12 7:57 ` David Hildenbrand (Arm) @ 2026-05-15 9:22 ` Wei Yang 2026-05-18 23:35 ` Andrew Morton 1 sibling, 1 reply; 7+ messages in thread From: Wei Yang @ 2026-05-15 9:22 UTC (permalink / raw) To: Vineet Agarwal; +Cc: akpm, david, ljs, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-15 9:22 ` Wei Yang @ 2026-05-18 23:35 ` Andrew Morton 2026-05-19 2:51 ` Wei Yang 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2026-05-18 23:35 UTC (permalink / raw) To: Wei Yang Cc: Vineet Agarwal, david, ljs, linux-mm, linux-kselftest, linux-kernel 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. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-18 23:35 ` Andrew Morton @ 2026-05-19 2:51 ` Wei Yang 2026-05-19 19:18 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: Wei Yang @ 2026-05-19 2:51 UTC (permalink / raw) To: Andrew Morton Cc: Wei Yang, Vineet Agarwal, david, ljs, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-19 2:51 ` Wei Yang @ 2026-05-19 19:18 ` Andrew Morton 2026-05-20 1:49 ` Wei Yang 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2026-05-19 19:18 UTC (permalink / raw) To: Wei Yang Cc: Vineet Agarwal, david, ljs, linux-mm, linux-kselftest, linux-kernel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] selftests/mm: check file initialization writes in split_huge_page_test 2026-05-19 19:18 ` Andrew Morton @ 2026-05-20 1:49 ` Wei Yang 0 siblings, 0 replies; 7+ messages in thread From: Wei Yang @ 2026-05-20 1:49 UTC (permalink / raw) To: Andrew Morton Cc: Wei Yang, Vineet Agarwal, david, ljs, linux-mm, linux-kselftest, linux-kernel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-05-20 1:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-12 7:49 [PATCH] selftests/mm: check file initialization writes in split_huge_page_test Vineet Agarwal 2026-05-12 7:57 ` David Hildenbrand (Arm) 2026-05-15 9:22 ` Wei Yang 2026-05-18 23:35 ` Andrew Morton 2026-05-19 2:51 ` Wei Yang 2026-05-19 19:18 ` Andrew Morton 2026-05-20 1:49 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox