Re: [PATCH] pagemap: Add alert to mapping_set_release_always() for mapping with no release_folio

[Deepak Karn]

On Fri, Dec 12, 2025 at 11:17 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Fri 12-12-25 12:18:11, Matthew Wilcox wrote:
> > On Thu, Dec 11, 2025 at 10:23:13AM +0100, Jan Kara wrote:
> > > On Wed 10-12-25 21:36:43, Matthew Wilcox wrote:
> > > > On Thu, Dec 11, 2025 at 01:31:04AM +0530, Deepakkumar Karn wrote:
> > > > >  static inline void mapping_set_release_always(struct address_space *mapping)
> > > > >  {
> > > > > +       /* Alert while setting the flag with no release_folio callback */
> > > >
> > > > The comment is superfluous.
> > >
> > > Agreed.
> > >
> > > > > +       VM_WARN_ONCE(!mapping->a_ops->release_folio,
> > > > > +                    "Setting AS_RELEASE_ALWAYS with no release_folio");
> > > >
> > > > But you haven't said why we need to do this.  Surely the NULL pointer
> > > > splat is enough to tell you that you did something stupid?
> > >
> > > Well, but this will tell it much earlier and it will directly point to the
> > > place were you've done the mistake (instead of having to figure out why
> > > drop_buffers() is crashing on you). So I think this assert makes sense to
> > > ease debugging and as kind of self-reminding documentation :).
> >
> > Oh.  So the real problem here is this:
> >
> >         if (mapping && mapping->a_ops->release_folio)
> >                 return mapping->a_ops->release_folio(folio, gfp);
> >         return try_to_free_buffers(folio);
> >
> > We should have a block_release_folio(), change all the BH-based
> > filesystems to add it to their aops, and then change
> > filemap_release_folio() to do:
> >
> >       if (mapping)
> >               return mapping->a_ops->release_folio(folio, gfp);
> >       return true;
>
> OK, yes, this would work for me and I agree it looks like a nice cleanup.
>
> > (actually, can the !mapping case be hit?  surely this can't be called
> > for folios which have already been truncated?)
>
> You'd think ;). There's some usage of try_to_free_buffers() from the dark
> ages predating git era in jbd2 (back then jbd) which is specifically run
> when we are done with journalling buffers on a page that was truncated -
> see fs/jbd2/commit.c:release_buffer_page(). Also there's an interesting
> case in migrate_folio_unmap() which calls try_to_free_buffers() for a
> truncated page. All the other users seem to have a valid mapping.
>

Thank you Matthew for the suggested change. Your suggestion seems to make more
sense to  cure the case. If my understanding is correct, with this
change the failure will
happens in filemap_release_folio(). The current change isn't solving
the case, as it
doesn't mandate FS to declare release.

As Jan mentioned, about the jbd2 and migrate_folio_unmap() cases
(honestly new learning
 for me, @Jan) this change should be okay. I will try and do some
tests with this. I will share
the updated patch.

Do let me know if I missed anything.

Regards,
Deepakkumar Karn