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

["Zi Yan" <ziy@nvidia.com>]

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 guess for now it might be better to make truncate to handle the
folio2-not-split situation.

-- 
Best Regards,
Yan, Zi