Re: [PATCH 03/19] jbd2: point the shadow buffer at the frozen data directly

Hi Jan,

Thanks for the suggestion - done in v2.  new_folio, new_offset and the
frozen flag are all gone; folio_set_bh() moved into the one path that
still needs it, and the two copy-out paths just set new_bh->b_data.
b_folio is already NULL out of alloc_buffer_head(), so nothing has to
clear it either.

Chao

On Tue, Aug 4, 2026 at 4:28=E2=80=AFAM Jan Kara <[email protected]> wrote:
>
> On Sat 01-08-26 18:00:47, Chao Shi wrote:
> > When a metadata buffer has to be copied out before it can be journalled=
,
> > jbd2_journal_write_metadata_buffer() writes jh->b_frozen_data rather th=
an
> > the page cache copy.  b_frozen_data is kmalloc()ed, so folio_set_bh() m=
akes
> > the shadow buffer point at a slab folio.
> >
> > That is not something the buffer_head layer can reason about.  A slab f=
olio
> > overloads ->mapping, so a shadow buffer looks like it belongs to an
> > address_space when it does not.  buffer_set_crypto_ctx() already has to=
 use
> > folio_mapping() to avoid tripping over this, and it is the reason
> > mark_buffer_write_io_error() cannot be called on a shadow buffer today.
> >
> > Point the shadow buffer at the frozen data itself instead: leave b_foli=
o
> > NULL and set b_data.  The previous patch taught fs/buffer.c to submit s=
uch
> > a buffer.  The two commit-path checksum helpers are the only other user=
s of
> > the shadow buffer's contents, and they take the data directly rather th=
an
> > kmapping a folio that is already mapped.
> >
> > Note that the shadow buffer must not be passed to bh_offset() while
> > b_folio is NULL.  All four callers that can see one are handled here an=
d in
> > the previous patch.
> >
> > Tested with ext4 mounted data=3Djournal,journal_checksum on a metadata_=
csum
> > filesystem, writing files whose every block begins with the JBD2 magic =
so
> > that escaping forces the copy-out, then crashing with sysrq-b without
> > unmounting and replaying the journal on the next mount.  Recovery
> > completed, the file contents matched, e2fsck -fn was clean, and an
> > instrumented build confirmed the b_folio =3D=3D NULL path was taken.
> >
> > Suggested-by: Matthew Wilcox (Oracle) <[email protected]>
> > Signed-off-by: Chao Shi <[email protected]>
>
> ...
>
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 09efa337649e..9e4cb04587b4 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -329,6 +329,7 @@ int jbd2_journal_write_metadata_buffer(transaction_=
t *transaction,
> >       struct buffer_head *new_bh;
> >       struct folio *new_folio;
> >       unsigned int new_offset;
> > +     bool frozen =3D false;
> >       struct buffer_head *bh_in =3D jh2bh(jh_in);
> >       journal_t *journal =3D transaction->t_journal;
> >
> > @@ -354,8 +355,7 @@ int jbd2_journal_write_metadata_buffer(transaction_=
t *transaction,
> >        * we use that version of the data for the commit.
> >        */
> >       if (jh_in->b_frozen_data) {
> > -             new_folio =3D virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset =3D offset_in_folio(new_folio, jh_in->b_frozen=
_data);
> > +             frozen =3D true;
> >               do_escape =3D jbd2_data_needs_escaping(jh_in->b_frozen_da=
ta);
> >               if (do_escape)
> >                       jbd2_data_do_escape(jh_in->b_frozen_data);
> > @@ -400,13 +400,22 @@ int jbd2_journal_write_metadata_buffer(transactio=
n_t *transaction,
> >               jh_in->b_frozen_triggers =3D jh_in->b_triggers;
> >
> >  copy_done:
> > -             new_folio =3D virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset =3D offset_in_folio(new_folio, jh_in->b_frozen=
_data);
> > +             frozen =3D true;
> >               jbd2_data_do_escape(jh_in->b_frozen_data);
> >       }
> >
> >  escape_done:
> > -     folio_set_bh(new_bh, new_folio, new_offset);
> > +     if (frozen) {
> > +             /*
> > +              * b_frozen_data is slab memory, not page cache.  Point t=
he
> > +              * buffer at it directly rather than at a slab folio, who=
se
> > +              * ->mapping is not an address_space.
> > +              */
> > +             new_bh->b_folio =3D NULL;
> > +             new_bh->b_data =3D jh_in->b_frozen_data;
> > +     } else {
> > +             folio_set_bh(new_bh, new_folio, new_offset);
> > +     }
>
> Can we please simplify the flow in this function? folio_set_bh() is now
> done only for a single branch in this function. You can move it there and
> completely avoid new_folio, new_offset, and frozen variables (you just ne=
ed
> to set new_bh->b_data in the two branches where frozen data is used).
>
>                                                                 Honza
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR