Re: [PATCH v4] fs/pipe: unify the page pools into a single per-pipe pool

[Breno Leitao <leitao@debian.org>]

On Fri, Jul 17, 2026 at 06:19:45PM +0200, Oleg Nesterov wrote:
> I personally like this change. To me it really makes this logic more
> simple/clear.
> 
> Reviewed-by: Oleg Nesterov <oleg@redhat.com>

Thanks for the review.

> > +static void anon_pipe_prefill_and_lock(struct pipe_inode_info *pipe, size_t total_len)
> >  {
> > -	unsigned int want, i;
> > -	struct page *page;
> > -
> > -	prealloc->count = 0;
> > -	if (total_len <= PAGE_SIZE)
> > -		return;
> > +	struct page *pages[PIPE_PREALLOC_MAX];
> > +	unsigned int want, have, need, n = 0;
> >
> >  	want = min_t(unsigned int, DIV_ROUND_UP(total_len, PAGE_SIZE),
> >  		     PIPE_PREALLOC_MAX);
> > +	/* Unlocked read; the pool is refilled under the lock below. */
> > +	have = min_t(unsigned int, READ_ONCE(pipe->prealloc.count), want);
> 
> And READ_ONCE() is enough correctness wise.
> 
> But AFAIK it is not enough to make KCSAN happy. anon_pipe_prealloc_pop/push
> which modify ->count under pipe->mutex need WRITE_ONCE() to please KCSAN.
> 
> Or anon_pipe_prefill_and_lock() can use data_race(READ_ONCE()).

I am not sure that is correct. I got the impression if the reading side
has READ_ONCE or data_race(), that is enough to please KCSAN.

I've also run a test with this code, and KCSAN doesn't show any complain
here.

The only complain is coming from osq_lock(), but not from here. This is
the osq_lock complain:

 BUG: KCSAN: data-race in osq_lock / osq_unlock

 write (marked) to 0xff11000527a42f80 of 8 bytes by task 1366 on cpu 23:
  osq_lock+0xf4/0x270
  __mutex_lock+0x1ab/0xe70
  mutex_lock_nested+0x17/0x20
  anon_pipe_read+0x69/0x570
  vfs_read+0x3e3/0x4b0
  ksys_read+0x95/0x120
  __x64_sys_read+0x3c/0x50
  x64_sys_call+0x69a/0x8d0
  do_syscall_64+0xfd/0x4f0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 read to 0xff11000527a42f80 of 8 bytes by task 1363 on cpu 20:
  osq_unlock+0x14c/0x1a0
  __mutex_lock+0x1dd/0xe70
  mutex_lock_nested+0x17/0x20
  anon_pipe_read+0x451/0x570
  vfs_read+0x3e3/0x4b0
  ksys_read+0x95/0x120
  __x64_sys_read+0x3c/0x50
  x64_sys_call+0x69a/0x8d0
  do_syscall_64+0xfd/0x4f0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53

 value changed: 0x0000000000000000 -> 0xff11000527bc2f80

that said, I think it makes sense to mark count as racy, given it is
read without a lock.

I will respin with, an additional "__data_racy" attribute in `count`

  struct anon_pipe_prealloc {
    struct page *pages[PIPE_PREALLOC_MAX];
    unsigned int __data_racy count;
  };

Thanks for the review,
--breno