Re: [PATCH] mm/page-writeback: drop usage of folio_index

[Matthew Wilcox <willy@infradead.org>]

On Fri, Aug 15, 2025 at 11:03:56PM +0800, Kairui Song wrote:
> On Fri, Aug 15, 2025 at 9:48 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Aug 15, 2025 at 08:12:52PM +0800, Kairui Song wrote:
> > > +++ b/mm/page-writeback.c
> > > @@ -2739,8 +2739,8 @@ void __folio_mark_dirty(struct folio *folio, struct address_space *mapping,
> > >       if (folio->mapping) {   /* Race with truncate? */
> > >               WARN_ON_ONCE(warn && !folio_test_uptodate(folio));
> > >               folio_account_dirtied(folio, mapping);
> > > -             __xa_set_mark(&mapping->i_pages, folio_index(folio),
> > > -                             PAGECACHE_TAG_DIRTY);
> > > +             __xa_set_mark(&mapping->i_pages, folio->index,
> > > +                           PAGECACHE_TAG_DIRTY);
> > >       }
> > >       xa_unlock_irqrestore(&mapping->i_pages, flags);
> > >  }
> >
> > What about a shmem folio that's been moved to the swap cache?  I used
> > folio_index() here because I couldn't prove to my satisfaction that this
> > couldn't happen.
> 
> I just checked all callers of __folio_mark_dirty:
> 
> - block_dirty_folio
>     __folio_mark_dirty
> 
> - filemap_dirty_folio
>     __folio_mark_dirty
> 
> For these two, all their caller are from other fs not related to
> shmem/swap cache)
> 
> - mark_buffer_dirty
>     __folio_mark_dirty (mapping is folio->mapping)
> 
> - folio_redirty_for_writepage
>     filemap_dirty_folio
>       __folio_mark_dirty (mapping is folio->mapping)
> 
> For these two, __folio_mark_dirty is called with folio->mapping, and
> swap cache space is never set to folio->mapping. If the folio is a
> swap cache here, folio_index returns its swap cache index, which is
> not equal to its index in shmem or any other map, things will go very
> wrong.
> 
> And, currently both shmem / swap cache uses noop_dirty_folio, so they
> should never call into the helper here.

Yes, we've made quite a few changes around here and maybe it can't
happen now.

> I think I can add below sanity check here, just to clarify things and
> for debugging:
> 
> /*
>  * Shmem writeback relies on swap, and swap writeback
>  * is LRU based, not using the dirty mark.
>  */
> VM_WARN_ON(shmem_mapping(mapping) || folio_test_swapcache(folio))

That might be a good idea.

> And maybe we can also have a VM_WARN_ON for `folio->mapping != mapping` here?

I don't think that will work.  We can definitely see folio->mapping ==
NULL as the zap_page_range() path blocks folio freeing with the page
table lock rather than by taking the folio lock.  So truncation can
start but not complete (as it will wait on the PTL for mapped folios).

I think it's always true that folio->mapping will be either NULL or
equal to mapping, but maybe there's another case I've forgotten about.