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

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

On Wed Sep 16, 2026 at 8:02 AM EDT, Jan Kara wrote:
> On Wed 16-09-26 17:24:48, 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>
>
> The changes mostly look good to me but I have some confusion around the
> folio2 splitting below.
>
>> @@ -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;
>> +
>>  		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;
>> +		}
>
> So I always found this folio2 lookup and revalidation somewhat suspicious
> and now that we're digging into it I'll ask: As you note in the changelog,
> by the time we compute split_at2 the page can be already freed and reused
> because it was split off from the original 'folio'. It can be for example a
> slab page or anything else. So is it guaranteed that page_folio() actually

For a slab or unrelated page, folio->mapping check rejects them.

> returns something sensible? What guarantees we properly detect the "reuse

No, but folio_try_get() prevents non ref'd folios.

> for something else" case in all possible cases for which the page can be
> reused? I understand this is mostly a preexisting issue so maybe these
> questions are more for MM guys than you...

I agree with you that trying to split a unlocked and not ref'd folio2 is
flaky. And it almost does a __filemap_get_folio() like you proposed
below.

>
> So for me as an filesystem guy I'd appreciate some comment in this code
> explaining why grabbing folio2 this way is actually safe. The really safe
> way of getting to folio2 would be to use
>   __filemap_get_folio(mapping, (offset+length) >> PAGE_SHIFT, FGP_LOCK, 0)
> but I suppose we don't use the mapping lookup as it is more expensive?

__filemap_get_folio() is a better version of the existing folio2 finding
code. When I wrote the code, I never thought about using
__filemap_get_folio(), but I probably should have done it.

BTW,
__filemap_get_folio(mapping, folio->index + (offset+length) >> PAGE_SHIFT,
		FGP_LOCK | FGP_NOWAIT, 0)
might be better:
1. folio->index is needed to get the right folio2,
2. nowait can us nonblocking.


>> +
>> +		/* 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
>> 




-- 
Best Regards,
Yan, Zi