diff mbox series

[v3,3/3] mm/truncate: clarify return value of truncate_inode_partial_folio()

Message ID 20260916092450.654408-4-yi.zhang@huaweicloud.com
State New
Headers show
Series mm/truncate: fix data loss when truncating straddling large folios | expand

Commit Message

Zhang Yi Sept. 16, 2026, 9:24 a.m. UTC 
From: Zhang Yi <yi.zhang@huawei.com>

With the earlier rework the callers no longer rely on the return value
of truncate_inode_partial_folio() to decide whether to adjust the
truncation range. The pstart/pend out-parameters carry that information
instead. The callers now only use the return value as a flag indicating
whether the loop should be reset to pick up newly split sub-folios on
the shmem path.

Return true if at least one split succeeded, and false otherwise.
This clarifies the existing confusing return value semantics.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 mm/truncate.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 16, 2026, 9:45 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: sub-folio page cache leak due to incorrect end boundary on split failure
--

commit 4bfa9d8fcd26bcd7a9625b0b4e4d07b1e0a6b145
Author: Zhang Yi <yi.zhang@huawei.com>

mm/truncate: clarify return value of truncate_inode_partial_folio()

This commit updates truncate_inode_partial_folio() to clarify its return
value semantics. Callers now use the return value strictly as a flag for
loop resetting, and rely instead on the pstart and pend out-parameters
to decide whether to adjust the truncation range.

> diff --git a/mm/truncate.c b/mm/truncate.c
> index 5ab7a40b1e256..23c90f00b5300 100644
> --- a/mm/truncate.c
> +++ b/mm/truncate.c
[ ... ]
> @@ -331,10 +330,9 @@ bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
>  			*pend = end;
>  		return true;
>  	}

[Severity: High]
With the new caller dependency on pstart and pend instead of the return
value, can this error path cause a sub-folio page cache leak?

If truncate_inode_partial_folio() is called for a range within a single
large folio, and the first split succeeds, *pstart is advanced. If the
second split attempt fails early during folio2 validation, the code behaves
as follows:

mm/truncate.c:truncate_inode_partial_folio() {
    ...
    end = folio->index;
    if (!folio_try_get(folio2))
        goto out;
    ...
out:
    if (pend)
        *pend = end;
    return true;
}

Does this set *pend backward to the start of the original large folio?

If this results in a situation where *pstart is greater than *pend, will
the caller bypass its discard loop entirely? This seems like it could
permanently leak all successfully split sub-folios in the range within the
page cache. For hole punches, could this result in reading stale data
instead of zeros?

> -	if (folio_test_dirty(folio))
> -		return false;
> -	truncate_inode_folio(folio->mapping, folio);
> -	return true;
> +	if (!folio_test_dirty(folio))
> +		truncate_inode_folio(folio->mapping, folio);
> +	return false;
>  }
>  
>  /*
diff mbox series

Patch

diff --git a/mm/truncate.c b/mm/truncate.c
index 5ab7a40b1e25..23c90f00b530 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -216,8 +216,7 @@  static int folio_split_or_unmap(struct folio *folio, struct page *split_at,
  * aligned inwards to min_order, i.e. the range of folios wholly within
  * [lstart, lend] and so safe to discard.
  *
- * Returns false if splitting failed so the caller can avoid
- * discarding the entire folio which is stubbornly unsplit.
+ * Return %true if at least one split succeeded, %false otherwise.
  */
 bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
 				  loff_t lend, pgoff_t *pstart, pgoff_t *pend)
@@ -247,7 +246,7 @@  bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
 	folio_wait_writeback(folio);
 	if (length == size) {
 		truncate_inode_folio(folio->mapping, folio);
-		return true;
+		return false;
 	}
 
 	/*
@@ -261,7 +260,7 @@  bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
 	if (folio_needs_release(folio))
 		folio_invalidate(folio, offset, length);
 	if (!folio_test_large(folio))
-		return true;
+		return false;
 
 	min_order = mapping_min_folio_order(folio->mapping);
 	min_nrbytes = mapping_min_folio_nrbytes(folio->mapping);
@@ -331,10 +330,9 @@  bool truncate_inode_partial_folio(struct folio *folio, loff_t lstart,
 			*pend = end;
 		return true;
 	}
-	if (folio_test_dirty(folio))
-		return false;
-	truncate_inode_folio(folio->mapping, folio);
-	return true;
+	if (!folio_test_dirty(folio))
+		truncate_inode_folio(folio->mapping, folio);
+	return false;
 }
 
 /*