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

[Joanne Koong]

On Thu, Sep 3, 2026 at 11:27 PM Zhang Yi <yi.zhang@xxxxxxxxxxxxxxx> wrote:
>
> On 9/4/2026 3:01 AM, Joanne Koong wrote:
> >>         /*
> >> @@ -259,6 +265,10 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
> >>                  * for shmem truncate
> >>                  */
> >>                 struct folio *folio2;
> >> +               bool tail_isolated = true;
> >> +
> >> +               if (end)
> >> +                       *end = (pos + offset + length) >> PAGE_SHIFT;
> >
> > If I'm understanding it correctly, based on how
> > truncate_inode_pages_range() uses the end value (eg the "while (index
> > < end)" loop condition and the find_get_entries(..., end - 1, ...)),
> > end needs to point to the start of the folio if the tail folio from
> > the split is a large folio, in order to exclude that folio from then
> > being truncated. But with the (pos + offset + length) >> PAGE_SHIFT
> > calculation here, does that result in some cases in it pointing to the
> > middle of a large folio? Maybe some logic is needed to make sure that
> > it points to the start?
> >
>
> Hi Joanne,
>
> Thanks for your careful review! I don't think that case can actually
> occur. Let me walk through the scenarios where the
> (pos + offset + length) >> PAGE_SHIFT calculation is kept as the final
> end value:

Hi Yi,

Thanks for your reply and for walking through the logic and explaining it.

>
> 1) offset + length == size:
>    The truncate range aligns exactly with the folio boundary, so
>    (pos + offset + length) >> PAGE_SHIFT points to the start of the next
>    folio, not the middle of one.
>
> 2) !folio_try_get(folio2):
>    The folio at that position has already been freed or is being freed,
>    so there is no folio in the page cache at that location. The
>    subsequent find_get_entries() won't find anything there.
>
> 3) !folio_test_large(folio2):
>    folio2 is no longer large, likely split to order-0 by a concurrent
>    operation. For an order-0 folio, the page index and folio index are
>    the same, so the calculation is correct.
>
> 4) folio2 becomes stale:
>    The same to case 2), folio2 is removed from the address space, so
>    there is no large folio straddling the boundary that needs
>    protection. The caller won't get folio from here through
>    find_get_entries(). Using the page index is safe here.
>
> If a large folio straddles the boundary at (offset + length), we will
> successfully get a reference via folio_try_get(folio2) and
> folio_test_large(folio2) will be true. In that case, if the split fails
> (cannot lock or split operation fails), we set tail_isolated to false
> and set *end = folio2->index to point to the start of that large folio
> since tail_isolated.

The case I have in mind is the case where the 2nd split succeeds
(returns 0) and tail_isolated will not be set to false, and *end still
gets returned back to the caller as the original "(pos + offset +
length) >> PAGE_SHIFT" calculation. I don't think it's guaranteed that
the split will create a folio that starts at that page.

The example I'm thinking about is a 64k folio being truncated at
offset 0 to 36k on a 16k blocksize filesystem with 4k pages:
*end = (pos + offset + length) >> PAGE_SHIFT = 36k >> PAGE_SHIFT = page index 9
start: [0 - 15]
after 1st split: [0 - 3] [4 - 7] [8 - 15]
split_at2 = 36k / PAGE_SIZE = 9
2nd split will try splitting [8 - 15] at split_at2
after 2nd split: [8 - 11] [12 - 15]

in the truncate_inode_pages_range() logic, start = 0, end = 9, so
find_get_entries(..., end - 1 (= 8) ...) returns the [8 - 11] folio
and truncate_inode_folio() will drop all 4 of those pages (including
36k to 48k which might have dirty data).

Do you think this makes sense or am I missing something?

>
> The page index from this setting is only kept when no large folio exists
> at the boundary, which makes it safe to use. What is particularly
> noteworthy is that for cases 2 and 4 above, aside from setting it to
> (pos + offset + length) >> PAGE_SHIFT, there does not seem to be any
> better alternative.

I think if we just rounded down *end by the min order (eg *end =
round_down((pos + offset + length) >> PAGE_SHIFT, 1UL << min_order);),
that would ensure end is always on a folio boundary and can't be
inside a folio.

Thanks,
Joanne