Re: [RFC PATCH] mm/truncate: fix data loss when splitting fails in truncate_inode_partial_folio()

[Zhang Yi <yizhang089@gmail.com>]

On 9/5/2026 3:31 AM, Zi Yan wrote:
> On Thu Sep 3, 2026 at 7:50 AM EDT, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@huawei.com>
>>
>> truncate_inode_partial_folio() splits a large folio so that the caller's
>> truncate loop can drop the in-range sub-folios while keeping the
>> out-of-range tail. The first split at the punch start edge is
>> non-uniform, which leaves the sub-folio at the truncation end edge as
>> large as possible, this means it may still straddle the range, holding
>> both zeroed in-range and valid out-of-range data. The function then
>> attempts a second split at offset + length to isolate that tail.
>>
>> If the second split fails the straddling sub-folio stays merged. The
>> function returned true unconditionally on all exit paths of the success
>> block, telling the caller it was fully handled. The caller kept its
>> default end and the truncate loop truncated every sub-folio below it,
>> including the merged straddler, discarding the valid out-of-range tail.
>>
>> For example, a 4-page order-2 folio punched from offset 0 to the middle
>> of the last page:
>>
>>    truncate_inode_pages_range()
>>      truncate_inode_partial_folio()      # same_folio == true
>>        1st split at page0 -> [p0, p1, p2-3]   # non-uniform, success
>>        folio2 = p2-3 # straddles: p2 zeroed, p3 tail valid
>>        2nd split of folio2 fails / cannot lock
>>        return true                       # BUG: caller keeps default end
>>      end = 3
>>      loop truncates p0, p1, p2-3        # p3's valid tail is lost
>>
>> This became reachable after commit 7460b470a131 ("mm/truncate: use
>> folio_split() in truncate operation") replaced the atomic split_folio()
>> with folio_split(), whose non-uniform split can partially split a folio
>> and leave the end edge merged.
>>
>> It has gone unnoticed because a dirty large folio normally carries the
>> filesystem's private data, for example buffer_head, so
>> filemap_release_folio() -> iomap_release_folio() returns false on a
>> dirty folio and folio_split() aborts with -EBUSY before any split,
>> leaving the straddler safely unsplit. The bug is only reachable on paths
>> that produce dirty large folios without filesystem private data, and it
>> was caught on the upcoming ext4 iomap buffered I/O path when no ifs is
>> attached.
> 
> Thank you for the analysis.
> 
>>
>> Rework the contract so the caller is told where to stop instead of
>> silently truncating the straddler:
>>
>>    - Return true only when a split occurred, false otherwise. This
>>      clarifies the existing confusing return value semantics.
> 
> Should we do "return false" for not split case as a minmal fix first?
> 
> Something like below. A second patch can optimize on top of it. Let me
> know if I miss anything.
> 
> BTW, Claude also mentioned that if min_order > 0 and end is not aligned
> to 1UL << min_order, there could be some issue. So
> 
> ret = !folio_split_or_unmap(folio2, split_at2, min_order);
> 
> should be
> 
> unsigned long idx2 = PAGE_ALIGN_DOWN(offset + length) / PAGE_SIZE;
> 
> ret = !folio_split_or_unmap(folio2, split_at2, min_order) &&
> IS_ALIGNED(idx2, 1UL << min_order);
> 
> ?
> 

Hi Zi Yan,

Thanks for the minimal fix. I agree with the core observation — the
tail is only really isolated when the second split and the boundary
alignment both cooperate.  However, I'd like to point out a trade-off:
the "return false to make the caller skip the folio" mechanism leads
to an incorrect 'start' in truncate_inode_pages_range() and over-keeps
the in-range sub-folios.

The problem is that the false return value was designed for the case
where the folio is unsplit.  Look at the caller:

         if (!truncate_inode_partial_folio(folio, lstart, lend)) {
                 start = folio_next_index(folio);
                 if (same_folio)
                         end = folio->index;
         }

start = folio_next_index(folio); end = folio->index only makes sense
when folio is still the whole folio.  But after the first split
succeeds, the caller's folio reference has already been transferred to
the sub-folio containing split_at, so folio is no longer the whole
folio.

Concretely, take the commit's example: a 4-page order-2 folio
[p0 p1 p2 p3], punched from offset 0 into the middle of p3,
min_order == 0:

         [p0 p1 p2 p3]  --1st split @p0-->  [p0] [p1] [p2-p3]
         folio now points to [p0]
         folio2 = [p2-p3]              # p2 zeroed, p3 tail valid
         2nd split of [p2-p3] fails    # e.g. -EBUSY, or can't lock
		
With your fix, ret becomes false, so the caller runs:

         start = folio_next_index([p0]) = 1;
         end   = folio->index          = 0;

The truncate loop then does while (index < end) -> 1 < 0 -> nothing,
and [p0] and [p1] are left in the page cache, even though they are
fully inside the punched range and should have been dropped.

To be fair, this will not lead to any data-corruption problem because
p0 and p1 are already zeroed.  So as a minimal fix to stop the data
loss, it is acceptable.  But it still leaves the in-range sub-folios
behind and wastes memory, which somewhat reduces the benefit of
splitting the folio. So I don't think change the return value alone can
solve this problem.

What do you think?

Thanks,
Yi.

>  From 564fd753071be9d59d9e45e4609a812bb81f778a Mon Sep 17 00:00:00 2001
> From: Zi Yan <ziy@nvidia.com>
> Date: Fri, 4 Sep 2026 15:25:06 -0400
> Subject: [PATCH] fix unsuccessful folio2 split
> 
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
>   mm/truncate.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/truncate.c b/mm/truncate.c
> index b58ba940be474..2c575f5e61889 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
> @@ -259,6 +259,7 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
>   		 * for shmem truncate
>   		 */
>   		struct folio *folio2;
> +		bool ret = true;
>   
>   		if (offset + length == size)
>   			goto no_split;
> @@ -273,19 +274,23 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
>   		if (!folio_test_large(folio2))
>   			goto out;
>   
> -		if (!folio_trylock(folio2))
> +		if (!folio_trylock(folio2)) {
> +			ret = false;
>   			goto out;
> +		}
>   
>   		/* make sure folio2 is large and does not change its mapping */
>   		if (folio_test_large(folio2) &&
>   		    folio2->mapping == folio->mapping)
> -			folio_split_or_unmap(folio2, split_at2, min_order);
> +			ret = !folio_split_or_unmap(folio2, split_at2, min_order);
> +		else
> +			ret = false;
>   
>   		folio_unlock(folio2);
>   out:
>   		folio_put(folio2);
>   no_split:
> -		return true;
> +		return ret;
>   	}
>   	if (folio_test_dirty(folio))
>   		return false;