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

[Chris S]

Hi Jan,
Thanks - changelog reworded along those lines in v2: a folio-less buffer is
a narrow thing, most of the buffer_head API will not tolerate one -
touch_buffer(), the async read completion path and plenty more - and it is
the job of whoever builds such a buffer to keep it away from all of that.
NULL only buys us that getting it wrong fails loudly instead of quietly
following a slab folio's ->mapping.  Reviewed-by carried; the code itself
is unchanged.

On your folio_mapping() question: it is the right call in general, just not
on this path, for the reason Matthew gave - it would hand back a
swap_address_space if a buffer ever sat on a swap cache folio.  v2 does
make that switch, but in a new patch, v2's 04, rather than in this one; my
reply to Matthew has why it cannot be done here.  And agreed on forming the
bio directly for these temporary bhs eventually - that is a bigger cleanup
than this series.

Chao

On Mon, Aug 3, 2026 at 12:13 PM Jan Kara <jack@xxxxxxx> 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:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
>                                                                 Honza
>
> >
> > Suggested-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
> > ---
> >  fs/buffer.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index be8b57a635cd..04fcc34e4fa6 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -1099,12 +1099,16 @@ EXPORT_SYMBOL(__bforget);
> >  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;
> >       fscrypt_set_bio_crypt_ctx(bio, mapping->host,
> > @@ -1142,7 +1146,11 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
> >       bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> >       bio->bi_write_hint = write_hint;
> >
> > -     bio_add_folio_nofail(bio, bh->b_folio, bh->b_size, bh_offset(bh));
> > +     if (bh->b_folio)
> > +             bio_add_folio_nofail(bio, bh->b_folio, bh->b_size,
> > +                                  bh_offset(bh));
> > +     else
> > +             bio_add_virt_nofail(bio, bh->b_data, bh->b_size);
> >
> >       bio->bi_end_io = end_bio;
> >       bio->bi_private = bh;
> > @@ -1152,7 +1160,8 @@ static void __bh_submit(struct buffer_head *bh, blk_opf_t opf,
> >
> >       if (wbc) {
> >               wbc_init_bio(wbc, bio);
> > -             wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
> > +             if (bh->b_folio)
> > +                     wbc_account_cgroup_owner(wbc, bh->b_folio, bh->b_size);
> >       }
> >
> >       blk_crypto_submit_bio(bio);
> > --
> > 2.43.0
> >
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR