[PATCH 10/10] mm: Use folio_next_pos()

[PATCH 10/10] mm: Use folio_next_pos()

[Matthew Wilcox (Oracle)]

This is one instruction more efficient than open-coding folio_pos() +
folio_size().  It's the equivalent of (x + y) << z rather than
x << z + y << z.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Cc: Hugh Dickins <hughd@google.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org
---
 mm/shmem.c    | 2 +-
 mm/truncate.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index b9081b817d28..c819cecf1ed9 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1133,7 +1133,7 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,
 	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
 	folio = shmem_get_partial_folio(inode, lstart >> PAGE_SHIFT);
 	if (folio) {
-		same_folio = lend < folio_pos(folio) + folio_size(folio);
+		same_folio = lend < folio_next_pos(folio);
 		folio_mark_dirty(folio);
 		if (!truncate_inode_partial_folio(folio, lstart, lend)) {
 			start = folio_next_index(folio);
diff --git a/mm/truncate.c b/mm/truncate.c
index 91eb92a5ce4f..a3d673533e32 100644
--- a/mm/truncate.c
+++ b/mm/truncate.c
@@ -387,7 +387,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
 	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
 	folio = __filemap_get_folio(mapping, lstart >> PAGE_SHIFT, FGP_LOCK, 0);
 	if (!IS_ERR(folio)) {
-		same_folio = lend < folio_pos(folio) + folio_size(folio);
+		same_folio = lend < folio_next_pos(folio);
 		if (!truncate_inode_partial_folio(folio, lstart, lend)) {
 			start = folio_next_index(folio);
 			if (same_folio)
-- 
2.47.2

Re: [PATCH 10/10] mm: Use folio_next_pos()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 10/10] mm: Use folio_next_pos()

[David Hildenbrand]

On 24.10.25 19:08, Matthew Wilcox (Oracle) wrote:
> This is one instruction more efficient than open-coding folio_pos() +
> folio_size().  It's the equivalent of (x + y) << z rather than
> x << z + y << z.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-mm@kvack.org
> ---

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers

David / dhildenb

Re: [PATCH 10/10] mm: Use folio_next_pos()

[Matthew Wilcox]

On Fri, Oct 24, 2025 at 06:08:18PM +0100, Matthew Wilcox (Oracle) wrote:
> +++ b/mm/truncate.c
> @@ -387,7 +387,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
>  	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
>  	folio = __filemap_get_folio(mapping, lstart >> PAGE_SHIFT, FGP_LOCK, 0);
>  	if (!IS_ERR(folio)) {
> -		same_folio = lend < folio_pos(folio) + folio_size(folio);
> +		same_folio = lend < folio_next_pos(folio);

This causes an intermittent failure with XFS.  Two reports here:
https://lore.kernel.org/linux-xfs/aQohjfEFmU8lef6M@casper.infradead.org/

This is a fun one.  The "fix" I'm running with right now is:

-               same_folio = lend < folio_next_pos(folio);
+               same_folio = lend < (u64)folio_next_pos(folio);

folio_pos() and folio_next_pos() return an loff_t.  folio_size() returns
a size_t.  So folio_pos() + folio_size() is unsigned (by the usual C
promotion rules).  Before this patch, this was an unsigned comparison
against lend, and with the patch it's now a signed comparison.  Since
lend can be -1 (to mean 'end of file'), same_folio will now be 'true'
when it used to be 'false'.

Funnily, on 32-bit systems, size_t is u32 and loff_t is s64, so their
addition is also s64.  That means this has been wrong on 32-bit systems
for ... a while.  And nobody noticed, so I guess nobody's testing 32-bit
all that hard.

Anyway, what's the *right* way to fix this?  Cast to (unsigned long
long)?  There's an ssize_t, but I really want the opposite, a uloff_t.

(now i'm going to go back through all the other patches in this series
and see if I've made the same mistake anywhere else)

Re: [PATCH 10/10] mm: Use folio_next_pos()

[Christian Brauner]

On Sat, Nov 08, 2025 at 05:00:28AM +0000, Matthew Wilcox wrote:
> On Fri, Oct 24, 2025 at 06:08:18PM +0100, Matthew Wilcox (Oracle) wrote:
> > +++ b/mm/truncate.c
> > @@ -387,7 +387,7 @@ void truncate_inode_pages_range(struct address_space *mapping,
> >  	same_folio = (lstart >> PAGE_SHIFT) == (lend >> PAGE_SHIFT);
> >  	folio = __filemap_get_folio(mapping, lstart >> PAGE_SHIFT, FGP_LOCK, 0);
> >  	if (!IS_ERR(folio)) {
> > -		same_folio = lend < folio_pos(folio) + folio_size(folio);
> > +		same_folio = lend < folio_next_pos(folio);
> 
> This causes an intermittent failure with XFS.  Two reports here:
> https://lore.kernel.org/linux-xfs/aQohjfEFmU8lef6M@casper.infradead.org/
> 
> This is a fun one.  The "fix" I'm running with right now is:
> 
> -               same_folio = lend < folio_next_pos(folio);
> +               same_folio = lend < (u64)folio_next_pos(folio);
> 
> folio_pos() and folio_next_pos() return an loff_t.  folio_size() returns
> a size_t.  So folio_pos() + folio_size() is unsigned (by the usual C
> promotion rules).  Before this patch, this was an unsigned comparison
> against lend, and with the patch it's now a signed comparison.  Since
> lend can be -1 (to mean 'end of file'), same_folio will now be 'true'
> when it used to be 'false'.
> 
> Funnily, on 32-bit systems, size_t is u32 and loff_t is s64, so their
> addition is also s64.  That means this has been wrong on 32-bit systems
> for ... a while.  And nobody noticed, so I guess nobody's testing 32-bit
> all that hard.

death by slow degradation...

> Anyway, what's the *right* way to fix this?  Cast to (unsigned long

Naively, I would think it should be an unsigned comparison for u64.

> long)?  There's an ssize_t, but I really want the opposite, a uloff_t.

Fwiw, I don't think anything is stopping us from defining uloff_t in the
kernel if it's helpful.

> (now i'm going to go back through all the other patches in this series
> and see if I've made the same mistake anywhere else)