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

[Zhang Yi]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> Looks good to me. Feel free to add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> 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.