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 8:41 PM, Zi Yan wrote:
> On Sat Sep 5, 2026 at 5:15 AM EDT, Zhang Yi wrote:
>> 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?
>>
> 
> Understood.
> 
> Alternatives are changing folio_split() to make sure folio2 is split.
>  From weakest guarantee to strongest guarantee:
> 
> 1. make folio_split() return with folio2 locked: but others can still
> put a ref on folio2 to fail the subsequent folio2 split.
> 
> 2. make folio_split() accept two split_at, folio_split() does both folio
> and folio2 split internally: but if folio2 split require an xa_node and
> the allocation fails, folio2 split can still fail.
> 
> 3. make folio_split() accept two split_at and preallocate two xa_node
> upfront: this should guarantee folio_split() to either split both folio
> and folio2 or split nothing, but it specializes folio_split() for
> truncate.
> 

I'm afraid even guaranteeing a successful folio2 split is not enough
here. As Joanne pointed out [1], when min_order is not 0, even if folio2
is successfully split and returns true, the folio remains a large folio.
In the outer loop, 'end' still cannot point to the head of the split
folio2, so the entire folio is still incorrectly dropped as a whole,
losing the valid data at the tail.

[1] 
https://lore.kernel.org/linux-mm/CAJnrk1bQYUe6+1ryyJur5EEnZYrC+_5AYsy=OWzVRgD4202y1g@mail.gmail.com/

> I guess for now it might be better to make truncate to handle the
> folio2-not-split situation.
> 

Yeah, agree.

Thanks,
Yi.