Re: [PATCH v3 2/9] mm: limit pagecache folio refcount when splicing into pipes

[Pedro Falcato <pfalcato@suse.de>]

On Thu, Nov 12, 2026 at 02:24:09PM +0100, David Hildenbrand (Arm) wrote:
> On 11/12/26 14:09, Pedro Falcato wrote:
> > On Tue, Nov 10, 2026 at 09:35:42PM +0100, David Hildenbrand (Arm) wrote:
> >> Similar to mapping pagecache folios into page tables, let's reject
> >> additional references when splicing folios into a pipe if the refcount
> >> is at its limit, similar to how we handle it during GUP.
> >>
> >> This will prevent more cases of possible folio refcount overflows with
> >> large pagecache folios.
> >>
> >> Introduce folio_get_checked() for checked reference acquisition when
> >> the caller already holds a reference. It warns and fails on a non-positive
> >> refcount and rejects additional references at the limit.
> >>
> >> Use folio_get_checked() in splice_folio_into_pipe() and
> >> fuse_ref_folio(), returning -EFAULT on failure, like we already do for
> >> pipe-buffer duplication. Similarly, make try_get_page() use it instead.
> >>
> >> Likely try_get_page() should be renamed (or removed) later, as it doesn't
> >> match the folio_try_get() semantics and is confusing.
> >>
> >> If we hit the limit after splicing some data, return the number of bytes
> >> already spliced. Teach the callers to handle errors, and stop
> >> filemap_splice_read() if we only spliced part of a folio, so we don't
> >> skip ahead to the next folio.
> >>
> >> The check in try_get_page() also covers pipe-buffer duplication by tee()
> >> and partial pipe-to-pipe splicing.
> >>
> >> Note that we deliberately will not fall back to the slow path of copying
> >> the folio, particularly as the folio might have an invalid refcount. If
> >> it is ever really required, we can revisit.
> >>
> >> Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
> >> Fixes: 99cb0dbd47a1 ("mm,thp: add read-only THP support for (non-shmem) FS")
> >> Fixes: 793917d997df ("mm/readahead: Add large folio readahead")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: David Hildenbrand (Arm) <david@kernel.org>
> >> ---
> 
> [...]
> >> index 0f6d57432a7e5..e6e73e37f3ea4 100644
> >> --- a/mm/filemap.c
> >> +++ b/mm/filemap.c
> >> @@ -3047,10 +3047,11 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> >>  EXPORT_SYMBOL(generic_file_read_iter);
> >>  
> >>  /*
> >> - * Splice subpages from a folio into a pipe.
> >> + * Splice subpages from a folio into a pipe. Return the number of bytes spliced,
> >> + * or -EFAULT if we cannot splice from the folio.
> > 
> > -EFAULT is a weird error code to return from splice, et al.
> 
> We might be able to use ETOOMANYREFS (have to check if that could confuse
> callers), but IMHO EFAULT is good enough.
> 
> What would be your suggestion?

ETOOMANYREFS I think is good, also EBUSY, EMLINK (though this would be weird)
or even a generic -EIO.

> 
> > 
> >>   */
> >> -size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
> >> -			      struct folio *folio, loff_t fpos, size_t size)
> >> +ssize_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
> >> +			       struct folio *folio, loff_t fpos, size_t size)
> >>  {
> >>  	struct page *page;
> >>  	size_t spliced = 0, offset = offset_in_folio(folio, fpos);
> >> @@ -3063,13 +3064,15 @@ size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
> >>  		struct pipe_buffer *buf = pipe_head_buf(pipe);
> >>  		size_t part = min_t(size_t, PAGE_SIZE - offset, size - spliced);
> >>  
> >> +		if (!folio_get_checked(folio))
> >> +			return spliced ? spliced : -EFAULT;
> > 			return spliced ?: -EFAULT;
> > 
> > I don't know if there's much kernel code out there doing this, but I like
> > the trick :)
> 
> There are a couple of thousands, but yes, one doesn't encounter it that often.
> 
> > 
> >> +
> >>  		*buf = (struct pipe_buffer) {
> >>  			.ops	= &page_cache_pipe_buf_ops,
> >>  			.page	= page,
> >>  			.offset	= offset,
> >>  			.len	= part,
> >>  		};
> >> -		folio_get(folio);
> >>  		pipe->head++;
> >>  		page++;
> >>  		spliced += part;
> >> @@ -3094,9 +3097,10 @@ size_t splice_folio_into_pipe(struct pipe_inode_info *pipe,
> >>   * Return: On success, the number of bytes read will be returned and *@ppos
> >>   * will be updated if appropriate; 0 will be returned if there is no more data
> >>   * to be read; -EAGAIN will be returned if the pipe had no space, and some
> >> - * other negative error code will be returned on error.  A short read may occur
> >> - * if the pipe has insufficient space, we reach the end of the data or we hit a
> >> - * hole.
> >> + * other negative error code will be returned on error; -EFAULT will be returned
> >> + * if acquiring a folio reference fails and no data has been spliced.
> >> + * A short read may occur if the pipe has insufficient space, we reach the
> >> + * end of the data, hit a hole or hit a folio refcount limit.
> >>   */
> >>  ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
> >>  			    struct pipe_inode_info *pipe,
> >> @@ -3154,7 +3158,7 @@ ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
> >>  
> >>  		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >>  			struct folio *folio = fbatch.folios[i];
> >> -			size_t n;
> >> +			ssize_t n;
> >>  
> >>  			if (folio_pos(folio) >= end_offset)
> >>  				goto out;
> >> @@ -3170,13 +3174,15 @@ ssize_t filemap_splice_read(struct file *in, loff_t *ppos,
> >>  
> >>  			n = min_t(loff_t, len, isize - *ppos);
> >>  			n = splice_folio_into_pipe(pipe, folio, *ppos, n);
> >> -			if (!n)
> >> +			if (n <= 0) {
> >> +				error = n;
> >>  				goto out;
> >> +			}
> >>  			len -= n;
> >>  			total_spliced += n;
> >>  			*ppos += n;
> >>  			in->f_ra.prev_pos = *ppos;
> >> -			if (pipe_is_full(pipe))
> >> +			if (pipe_is_full(pipe) || *ppos < folio_next_pos(folio))
> > 
> > I'm confused. Why does this need to be done here?
> >  
> 
> I have above:
> 
> "stop filemap_splice_read() if we only spliced part of a folio, so we don't
> skip ahead to the next folio"

Yes, but is this a separate fix? A folio could be partially spliced previously
as well, no?

> 
> Thanks a lot for the review!

You're welcome! :)

FWIW, you can add my

Reviewed-by: Pedro Falcato <pfalcato@suse.de>

to whatever comes out of these nits :)

-- 
Pedro