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

Thank you so much.
Taken, with a change: the helper needs a matching unmap, and must not map
the folio-less case at all.

static inline void *kmap_local_bh(const struct buffer_head *bh)
{
if (!bh->b_folio)
return bh->b_data;
return kmap_local_folio(bh->b_folio, bh_offset(bh));
}

static inline void kunmap_local_bh(const struct buffer_head *bh, void *addr)
{
if (bh->b_folio)
kunmap_local(addr);
}

Two reasons.  kmap_local_page(virt_to_page(bh->b_data)) loses the offset
within the page, and b_frozen_data is kmalloc(b_size), which is only
guaranteed page aligned when the block size is at least PAGE_SIZE.

The second one is your other point.  With CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP,
__kmap_local_page_prot() maps even a lowmem page, one page at a time.  So
mapping memory that is already permanently mapped would walk this path
straight into the bs>PS problem you just described, which it is otherwise
immune to - b_data covers the whole block whatever its size.  Returning
b_data and calling kunmap_local() on it anyway is not an option either:
kunmap_local_indexed() WARNs on an address outside the fixmap range under
that config.  Hence the pair.

On the bs>PS folio path itself: agreed it is real and predates this series.
I would rather not fold a fix into this one, but I will take it - I'll look
at it separately once this has settled.

Chao

On Mon, Aug 3, 2026 at 11:12 PM Matthew Wilcox <[email protected]> wrote:
>
> On Mon, Aug 03, 2026 at 07:11:49PM +0100, Matthew Wilcox wrote:
> > On Sat, Aug 01, 2026 at 06:00:47PM -0400, Chao Shi wrote:
> > > @@ -330,6 +330,8 @@ static __u32 jbd2_checksum_data(__u32 crc32_sum, struct buffer_head *bh)
> > >     char *addr;
> > >     __u32 checksum;
> > >
> > > +   if (!bh->b_folio)
> > > +           return crc32_be(crc32_sum, bh->b_data, bh->b_size);
> > >     addr = kmap_local_folio(bh->b_folio, bh_offset(bh));
> > >     checksum = crc32_be(crc32_sum, addr, bh->b_size);
> > >     kunmap_local(addr);
> >
> > This is awkward.  How about ...
>
> ... Oh.  It's not just awkward.  The current code is actually wrong.
> If one uses ext4 on a bs>PS device and one has
> CONFIG_DEBUG_KMAP_LOCAL_FORCE_MAP enabled, it'll only map one
> page of the buffer and we should get a crash when trying to checksum
> the entire block.
>
> Supporting mapping multiple pages from a folio simultaneously is
> something we haven't figured out how to support.  Since HIGHMEM is
> dying (see Arnd's recent work), I'm not inclined to spend effort
> on it.
>
> We probably just need to make ext4 depend on !DEBUG_KMAP_LOCAL_FORCE_MAP
> or something?
>
> > static inline void *kmap_local_bh(const struct buffer_head *bh)
> > {
> >       if (bh->b_folio)
> >               return kmap_local_folio(bh->b_folio, bh_offset(bh));
> >       return kmap_local_page(virt_to_page(bh->b_data);
> > }
> >
> > (this is also somewhat awkward because it feels like we could just
> > return bh->b_data, but kunmap_local_indexed() does some ... stuff)
> >
> > Anyway, it all gets optimised away on non-HIGHMEM.  Or if it doesn't,
> > you can force it to ;-)
> >
> >