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

[Zhang Yi]

On 9/5/2026 1:33 AM, Joanne Koong wrote:
> 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?

Ha, indeed, thanks for pointing this out.  The second split only
isolates the tail when the boundary is min_order-aligned, otherwise
the straddler still remains and the valid tail gets dropped.  So this
problem is not only triggered when the second split fails and I've
reproduced this scenario. :)

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

Yeah, rounding *end down to a min_order boundary looks good to me.
I'll update the patch accordingly.

Thanks,
Yi.