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 1:33 AM, Joanne Koong wrote:
> On Thu, Sep 3, 2026 at 11:27 PM Zhang Yi <yi.zhang@huaweicloud.com> 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.