Re: [PATCH v10 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(s) to consider:
- [Medium] mm: double WB_DONTCACHE_DIRTY accounting in folio_mark_dropbehind()
--

commit 22fee9ca43388b4bd59c934777df4d9990967b8e
Author: Mike Snitzer <snitzer@kernel.org>

mm: add folio_mark_dropbehind() to mark a folio dropbehind and account it

This commit introduces folio_mark_dropbehind() to mark a locked folio
with PG_dropbehind and increment WB_DONTCACHE_DIRTY under the i_pages
lock when both PG_dirty and PAGECACHE_TAG_DIRTY are set.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 3a13289c07dc4..5134227ba3a23 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1643,6 +1643,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);
> +}

[Severity: Medium]
As reported by Sashiko on v9 [1], can WB_DONTCACHE_DIRTY be double-counted
in folio_mark_dropbehind() when a stale PAGECACHE_TAG_DIRTY mark coincides
with an unlocked dirtier that has just set PG_dirty?

Specifically, PAGECACHE_TAG_DIRTY can remain set in mapping->i_pages on a
clean folio after __folio_cancel_dirty() clears PG_dirty and calls
folio_account_cleaned():

mm/page-writeback.c:__folio_cancel_dirty() {
	...
		if (folio_test_clear_dirty(folio))
			folio_account_cleaned(folio, wb);
	...
}

(or after folio_clear_dirty_for_io() clears PG_dirty and writeback exits
early before calling folio_start_writeback()).

If an unlocked dirtier that does not hold the folio lock (such as
zap_present_folio_ptes() -> folio_mark_dirty() -> filemap_dirty_folio()
under the PTE lock) then runs concurrently, it sets PG_dirty before
acquiring mapping->i_pages.xa_lock in __folio_mark_dirty():

mm/page-writeback.c:filemap_dirty_folio() {
	folio_ clamp/test:
	if (folio_test_set_dirty(folio))
		return false;

	__folio_mark_dirty(folio, mapping, !folio_has_attached_private(folio));
}

If folio_mark_dropbehind() acquires mapping->i_pages.xa_lock in that
window, it sets PG_dropbehind, observes both folio_test_dirty(folio) and
the stale PAGECACHE_TAG_DIRTY mark as true, and adds folio_nr_pages(folio)
to WB_DONTCACHE_DIRTY.

When __folio_mark_dirty() subsequently acquires mapping->i_pages.xa_lock,
folio_account_dirtied() sees PG_dropbehind set and adds
folio_nr_pages(folio) to WB_DONTCACHE_DIRTY a second time:

mm/page-writeback.c:folio_account_dirtied() {
	...
		lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, nr);
		if (folio_test_dropbehind(folio))
			wb_stat_mod(wb, WB_DONTCACHE_DIRTY, nr);
}

When the folio is later cleaned, WB_DONTCACHE_DIRTY is only decremented
once, leaving a permanent positive drift in the counter.

[1] https://lore.kernel.org/r/sashiko-outbox-184464@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103211754.35352-1-snitzer@kernel.org?part=4