Re: [PATCH v9 3/6] 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 c61c906e71a09ef6325d33136e8bd76033c66585
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 an existing
locked folio and increment WB_DONTCACHE_DIRTY when the folio is already
counted as dirty. To decide whether the folio has been counted, it checks
both PG_dirty and PAGECACHE_TAG_DIRTY under the mapping->i_pages lock.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 5176c4125a4db..d0e4d1788fd40 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1644,6 +1644,48 @@ 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.  A folio
> + * already counted as dirty is added to WB_DONTCACHE_DIRTY, as
> + * folio_account_dirtied() would have added it had it been marked when
> + * dirtied, so that the clean that subtracts it leaves the counter balanced.
> + *
> + * folio_account_dirtied() counts a folio under the i_pages lock, in the
> + * section that sets its dirty tag, after PG_dirty has been set without the
> + * folio lock.  Every path that uncounts a dirty folio holds the folio lock.
> + * So with the folio locked and the i_pages lock held, the folio is counted
> + * when both PG_dirty and the dirty tag are set.  PG_dirty alone is not
> + * enough: a dirtier past folio_test_set_dirty() but not yet counting finds
> + * the flag set and adds the folio itself.  The tag alone is not enough
> + * either: __folio_cancel_dirty() uncounts a folio and clears PG_dirty, but
> + * leaves the tag until writeback or truncation clears it.
> + */
> +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]
Can WB_DONTCACHE_DIRTY still be double-counted here if a stale
PAGECACHE_TAG_DIRTY coincides with an unlocked dirtier that has just set
PG_dirty?

The comment explains two separate hazards:
1. __folio_cancel_dirty() (or folio_clear_dirty_for_io() when writeback exits
   early before folio_start_writeback(), such as in
   iomap_writeback_folio() -> iomap_writeback_handle_eof()) clears PG_dirty
   and uncounts the folio while leaving PAGECACHE_TAG_DIRTY set in
   mapping->i_pages.
2. An unlocked dirtier that does not hold folio_lock (for example,
   zap_present_folio_ptes() or follow_page_pte() calling folio_mark_dirty()
   under the PTE lock, or mark_buffer_dirty()) sets PG_dirty before
   acquiring mapping->i_pages.xa_lock:

mm/page-writeback.c:filemap_dirty_folio() {
	folio_Batch /unlocked caller
	if (folio_test_set_dirty(folio))
		return false;

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

If both happen on the same folio, PAGECACHE_TAG_DIRTY is already set from the
prior __folio_cancel_dirty() or folio_clear_dirty_for_io(), and then an
unlocked dirtier sets PG_dirty via folio_test_set_dirty(folio) before taking
mapping->i_pages.xa_lock in __folio_mark_dirty().

If folio_mark_dropbehind() runs in that window, it acquires
mapping->i_pages.xa_lock, sets PG_dropbehind, sees both
folio_test_dirty(folio) and PAGECACHE_TAG_DIRTY set, and adds
folio_nr_pages(folio) to WB_DONTCACHE_DIRTY.

When the unlocked dirtier then acquires mapping->i_pages.xa_lock in
__folio_mark_dirty() -> folio_account_dirtied():

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);
}

it sees folio_test_dropbehind(folio) is now true and adds
folio_nr_pages(folio) to WB_DONTCACHE_DIRTY a second time, while cleaning the
folio will only subtract it once. (And symmetrically, wouldn't
filemap_clear_dropbehind() in the same window subtract WB_DONTCACHE_DIRTY
before folio_account_dirtied() has added it, while folio_account_dirtied()
then sees PG_dropbehind cleared and never adds it back?)

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