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

[Chris S]

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 AM Jan Kara <jack@xxxxxxx> 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 than
> > the page cache copy.  b_frozen_data is kmalloc()ed, so folio_set_bh() makes
> > the shadow buffer point at a slab folio.
> >
> > That is not something the buffer_head layer can reason about.  A slab folio
> > 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_folio
> > NULL and set b_data.  The previous patch taught fs/buffer.c to submit such
> > a buffer.  The two commit-path checksum helpers are the only other users of
> > the shadow buffer's contents, and they take the data directly rather than
> > 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 and in
> > the previous patch.
> >
> > Tested with ext4 mounted data=journal,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 == NULL path was taken.
> >
> > Suggested-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > Signed-off-by: Chao Shi <coshi036@xxxxxxxxx>
>
> ...
>
> > 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 = false;
> >       struct buffer_head *bh_in = jh2bh(jh_in);
> >       journal_t *journal = 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 = virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> > +             frozen = true;
> >               do_escape = jbd2_data_needs_escaping(jh_in->b_frozen_data);
> >               if (do_escape)
> >                       jbd2_data_do_escape(jh_in->b_frozen_data);
> > @@ -400,13 +400,22 @@ int jbd2_journal_write_metadata_buffer(transaction_t *transaction,
> >               jh_in->b_frozen_triggers = jh_in->b_triggers;
> >
> >  copy_done:
> > -             new_folio = virt_to_folio(jh_in->b_frozen_data);
> > -             new_offset = offset_in_folio(new_folio, jh_in->b_frozen_data);
> > +             frozen = 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 the
> > +              * buffer at it directly rather than at a slab folio, whose
> > +              * ->mapping is not an address_space.
> > +              */
> > +             new_bh->b_folio = NULL;
> > +             new_bh->b_data = 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 need
> to set new_bh->b_data in the two branches where frozen data is used).
>
>                                                                 Honza
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR