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

[sashiko-bot@kernel.org]

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