Re: [PATCH] xfs: compute buffer address correctly in xmbuf_map_backing_mem

Re: [PATCH] xfs: compute buffer address correctly in xmbuf_map_backing_mem

[Christoph Hellwig]

On Mon, Apr 07, 2025 at 05:30:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Prior to commit e614a00117bc2d, xmbuf_map_backing_mem relied on
> folio_file_page to return the base page for the xmbuf's loff_t in the
> xfile, and set b_addr to the page_address of that base page.
> 
> Now that folio_file_page has been removed from xmbuf_map_backing_mem, we
> always set b_addr to the folio_address of the folio.  This is correct
> for the situation where the folio size matches the buffer size, but it's
> totally wrong if tmpfs uses large folios.  We need to use
> offset_in_folio here.
> 
> Found via xfs/801, which demonstrated evidence of corruption of an
> in-memory rmap btree block right after initializing an adjacent block.

Hmm, I thought we'd never get large folios for our non-standard tmpfs
use.  I guess I was wrong on that..

The fix looks good:

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

But a little note below:

> +	bp->b_addr = folio_address(folio) + offset_in_folio(folio, pos);

Given that this is or at least will become a common pattern, do we
want a mm layer helper for it?

Re: [PATCH] xfs: compute buffer address correctly in xmbuf_map_backing_mem

[Darrick J. Wong]

On Mon, Apr 07, 2025 at 10:11:08PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 07, 2025 at 05:30:30PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Prior to commit e614a00117bc2d, xmbuf_map_backing_mem relied on
> > folio_file_page to return the base page for the xmbuf's loff_t in the
> > xfile, and set b_addr to the page_address of that base page.
> > 
> > Now that folio_file_page has been removed from xmbuf_map_backing_mem, we
> > always set b_addr to the folio_address of the folio.  This is correct
> > for the situation where the folio size matches the buffer size, but it's
> > totally wrong if tmpfs uses large folios.  We need to use
> > offset_in_folio here.
> > 
> > Found via xfs/801, which demonstrated evidence of corruption of an
> > in-memory rmap btree block right after initializing an adjacent block.
> 
> Hmm, I thought we'd never get large folios for our non-standard tmpfs
> use.  I guess I was wrong on that..

Yeah, you can force THPs for tmpfs.  I don't know why you would, the
memory usage is gawful on most files that end up in there.

> The fix looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> But a little note below:
> 
> > +	bp->b_addr = folio_address(folio) + offset_in_folio(folio, pos);
> 
> Given that this is or at least will become a common pattern, do we
> want a mm layer helper for it?

Yeah, we should; this is the third one in XFS.  What to name it, though?

void *folio_addr(const struct folio *folio, loff_t pos) ?

I'm surprised there wasn't an equivalent for struct page.

--D