* [PATCH 10/10] mm: Use folio_next_pos() [not found] <20251024170822.1427218-1-willy@infradead.org> @ 2025-10-24 17:08 ` Matthew Wilcox (Oracle) 2025-10-27 7:54 ` Christoph Hellwig ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Matthew Wilcox (Oracle) @ 2025-10-24 17:08 UTC (permalink / raw) To: linux-fsdevel, Christian Brauner Cc: Matthew Wilcox (Oracle), Hugh Dickins, Baolin Wang, Andrew Morton, linux-mm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 10/10] mm: Use folio_next_pos() 2025-10-24 17:08 ` [PATCH 10/10] mm: Use folio_next_pos() Matthew Wilcox (Oracle) @ 2025-10-27 7:54 ` Christoph Hellwig 2025-10-27 15:43 ` David Hildenbrand 2025-11-08 5:00 ` Matthew Wilcox 2 siblings, 0 replies; 5+ messages in thread From: Christoph Hellwig @ 2025-10-27 7:54 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: linux-fsdevel, Christian Brauner, Hugh Dickins, Baolin Wang, Andrew Morton, linux-mm Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 10/10] mm: Use folio_next_pos() 2025-10-24 17:08 ` [PATCH 10/10] mm: Use folio_next_pos() Matthew Wilcox (Oracle) 2025-10-27 7:54 ` Christoph Hellwig @ 2025-10-27 15:43 ` David Hildenbrand 2025-11-08 5:00 ` Matthew Wilcox 2 siblings, 0 replies; 5+ messages in thread From: David Hildenbrand @ 2025-10-27 15:43 UTC (permalink / raw) To: Matthew Wilcox (Oracle), linux-fsdevel, Christian Brauner Cc: Hugh Dickins, Baolin Wang, Andrew Morton, linux-mm 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 10/10] mm: Use folio_next_pos() 2025-10-24 17:08 ` [PATCH 10/10] mm: Use folio_next_pos() Matthew Wilcox (Oracle) 2025-10-27 7:54 ` Christoph Hellwig 2025-10-27 15:43 ` David Hildenbrand @ 2025-11-08 5:00 ` Matthew Wilcox 2025-11-11 9:59 ` Christian Brauner 2 siblings, 1 reply; 5+ messages in thread From: Matthew Wilcox @ 2025-11-08 5:00 UTC (permalink / raw) To: linux-fsdevel, Christian Brauner Cc: Hugh Dickins, Baolin Wang, Andrew Morton, linux-mm 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 10/10] mm: Use folio_next_pos() 2025-11-08 5:00 ` Matthew Wilcox @ 2025-11-11 9:59 ` Christian Brauner 0 siblings, 0 replies; 5+ messages in thread From: Christian Brauner @ 2025-11-11 9:59 UTC (permalink / raw) To: Matthew Wilcox Cc: linux-fsdevel, Hugh Dickins, Baolin Wang, Andrew Morton, linux-mm 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) ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-11-11 9:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20251024170822.1427218-1-willy@infradead.org>
2025-10-24 17:08 ` [PATCH 10/10] mm: Use folio_next_pos() Matthew Wilcox (Oracle)
2025-10-27 7:54 ` Christoph Hellwig
2025-10-27 15:43 ` David Hildenbrand
2025-11-08 5:00 ` Matthew Wilcox
2025-11-11 9:59 ` Christian Brauner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox