diff mbox series

[v4,4/4] mm/truncate: clarify return value of truncate_inode_partial_folio()

Message ID 20260922110703.468389-5-yi.zhang@huaweicloud.com
State Not Applicable
Headers show
Series mm/truncate: fix data loss when truncating straddling large folios | expand

Commit Message

Zhang Yi Sept. 22, 2026, 11:07 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. 22, 2026, 11:29 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: newly split large folios are skipped during shmem truncation
--

commit 15553d4635c639eef475fb728e2da88b1d36fe93
Author: Zhang Yi <yi.zhang@huawei.com>

mm/truncate: clarify return value of truncate_inode_partial_folio()

This commit clarifies the return value of truncate_inode_partial_folio()
to strictly indicate whether at least one split succeeded. The callers now
use this return value as a flag to determine whether the truncation loop
should be reset to pick up newly split sub-folios on the shmem path.

> diff --git a/mm/truncate.c b/mm/truncate.c
> index 81fb4de6226bc..317cb4ae36259 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)

[Severity: High]
Does shmem_undo_range() still rely on an outdated check instead of just this
return value?

mm/shmem.c:shmem_undo_range() {
    ...
				} else if (truncate_inode_partial_folio(folio,
						lstart, lend, NULL, NULL)) {
					if (!folio_test_large(folio)) {
						folio_unlock(folio);
						index = start;
						break;
					}
				}
    ...
}

Because folio_split() can perform non-uniform splits, the resulting sub-folio
could still be a large folio (order > 0). If it is, folio_test_large(folio)
evaluates to true, so this check prevents the loop from resetting even when
truncate_inode_partial_folio() successfully split it.

Since find_get_entries() has already advanced the search index, the loop
continues from the advanced index and bypasses the newly split sub-folios
within the truncation range.

Can this cause newly split sub-folios to be skipped, leaving them in the page
cache beyond EOF?

[ ... ]
Jan Kara Sept. 22, 2026, 11:58 a.m. UTC | #2 
On Tue 22-09-26 19:07:03, Zhang Yi wrote:
> 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>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

Just as I'm checking the only use of the return value of
truncate_inode_partial_folio() in shmem_undo_range(), the code there looks
fishy. It does:

        } else if (truncate_inode_partial_folio(folio, lstart, lend)) {
                /*
                 * If we split a page, reset the loop so
                 * that we pick up the new sub pages.
                 * Otherwise the THP was entirely
                 * dropped or the target range was
                 * zeroed, so just continue the loop as
                 * is.
                 */
                if (!folio_test_large(folio)) {
                        folio_unlock(folio);
                        index = start;
                        break;
                }
        }

However if folio was say order-2 (4 pages), lstart is inside the 3rd page,
then the split done by truncate_inode_partial_folio() can result in order-1
folio, and two order-0 folios. Hence the !folio_test_large(folio) check
fails and we won't pickup the new smaller folios so that we can free the
4th one... So I think we need to unconditionally reset the loop if
truncate_inode_partial_folio() returned true.

								Honza
Zi Yan Sept. 22, 2026, 5:28 p.m. UTC | #3 
On Tue Sep 22, 2026 at 7:07 AM EDT, Zhang Yi wrote:
> 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(-)
>

Thanks.

Acked-by: Zi Yan <ziy@nvidia.com>
Zhang Yi Sept. 23, 2026, 1:29 p.m. UTC | #4 
On 9/22/2026 7:58 PM, Jan Kara wrote:
> On Tue 22-09-26 19:07:03, Zhang Yi wrote:
>> 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>
> 
> Looks good to me. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Just as I'm checking the only use of the return value of
> truncate_inode_partial_folio() in shmem_undo_range(), the code there looks
> fishy. It does:
> 
>          } else if (truncate_inode_partial_folio(folio, lstart, lend)) {
>                  /*
>                   * If we split a page, reset the loop so
>                   * that we pick up the new sub pages.
>                   * Otherwise the THP was entirely
>                   * dropped or the target range was
>                   * zeroed, so just continue the loop as
>                   * is.
>                   */
>                  if (!folio_test_large(folio)) {
>                          folio_unlock(folio);
>                          index = start;
>                          break;
>                  }
>          }
> 
> However if folio was say order-2 (4 pages), lstart is inside the 3rd page,
> then the split done by truncate_inode_partial_folio() can result in order-1
> folio, and two order-0 folios. Hence the !folio_test_large(folio) check
> fails and we won't pickup the new smaller folios so that we can free the
> 4th one... So I think we need to unconditionally reset the loop if
> truncate_inode_partial_folio() returned true.
> 
> 								Honza

Indeed, that's a good point. Since this is a pre-existing and
independent issue, I'll send a separate patch to fix it.

Thanks,
Yi.
diff mbox series

Patch

diff --git a/mm/truncate.c b/mm/truncate.c
index 81fb4de6226b..317cb4ae3625 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);
@@ -326,10 +325,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;
 }
 
 /*