Re: [PATCH v8 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: folio_mark_dropbehind() miscounts clean folios with stale dirty tag
--

commit 4468ce091bf80c4838cca1c8c9c9c92e78ab5c97
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 account it in WB_DONTCACHE_DIRTY when the folio's
PAGECACHE_TAG_DIRTY mark is set in mapping->i_pages.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1cc28224d547a..0c76ee5750b3b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1644,6 +1644,45 @@ 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, 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 has been counted: a dirtier past
> + * folio_test_set_dirty() but not yet counting finds the flag set and adds
> + * the folio itself.
> + */
> +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 (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 folio_mark_dropbehind() miscount a clean folio in WB_DONTCACHE_DIRTY when
a lingering PAGECACHE_TAG_DIRTY mark remains set in mapping->i_pages after
PG_dirty was cleared?

In fs/fs-writeback.c, inode_do_switch_wbs() notes that PAGECACHE_TAG_DIRTY
points to possibly dirty folios and therefore checks both PAGECACHE_TAG_DIRTY
and folio_test_dirty(folio) before transferring WB_RECLAIMABLE and
WB_DONTCACHE_DIRTY:

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

Neither of the two dirty-uncounting functions in mm/page-writeback.c clears
PAGECACHE_TAG_DIRTY in mapping->i_pages:

1. __folio_cancel_dirty() clears PG_dirty via folio_test_clear_dirty(folio)
   and calls folio_account_cleaned(), leaving PAGECACHE_TAG_DIRTY set in
   mapping->i_pages:

	if (folio_test_clear_dirty(folio))
		folio_account_cleaned(folio, wb);

   Callers such as try_to_free_buffers() (for example via
   block_invalidate_folio() or filemap_release_folio() when
   __remove_mapping() fails), nfs_wb_folio_cancel(),
   netfs_invalidate_folio(), and orangefs_invalidate_folio() can leave the
   folio in mapping->i_pages with PG_dirty cleared and PAGECACHE_TAG_DIRTY
   still set.

2. folio_clear_dirty_for_io() clears PG_dirty and subtracts
   WB_DONTCACHE_DIRTY while leaving PAGECACHE_TAG_DIRTY set for
   __folio_start_writeback() to clear:

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

   Paths that call folio_clear_dirty_for_io() and unlock the folio without
   calling folio_start_writeback(), such as iomap_writeback_folio() when
   iomap_writeback_handle_eof() returns false at or beyond i_size,
   block_write_full_folio(), mpage_release_unused_pages(),
   f2fs_write_single_data_page(), ceph_writepages_start(), and
   btrfs_folio_clear_fixup_dirty(), also leave a clean folio in
   mapping->i_pages with PAGECACHE_TAG_DIRTY set. When writeback later visits
   such a folio in writeback_get_folio() -> folio_prepare_writeback(), it
   returns false on !folio_test_dirty(folio) without clearing
   PAGECACHE_TAG_DIRTY.

If folio_mark_dropbehind() is called on a locked folio in that state,
xa_get_mark(&mapping->i_pages, folio->index, PAGECACHE_TAG_DIRTY) returns
true even though the folio is clean and not counted as dirty, so
WB_DONTCACHE_DIRTY is incremented by folio_nr_pages(folio).

If the caller then dirties the folio via folio_mark_dirty(),
filemap_dirty_folio() transitions PG_dirty from 0 to 1 and calls
__folio_mark_dirty() -> folio_account_dirtied(), which sees PG_dropbehind set
and adds folio_nr_pages(folio) to WB_DONTCACHE_DIRTY a second time, while
cleaning the folio later only subtracts it once. Or if the folio is evicted
or truncated without being dirtied again, neither folio_clear_dirty_for_io()
nor __folio_cancel_dirty() ever subtracts it because folio_test_clear_dirty()
is false, leaving WB_DONTCACHE_DIRTY permanently elevated.

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