Re: [PATCH v11 4/7] mm: add folio_mark_dropbehind() to mark a folio dropbehind and account it

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Nov 12, 2026 at 01:56:48AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Medium] mm: double WB_DONTCACHE_DIRTY accounting in
>   folio_mark_dropbehind()
> --
> 
> commit 99c7597afc26e4465bf2ced6117179f389f8476b
> Author: Mike Snitzer <snitzer@kernel.org>
> 
> mm: add folio_mark_dropbehind() to mark a folio dropbehind and account it
> 
> This commit adds folio_mark_dropbehind() to set PG_dropbehind on a locked
> folio and account it in WB_DONTCACHE_DIRTY if the folio is already dirty.
> 
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 98ce955404127..a54d0fbd37222 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1654,6 +1654,38 @@ void folio_end_dropbehind(struct folio *folio)
> >  }
> >  EXPORT_SYMBOL_GPL(folio_end_dropbehind);
> >  
> > +/**
> > + * folio_mark_dropbehind - mark an existing folio dropbehind
> > + * @folio: the folio, locked by the caller
> > + *
> > + * Mark @folio so that the writeback that cleans it drops it.  If the folio
> > + * is already counted as dirty, which with the i_pages lock held means both
> > + * PG_dirty and the dirty tag are set, add it to WB_DONTCACHE_DIRTY so that
> > + * the clean that subtracts it leaves the counter balanced.
> > + */
> > +void folio_mark_dropbehind(struct folio *folio)
> > +{
> > +	struct address_space *mapping = folio->mapping;
> > +	unsigned long flags;
> > +
> > +	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> > +
> > +	if (folio_test_dropbehind(folio))
> > +		return;
> > +	if (!mapping || !mapping_can_writeback(mapping)) {
> > +		folio_set_dropbehind(folio);
> > +		return;
> > +	}
> > +	xa_lock_irqsave(&mapping->i_pages, flags);
> > +	folio_set_dropbehind(folio);
> > +	if (folio_test_dirty(folio) &&
> > +	    xa_get_mark(&mapping->i_pages, folio->index, PAGECACHE_TAG_DIRTY))
> > +		wb_stat_mod(inode_to_wb(mapping->host), WB_DONTCACHE_DIRTY,
> > +			    folio_nr_pages(folio));
> > +	xa_unlock_irqrestore(&mapping->i_pages, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(folio_mark_dropbehind);
> 
> [Severity: Medium]
> Can this double-account WB_DONTCACHE_DIRTY when racing with an unlocked
> dirtier on a folio that still has a leftover PAGECACHE_TAG_DIRTY mark?
> 
> __folio_cancel_dirty() clears PG_dirty and uncounts the folio without
> clearing PAGECACHE_TAG_DIRTY in mapping->i_pages.  Because callers such as
> zap_present_folio_ptes() -> folio_mark_dirty() -> filemap_dirty_folio()
> dirty mapped folios under the PTE lock without holding the folio lock,
> they can set PG_dirty before acquiring mapping->i_pages.xa_lock:
> 
> CPU0 (holds folio lock)           CPU1 (holds PTE lock)
> __folio_cancel_dirty(folio)
>   folio_test_clear_dirty(folio)
>   folio_account_cleaned(folio)
>   /* PAGECACHE_TAG_DIRTY stays set */
>                                   filemap_dirty_folio(mapping, folio)
>                                     folio_test_set_dirty(folio)
> folio_mark_dropbehind(folio)
>   xa_lock_irqsave(&mapping->i_pages)
>   folio_set_dropbehind(folio)
>   /* PG_dirty && TAG_DIRTY both set */
>   wb_stat_mod(..., WB_DONTCACHE_DIRTY, +nr)
>   xa_unlock_irqrestore(&mapping->i_pages)
>                                     __folio_mark_dirty(folio, mapping, ...)
>                                       xa_lock_irqsave(&mapping->i_pages)
>                                       folio_account_dirtied(folio, mapping)
>                                         if (folio_test_dropbehind(folio))
>                                           wb_stat_mod(...,
>                                             WB_DONTCACHE_DIRTY, +nr)
>                                             <-- counted twice
> 
> When the folio is later cleaned, folio_clear_dirty_for_io() or
> folio_account_cleaned() only subtracts folio_nr_pages(folio) once,
> leaving WB_DONTCACHE_DIRTY permanently inflated and causing
> wb_check_start_dontcache() to trigger extra writeback.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261111015607.46338-1-snitzer@kernel.org?part=4

This is the third time sashiko has raised this race: as v9 3/6, as v10
4/7 and now as v11 4/7.

But I've finally fixed it, I now have a Kunit test that reproduces this
race and have verified it is now fixed with the following changes.

Will post v12 shortly.

folio_mark_dropbehind() will now return early for mapped folios, here is
the incremental change on v11 4/7 that I'll be folding into v12's 4/7:

  @@ -1651,7 +1651,8 @@ EXPORT_SYMBOL_GPL(folio_end_dropbehind);
    * Mark @folio so that the writeback that cleans it drops it.  If the folio
    * is already counted as dirty, which with the i_pages lock held means both
    * PG_dirty and the dirty tag are set, add it to WB_DONTCACHE_DIRTY so that
  - * the clean that subtracts it leaves the counter balanced.
  + * the clean that subtracts it leaves the counter balanced.  A mapped folio
  + * is left unmarked.
    */
   void folio_mark_dropbehind(struct folio *folio)
   {
  @@ -1662,6 +1663,15 @@ void folio_mark_dropbehind(struct folio *folio)

        if (folio_test_dropbehind(folio))
                return;
  +     /*
  +      * Through its page table, a mapped folio can be dirtied without the
  +      * folio lock, and PG_dirty set before the folio is counted: with a
  +      * stale dirty tag it would be counted twice.  The folio lock keeps
  +      * an unmapped folio unmapped.  filemap_end_dropbehind() does not drop
  +      * a mapped folio either.
  +      */
  +     if (folio_mapped(folio))
  +             return;
        if (!mapping || !mapping_can_writeback(mapping)) {
                folio_set_dropbehind(folio);
                return;

And for 6/7, filemap_dontcache_edge_complete() will skip every mapped
folio -- not just clean ones.  Incremental that'll be folded into v12's 6/7:

  @@ -1757,8 +1767,8 @@ EXPORT_SYMBOL_GPL(filemap_dontcache_edge_completes);
    * @pos: any byte offset within the folio
    *
    * Call once the write, and any sync of it, is done.  A dirty folio is marked
  - * for the writeback that cleans it, and a clean unmapped folio is dropped now.
  - * A clean mapped folio stays cached, as filemap_end_dropbehind() leaves it.
  + * for the writeback that cleans it, and a clean folio is dropped now.  A
  + * mapped folio stays cached, as folio_mark_dropbehind() leaves it.
    */
   void filemap_dontcache_edge_complete(struct address_space *mapping, loff_t pos)
   {
  @@ -1773,10 +1783,12 @@ void filemap_dontcache_edge_complete(struct address_space *mapping, loff_t pos)
                goto out;
        /* Writeback in flight was submitted unmarked and would not drop it. */
        folio_wait_writeback(folio);
  +     if (folio_mapped(folio))
  +             goto out;
        if (folio_test_dirty(folio)) {
                folio_mark_dropbehind(folio);
                kick = true;
  -     } else if (!folio_mapped(folio)) {
  +     } else {
                /*
                 * Drop it now, unmarked: a mark cleared outside the i_pages
                 * lock could miss a store that counts the folio.

Thanks,
Mike