Re: [PATCH v3 1/3] mm/truncate: fix data loss when splitting straddling large folios fails

[Zhang Yi <yizhang089@gmail.com>]

On 9/17/2026 1:38 AM, Brian Foster wrote:
> On Wed, Sep 16, 2026 at 05:24:48PM +0800, 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.
>>
>> Rework the contract so the caller is told the page range to discard:
>>
>>    - Add pgoff_t *pstart and *pend out-parameters that receive the page
>>      range fully covered by [lstart, lend] after any split (or none),
>>      i.e. the pages wholly within the range and safe to discard.
>>
>>    - Adjust the ordering of the validate check when splitting folio2.
>>      folio2->index is only reliable after the reference count and lock
>>      have been successfully acquired, since it may have been split
>>      concurrently, or freed and recycled to an unrelated mapping. On any
>>      failure to obtain a reliable end position, fall back to
>>      folio->index, which is safe but leaves the sub-folios split off at
>>      the offset edge in the page cache.
>>
>>    - Rename the byte-range parameters start/end to lstart/lend to better
>>      express their semantics.
>>
>> Callers in truncate_inode_pages_range() and shmem_undo_range() pass
>> &pstart for the folio at the start edge and &pend for the folio at the
>> end edge, so the truncate loop drops exactly the fully covered pages and
>> never touches a straddling folio that still holds valid out-of-range
>> data.
>>
>> Suggested-by: Brian Foster <bfoster@redhat.com>
>> Link: https://lore.kernel.org/linux-fsdevel/anH-WKA1coW6wtfG@bfoster/
>> Fixes: 7460b470a131 ("mm/truncate: use folio_split() in truncate operation")
>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>> ---
>>   mm/internal.h |  4 +--
>>   mm/shmem.c    | 13 +++-----
>>   mm/truncate.c | 89 ++++++++++++++++++++++++++++++++++++---------------
>>   3 files changed, 71 insertions(+), 35 deletions(-)
>>
> ...
>> diff --git a/mm/truncate.c b/mm/truncate.c
>> index b58ba940be47..bec6d881d022 100644
>> --- a/mm/truncate.c
>> +++ b/mm/truncate.c
> ...
>> @@ -259,32 +271,62 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t start, loff_t end)
>>   		 * for shmem truncate
>>   		 */
>>   		struct folio *folio2;
>> +		pgoff_t end, aligned_end = (pos + offset + length) >>
>> +					   PAGE_SHIFT;
>>   
>> -		if (offset + length == size)
>> -			goto no_split;
>> +		if (pstart)
>> +			*pstart = round_up(pos + offset, PAGE_SIZE) >>
>> +				  PAGE_SHIFT;
>> +
>> +		if (offset + length == size) {
>> +			end = aligned_end;
>> +			goto out;
>> +		}
>>   
>>   		split_at2 = folio_page(folio,
>>   				PAGE_ALIGN_DOWN(offset + length) / PAGE_SIZE);
>>   		folio2 = page_folio(split_at2);
>>   
>> +		/*
>> +		 * folio2 may become stale due to a concurrent split or
>> +		 * freeing, so validate it before and after taking its lock.
>> +		 * If it fails, we can't get an accurate end position and fall
>> +		 * back to folio->index, which may leave sub-folios split off
>> +		 * at the offset edge in the page cache this round.
>> +		 */
>> +		end = folio->index;
>>   		if (!folio_try_get(folio2))
>> -			goto no_split;
>> -
>> -		if (!folio_test_large(folio2))
>>   			goto out;
>>   
>> +		if (folio2->mapping != folio->mapping ||
>> +		    !folio_test_large(folio2))
>> +			goto out_put;
>> +
> 
> Hi Zhang,
> 
> The only thing that sticks out to me in this version is the
> !folio_test_large() check above. If we split (or something happens) such
> that folio2 is no longer large, wouldn't it be more likely to catch it
> here and fail to update end like we do in the same check a bit further
> down after the folio lock? Maybe I'm missing something here, but it
> seems a little odd for the same condition to return two possible states
> like this. Otherwise I like the changes you've made and the rest of the
> patch LGTM.
> 
> Brian

Yes, you're right! That check should be dropped, I got myself
confused with all these complicated checks. Fortunately Jan and Zi Yan
suggested using __filemap_get_folio(), and with that I think we can
get rid of all these convoluted checks.

Thanks,
Yi.

> 
>>   		if (!folio_trylock(folio2))
>> -			goto out;
>> +			goto out_put;
>>   
>> -		/* make sure folio2 is large and does not change its mapping */
>> -		if (folio_test_large(folio2) &&
>> -		    folio2->mapping == folio->mapping)
>> -			folio_split_or_unmap(folio2, split_at2, min_order);
>> +		if (page_folio(split_at2) != folio2) {
>> +			folio_unlock(folio2);
>> +			goto out_put;
>> +		}
>> +		if (!folio_test_large(folio2)) {
>> +			end = aligned_end;
>> +			folio_unlock(folio2);
>> +			goto out_put;
>> +		}
>> +
>> +		/* Split failed: back off to the head of the straddler */
>> +		if (folio_split_or_unmap(folio2, split_at2, min_order))
>> +			end = folio2->index;
>> +		else
>> +			end = aligned_end;
>>   
>>   		folio_unlock(folio2);
>> -out:
>> +out_put:
>>   		folio_put(folio2);
>> -no_split:
>> +out:
>> +		if (pend)
>> +			*pend = end;
>>   		return true;
>>   	}
>>   	if (folio_test_dirty(folio))
>> @@ -413,11 +455,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>>   	folio = __filemap_get_folio(mapping, lstart >> PAGE_SHIFT, FGP_LOCK, 0);
>>   	if (!IS_ERR(folio)) {
>>   		same_folio = lend < folio_next_pos(folio);
>> -		if (!truncate_inode_partial_folio(folio, lstart, lend)) {
>> -			start = folio_next_index(folio);
>> -			if (same_folio)
>> -				end = folio->index;
>> -		}
>> +		truncate_inode_partial_folio(folio, lstart, lend, &start,
>> +					     same_folio ? &end : NULL);
>>   		folio_unlock(folio);
>>   		folio_put(folio);
>>   		folio = NULL;
>> @@ -427,8 +466,8 @@ void truncate_inode_pages_range(struct address_space *mapping,
>>   		folio = __filemap_get_folio(mapping, lend >> PAGE_SHIFT,
>>   						FGP_LOCK, 0);
>>   		if (!IS_ERR(folio)) {
>> -			if (!truncate_inode_partial_folio(folio, lstart, lend))
>> -				end = folio->index;
>> +			truncate_inode_partial_folio(folio, lstart, lend,
>> +						     NULL, &end);
>>   			folio_unlock(folio);
>>   			folio_put(folio);
>>   		}
>> -- 
>> 2.52.0
>>
>