Re: [PATCH 02/19] buffer: allow a buffer_head to point at memory outside the page cache

On Mon 03-08-26 18:59:01, Matthew Wilcox wrote:
> On Mon, Aug 03, 2026 at 06:13:19PM +0200, Jan Kara wrote:
> > On Sat 01-08-26 18:00:46, Chao Shi wrote:
> > > jbd2 builds a temporary buffer_head to write out the frozen copy of a
> > > metadata block, and that copy lives in slab memory.  Today jbd2 points the
> > > temporary buffer at the slab folio backing it.  A slab folio's ->mapping is
> > > not an address_space, so anything that follows bh->b_folio->mapping there
> > > gets garbage rather than NULL; mark_buffer_write_io_error() does exactly
> > > that, and we are about to start calling it on this buffer.
> > > 
> > > Rather than teach every such helper about slab folios, allow bh->b_folio to
> > > be NULL and let b_data point straight at the memory.  Code that needs the
> > > folio has to check.  There are two places in this file:
> > > 
> > >  - __bh_submit() adds the data by virtual address using
> > >    bio_add_virt_nofail(), and skips the cgroup accounting: a buffer that is
> > >    not in the page cache has no owning folio to attribute writeback to.
> > > 
> > >  - buffer_set_crypto_ctx() returns early.  fscrypt has no interest in a
> > >    buffer that is not part of a file mapping, which is why it already
> > >    returns when folio_mapping() comes back NULL.
> > > 
> > > Nothing sets b_folio to NULL yet, so this patch is a no-op on its own.
> > > 
> > > This is deliberately not a general capability.  Buffers over highmem have
> > > no permanent kernel virtual address, which is why folio_set_bh() records a
> > > folio and an offset instead of an address.  A folio-less buffer_head is
> > > only valid over memory that is always mapped, and must not be passed to
> > > bh_offset().
> > 
> > There are much more things you cannot do with a bh that doesn't have valid
> > b_folio - touch_buffer(), end_buffer_async_read(), ... and many more. But
> > that's a bussiness of the code that sets up such temporary bhs. I agree
> > that setting b_folio to NULL will if nothing else lead to much more obvious
> > failures than when we accidentally get slab folio. So I'd prefer we update
> > the description a bit in this direction but otherwise feel free to add:
> 
> Yes, agreed.  I think it's fine to add in some 'if (!bh->b_folio)'
> tests, but only where they're needed for the occasional consumer.  This
> really is a rare (but legitimate) case.

Yes. If we were to write this code from scratch, I'd rather push for
forming and submitting the bio directly instead of trying to bend the bh
layer for these temporary bhs. Eventually we might want to do this cleanup
anyway (since bhs don't really buy us much in this path) but not in this
patch set.

> > >  static void buffer_set_crypto_ctx(struct bio *bio, const struct buffer_head *bh,
> > >  				  gfp_t gfp_mask)
> > >  {
> > > -	const struct address_space *mapping = folio_mapping(bh->b_folio);
> > > +	const struct address_space *mapping;
> > >  
> > >  	/*
> > >  	 * The ext4 journal (jbd2) can submit a buffer_head it directly created
> > > -	 * for a non-pagecache page.  fscrypt doesn't care about these.
> > > +	 * for memory that is not in the page cache at all.  fscrypt doesn't
> > > +	 * care about these.
> > >  	 */
> > > +	if (!bh->b_folio)
> > > +		return;
> > > +	mapping = folio_mapping(bh->b_folio);
> > >  	if (!mapping)
> > >  		return;
> 
> Do we want to call folio_mapping() here?  The only case where I can see
> this being useful is if we attach a buffer_head to an anonymous folio,
> and I don't see a good reason to do that.  I think this can just be
> 
> 	mapping = bh->b_folio->mapping;

For all I know yes, bh->b_folio->mapping should be enough here. But I
thought folio_mapping() is kind of preferred to open-coding?

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR