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

["David Hildenbrand (Arm)" <david@kernel.org>]

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>
>> ---

[...]

>> +static inline __must_check bool folio_get_checked(struct folio *folio)
>>  {
>> -	page = compound_head(page);
>> -	if (WARN_ON_ONCE(page_ref_count(page) <= 0))
>> +	if (WARN_ON_ONCE(folio_ref_count(folio) <= 0))
>>  		return false;
>> -	page_ref_inc(page);
>> +	if (unlikely(folio_ref_count_over_limit(folio)))
>> +		return false;
> 
> Ah nice, the helper I wanted :)
> 
> Nit: Having these two functions called back-to-back will result in two
> separate loads on the refcount (folio_ref_count() distills into a READ_ONCE).

Ack, something to optimize. I'd likely leave that for future work, as we'd need
yet another helper.

Unless I can convince myself that it is easy to do.

> 
>> +	folio_ref_inc(folio);
>>  	return true;
>>  }
>>  
>> +static inline __must_check bool try_get_page(struct page *page)
>> +{
>> +	return folio_get_checked(page_folio(page));
>> +
> 
> God I hate this. I never realized try_get_page() isn't atomic, but
> folio_try_get() is.

Yeah, but that's for another purpose and more expensive.

> 
>> +
>>  /**
>>   * folio_put - Decrement the reference count on a folio.
>>   * @folio: The folio.
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> 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?

> 
>>   */
>> -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"

Thanks a lot for the review!

-- 
Cheers,

David