Re: [PATCH v8 2/7] mm: take a folio off WB_DONTCACHE_DIRTY only if it was counted there

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm: double-decrement of WB_DONTCACHE_DIRTY in filemap_clear_dropbehind()
--

commit 4fb0b9f6bc322dc01cdee84f7076d22537c5bd27
Author: Mike Snitzer <snitzer@kernel.org>

mm: take a folio off WB_DONTCACHE_DIRTY only if it was counted there

This commit introduces filemap_clear_dropbehind() to clear PG_dropbehind
and decrement WB_DONTCACHE_DIRTY under the folio lock and i_pages lock
when a cached lookup without FGP_DONTCACHE finds a dropbehind folio.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 00fd89cf6f550..1cc28224d547a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1927,6 +1927,45 @@ void *filemap_get_entry(struct address_space *mapping, pgoff_t index)
>  	return folio;
>  }
>  
> +/*
> + * Clear the uncached flag of a folio a cached lookup found, and take the
> + * folio off WB_DONTCACHE_DIRTY if it is counted there.
> + *
> + * folio_account_dirtied() counts a dropbehind folio under the i_pages lock,
> + * in the section that sets its dirty tag, after PG_dirty has been set
> + * without the folio lock, and every path that uncounts a dirty folio holds
> + * the folio lock.  So with the folio locked and the i_pages lock held, the
> + * dirty tag, not PG_dirty, says whether the folio is counted: a dirtier past
> + * folio_test_set_dirty() but not yet counting finds the flag clear and does
> + * not count it.  A folio another task has locked keeps the flag, as it
> + * would have had this lookup not run.
> + */
> +static void filemap_clear_dropbehind(struct address_space *mapping,
> +				     struct folio *folio, bool locked)
> +{
> +	unsigned long flags;
> +
> +	if (!locked) {
> +		if (!folio_trylock(folio))
> +			return;
> +		if (unlikely(folio->mapping != mapping))
> +			goto out;
> +	}
> +	if (!mapping_can_writeback(mapping)) {
> +		folio_clear_dropbehind(folio);
> +		goto out;
> +	}
> +	xa_lock_irqsave(&mapping->i_pages, flags);
> +	if (folio_test_clear_dropbehind(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 testing only PAGECACHE_TAG_DIRTY without folio_test_dirty(folio) in
filemap_clear_dropbehind() decrement WB_DONTCACHE_DIRTY a second time for
a folio that was already uncounted?

In fs/fs-writeback.c:inode_do_switch_wbs(), the PAGECACHE_TAG_DIRTY walk
checks folio_test_dirty(folio) before moving WB_RECLAIMABLE and
WB_DONTCACHE_DIRTY because PAGECACHE_TAG_DIRTY remains set in
mapping->i_pages after several paths clear PG_dirty and uncount the folio:

	xas_for_each_marked(&xas, folio, ULONG_MAX, PAGECACHE_TAG_DIRTY) {
		if (folio_test_dirty(folio)) {
			long nr = folio_nr_pages(folio);
			wb_stat_mod(old_wb, WB_RECLAIMABLE, -nr);
			wb_stat_mod(new_wb, WB_RECLAIMABLE, nr);
			if (folio_test_dropbehind(folio)) {
				wb_stat_mod(old_wb, WB_DONTCACHE_DIRTY, -nr);
				wb_stat_mod(new_wb, WB_DONTCACHE_DIRTY, nr);
			}
		}
	}

For example, during writeback,
writeback_iter()->writeback_get_folio()->folio_prepare_writeback() calls
folio_clear_dirty_for_io(), which clears PG_dirty and subtracts the folio
from WB_DONTCACHE_DIRTY while leaving PAGECACHE_TAG_DIRTY set in
mapping->i_pages (normally cleared later by folio_start_writeback()):

mm/page-writeback.c:folio_clear_dirty_for_io() {
	...
	if (folio_test_clear_dirty(folio)) {
		long nr = folio_nr_pages(folio);
		lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, -nr);
		if (folio_test_dropbehind(folio))
			wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);
	...
}

If writeback races with truncate_setsize() lowering i_size, writeback
handlers such as iomap_writepages()->iomap_writeback_folio() see that the
folio is at or past i_size via iomap_writeback_handle_eof() and return
early without calling folio_start_writeback(). The folio is then unlocked
in mapping->i_pages with PG_dirty clear, PG_dropbehind still set, and
PAGECACHE_TAG_DIRTY still set. When truncate_inode_pages_range() or
another non-FGP_DONTCACHE caller subsequently looks up the folio via
__filemap_get_folio_mpol(), filemap_clear_dropbehind() will see
PAGECACHE_TAG_DIRTY set and subtract the folio from WB_DONTCACHE_DIRTY a
second time.

Similarly, mm/page-writeback.c:__folio_cancel_dirty() clears PG_dirty and
calls folio_account_cleaned(), subtracting WB_DONTCACHE_DIRTY without
clearing PAGECACHE_TAG_DIRTY or PG_dropbehind:

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

When folio_cancel_dirty() is called on a folio that stays in the page
cache (for instance via filemap_release_folio()->try_to_free_buffers()
when reclaim or migration fails to remove the folio, or during partial
folio invalidation), PAGECACHE_TAG_DIRTY remains set on the clean
dropbehind folio. Furthermore, if such a folio with a lingering
PAGECACHE_TAG_DIRTY mark is later dirtied without the folio lock via
filemap_dirty_folio(), both PG_dirty and PAGECACHE_TAG_DIRTY are set
before __folio_mark_dirty() takes the i_pages lock to call
folio_account_dirtied().

Should filemap_clear_dropbehind() also require folio_test_dirty(folio), or
should paths that clear PG_dirty and uncount WB_DONTCACHE_DIRTY without
starting writeback also clear PAGECACHE_TAG_DIRTY (or PG_dropbehind)?

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