Re: [PATCH 2/2] mm, iomap: account dirty at page granularity for per-block dirty folios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] iomap: dirty counter leak on early EOF return in iomap_writeback_folio()
- [High] ntfs, ntfs3: dirty counter leak in ntfs_mft_writepages() and ntfs_writepages()
- [High] block: missing iomap per-block dirty callbacks in def_blk_aops
- [Medium] mm: wrong wb stats in inode_do_switch_wbs() and __filemap_get_folio_mpol()
- [Medium] gfs2: stale AS_DIRTY_PER_BLOCK flag after switching to gfs2_jdata_aops
--

commit ec03cb36d468351e028d982f93868e296ff14cfc
Author: Salvatore Dipietro <dipiets@amazon.it>

mm, iomap: account dirty at page granularity for per-block dirty folios

This commit updates iomap to account mm and writeback dirty page counters at
page granularity rather than full folio granularity when folios carry
per-block dirty state in struct iomap_folio_state, and wires up
.accounted_dirty_pages = iomap_accounted_dirty_pages across iomap-based
filesystems.

> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 0a7b8076af3a5..ea77af00b111e 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -713,6 +713,7 @@ static const struct address_space_operations gfs2_aops = {
>  	.read_folio = gfs2_read_folio,
>  	.readahead = gfs2_readahead,
>  	.dirty_folio = iomap_dirty_folio,
> +	.accounted_dirty_pages = iomap_accounted_dirty_pages,

[Severity: Medium]
What happens if a GFS2 file using gfs2_aops with blocksize < folio_size()
allocates struct iomap_folio_state via ifs_alloc() (which sets
AS_DIRTY_PER_BLOCK on inode->i_mapping), and later has GFS2_DIF_JDATA
enabled via gfs2_fileattr_set() -> do_gfs2_set_flags() -> gfs2_set_aops()?

fs/gfs2/aops.c:gfs2_set_aops() {
	if (gfs2_is_jdata(GFS2_I(inode)))
		inode->i_mapping->a_ops = &gfs2_jdata_aops;
	else
		inode->i_mapping->a_ops = &gfs2_aops;
}

Switching inode->i_mapping->a_ops to gfs2_jdata_aops leaves
AS_DIRTY_PER_BLOCK set on inode->i_mapping. Subsequent writes in jdata mode
attach struct buffer_head rings to folio->private and charge
folio_nr_pages(folio) via gfs2_jdata_dirty_folio() -> block_dirty_folio().

When gfs2_jdata_writepages() later runs folio_clear_dirty_for_io():

mm/page-writeback.c:folio_clear_dirty_for_io() {
    ...
		if (folio_test_clear_dirty(folio)) {
			if (!mapping_dirty_per_block(mapping) || !folio->private) {
				long nr = folio_nr_pages(folio);
    ...
}

Because mapping_dirty_per_block(mapping) is still true and folio->private is
non-NULL (pointing to buffer_heads), won't folio_clear_dirty_for_io() skip
discharging NR_FILE_DIRTY, WB_DONTCACHE_DIRTY, NR_ZONE_WRITE_PENDING, and
WB_RECLAIMABLE on every writeback?

> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 0a5ebfda90f12..75671b29f7cce 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c

[ ... ]

> @@ -180,6 +189,89 @@ static unsigned iomap_find_dirty_range(struct folio *folio, u64 *range_start,

[ ... ]

> +static void iomap_account_dirty_pages(struct folio *folio, long nr_pages,
> +		bool new_dirty)
> +{
> +	struct address_space *mapping = folio->mapping;
> +	struct inode *inode = mapping->host;
> +	struct wb_lock_cookie cookie = {};
> +	struct bdi_writeback *wb;
> +	bool dirtied = nr_pages > 0 && new_dirty;
> +
> +	if (!nr_pages || !mapping_can_writeback(mapping))
> +		return;
> +
> +	lruvec_stat_mod_folio(folio, NR_FILE_DIRTY, nr_pages);
> +	zone_stat_mod_folio(folio, NR_ZONE_WRITE_PENDING, nr_pages);
> +	if (dirtied)
> +		node_stat_mod_folio(folio, NR_DIRTIED, nr_pages);
> +
> +	/* Pin the wb against a concurrent inode_do_switch_wbs(). */
> +	inode_attach_wb(inode, folio);
> +	wb = unlocked_inode_to_wb_begin(inode, &cookie);
> +	wb_stat_mod(wb, WB_RECLAIMABLE, nr_pages);
> +	if (folio_test_dropbehind(folio))
> +		wb_stat_mod(wb, WB_DONTCACHE_DIRTY, nr_pages);
> +	if (dirtied)
> +		wb_stat_mod(wb, WB_DIRTIED, nr_pages);
> +	unlocked_inode_to_wb_end(inode, &cookie);

[Severity: Medium]
Can charging WB_RECLAIMABLE and WB_DONTCACHE_DIRTY by nr_pages here corrupt
per-wb counters when inode_do_switch_wbs() or __filemap_get_folio_mpol()
runs on a partially dirty large folio?

In fs/fs-writeback.c, inode_do_switch_wbs() still transfers
folio_nr_pages(folio) instead of folio_accounted_dirty_pages(folio):

fs/fs-writeback.c:inode_do_switch_wbs() {
    ...
	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);
			}
		}
	}
    ...
}

When new_wb later writes back or invalidates the folio,
iomap_clear_range_dirty() only subtracts ifs->dirty_pages from new_wb,
leaving old_wb underflowed and new_wb inflated by
folio_nr_pages(folio) - ifs->dirty_pages.

Also, because iomap_account_dirty_pages() updates WB_RECLAIMABLE and
WB_DONTCACHE_DIRTY outside mapping->i_pages.xa_lock before
__filemap_dirty_folio() sets PG_dirty, and because
folio_clear_dirty_for_io() clears PG_dirty while keeping WB_RECLAIMABLE and
WB_DONTCACHE_DIRTY charged until iomap_clear_range_dirty(), can a concurrent
inode_do_switch_wbs() miss or misaccount a folio whose PG_dirty state does
not match its active wb charge?

Similarly, in mm/filemap.c, __filemap_get_folio_mpol() decrements
WB_DONTCACHE_DIRTY by folio_nr_pages(folio) when clearing PG_dropbehind:

mm/filemap.c:__filemap_get_folio_mpol() {
    ...
	if (!(fgp_flags & FGP_DONTCACHE) && folio_test_clear_dropbehind(folio)) {
		if (folio_test_dirty(folio) &&
		    mapping_can_writeback(mapping)) {
			struct inode *inode = mapping->host;
			struct bdi_writeback *wb;
			struct wb_lock_cookie cookie = {};
			long nr = folio_nr_pages(folio);

			wb = unlocked_inode_to_wb_begin(inode, &cookie);
			wb_stat_mod(wb, WB_DONTCACHE_DIRTY, -nr);
			unlocked_inode_to_wb_end(inode, &cookie);
		}
	}
    ...
}

Won't that underflow WB_DONTCACHE_DIRTY when a partially dirty folio was
only charged ifs->dirty_pages, or leak WB_DONTCACHE_DIRTY if PG_dropbehind
is cleared after folio_clear_dirty_for_io() cleared PG_dirty but before
iomap_clear_range_dirty() runs?

[ ... ]

> @@ -276,9 +396,13 @@ static struct iomap_folio_state *ifs_alloc(struct inode *inode,
>  	spin_lock_init(&ifs->state_lock);
>  	if (folio_test_uptodate(folio))
>  		bitmap_set(ifs->state, 0, nr_blocks);
> -	if (folio_test_dirty(folio))
> +	if (folio_test_dirty(folio)) {
>  		bitmap_set(ifs->state, nr_blocks, nr_blocks);
> +		ifs->dirty_pages = folio_nr_pages(folio);
> +	}
>  	folio_attach_private(folio, ifs);
> +	/* The mm dirty counters are now charged per dirty page for this file. */
> +	mapping_set_dirty_per_block(inode->i_mapping);

[Severity: High]
What happens on block devices when !CONFIG_BUFFER_HEAD is enabled?

In block/fops.c, def_blk_aops uses iomap for buffered I/O, writepages,
release_folio, and invalidate_folio when !CONFIG_BUFFER_HEAD:

block/fops.c:
const struct address_space_operations def_blk_aops = {
	.dirty_folio	= filemap_dirty_folio,
	.release_folio		= iomap_release_folio,
	.invalidate_folio	= iomap_invalidate_folio,
	.read_folio		= blkdev_read_folio,
	.readahead		= blkdev_readahead,
	.writepages		= blkdev_writepages,
	.is_partially_uptodate  = iomap_is_partially_uptodate,
	.error_remove_folio	= generic_error_remove_folio,
	.migrate_folio		= filemap_migrate_folio,
};

When ifs_alloc() attaches ifs to a block device folio with
i_blocks_per_folio(inode, folio) > 1, it sets AS_DIRTY_PER_BLOCK on the
block device mapping, but def_blk_aops does not set
.accounted_dirty_pages = iomap_accounted_dirty_pages or
.dirty_folio = iomap_dirty_folio.

When iomap_invalidate_folio(folio, 0, folio_size(folio)) later runs on a
dirty block device folio, it first calls iomap_clear_range_dirty() (which
discharges ifs->dirty_pages and sets ifs->dirty_pages = 0) and then calls
folio_cancel_dirty() -> folio_account_cleaned() ->
folio_accounted_dirty_pages(). Because mapping->a_ops->accounted_dirty_pages
is NULL, folio_accounted_dirty_pages() returns folio_nr_pages(folio) instead
of 0, discharging the counters a second time.

Also, if a block device folio that already has ifs attached from a read is
dirtied via mmap (filemap_dirty_folio()), the dirty bits in ifs->state are
not set, so writeback via folio_clear_dirty_for_io() and
iomap_writeback_folio() neither writes the blocks nor discharges the dirty
counters. Should def_blk_aops also use iomap_dirty_folio and
iomap_accounted_dirty_pages when !CONFIG_BUFFER_HEAD?

[ ... ]

> @@ -2001,8 +2151,13 @@ int iomap_writeback_folio(struct iomap_writepage_ctx *wpc, struct folio *folio)
>  
>  	if (i_blocks_per_folio(inode, folio) > 1) {
>  		if (!ifs) {
> +			/*
> +			 * Dirtied before it had per-block state, so charged
> +			 * per-folio.  Re-mark the blocks for the walk below,
> +			 * paired with the clear at the end of this function.
> +			 */
>  			ifs = ifs_alloc(inode, folio, 0);
> -			iomap_set_range_dirty(folio, 0, end_pos - pos);
> +			iomap_redirty_range(folio, 0, end_pos - pos);
>  		}
>  
>  		iomap_writeback_init(inode, folio);

[Severity: High]
Can iomap_writeback_folio() leak the dirty page counters when a folio with
per-block state is at or beyond EOF?

Before calling iomap_writeback_folio() (via writeback_iter() ->
folio_prepare_writeback() or fuse_launder_folio()), the caller invokes
folio_clear_dirty_for_io(folio), which clears PG_dirty without discharging
NR_FILE_DIRTY, WB_DONTCACHE_DIRTY, NR_ZONE_WRITE_PENDING, or WB_RECLAIMABLE
when mapping_dirty_per_block(mapping) && folio->private is true.

However, at the start of iomap_writeback_folio():

fs/iomap/buffered-io.c:iomap_writeback_folio() {
    ...
	if (!iomap_writeback_handle_eof(folio, wpc, &end_pos))
		return 0;
    ...
}

If iomap_writeback_handle_eof() returns false (for instance, when writeback
races with truncate_setsize() after i_size_write() lowers i_size but before
truncate_pagecache() locks the folio, or in folio_unmap_invalidate() ->
fuse_launder_folio()), iomap_writeback_folio() returns 0 without calling
iomap_clear_range_dirty(folio, 0, folio_size(folio)).

That leaves the folio clean (PG_dirty == false, PG_writeback == false) while
ifs->dirty_pages remains charged. If filemap_release_folio() ->
iomap_release_folio() then runs (immediately in folio_unmap_invalidate(), or
via reclaim, splitting, or eviction before truncate_cleanup_folio() locks
the folio):

fs/iomap/buffered-io.c:iomap_release_folio() {
    ...
	if (folio_test_dirty(folio))
		return false;
	ifs_free(folio);
	return true;
}

Won't iomap_release_folio() see folio_test_dirty(folio) == false and free
ifs without ever discharging ifs->dirty_pages?

> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index 517d9a1563e7d..d26ddd5ae1185 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -308,6 +308,7 @@ const struct address_space_operations ntfs_aops = {
>  	.readahead		= ntfs_readahead,
>  	.writepages		= ntfs_writepages,
>  	.dirty_folio		= iomap_dirty_folio,
> +	.accounted_dirty_pages	= iomap_accounted_dirty_pages,
>  	.bmap			= ntfs_bmap,
>  	.migrate_folio		= filemap_migrate_folio,
>  	.is_partially_uptodate	= iomap_is_partially_uptodate,
> @@ -322,6 +323,7 @@ const struct address_space_operations ntfs_mft_aops = {
>  	.readahead		= ntfs_readahead,
>  	.writepages		= ntfs_mft_writepages,
>  	.dirty_folio		= iomap_dirty_folio,
> +	.accounted_dirty_pages	= iomap_accounted_dirty_pages,
>  	.bmap			= ntfs_bmap,
>  	.migrate_folio		= filemap_migrate_folio,
>  	.is_partially_uptodate	= iomap_is_partially_uptodate,
> diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c
> index 4ac26c80bd34f..509570091e483 100644
> --- a/fs/ntfs3/inode.c
> +++ b/fs/ntfs3/inode.c
> @@ -2301,6 +2301,7 @@ const struct address_space_operations ntfs_aops = {
>  	.writepages	= ntfs_writepages,
>  	.bmap		= ntfs_bmap,
>  	.dirty_folio	= iomap_dirty_folio,
> +	.accounted_dirty_pages = iomap_accounted_dirty_pages,

[Severity: High]
Does enabling per-block dirty accounting on ntfs_mft_aops (in fs/ntfs) and
resident files using ntfs_aops (in fs/ntfs3) leak dirty page counters during
writeback?

In fs/ntfs, ntfs_mft_aops uses ntfs_mft_writepages(), which iterates with
writeback_iter() and calls ntfs_write_mft_block(folio, wbc) rather than
iomap_writeback_folio():

fs/ntfs/mft.c:ntfs_mft_writepages() {
    ...
	while ((folio = writeback_iter(mapping, wbc, folio, &error)))
		error = ntfs_write_mft_block(folio, wbc);
    ...
}

Similarly, in fs/ntfs3, ntfs_writepages() handles resident files via
writeback_iter() and attr_data_write_resident(ni, folio) without calling
iomap_writepages():

fs/ntfs3/inode.c:ntfs_writepages() {
    ...
	if (is_resident(ni)) {
		struct folio *folio = NULL;
		err = 0;

		while ((folio = writeback_iter(mapping, wbc, folio, &err))) {
			int err2;

			ni_lock(ni);
			err2 = attr_data_write_resident(ni, folio);
			ni_unlock(ni);
    ...
}

When these folios are dirtied via iomap_dirty_folio() (such as
ntfs_mft_mark_dirty() on $MFT or mmap writes on a resident file),
ifs_alloc() attaches ifs and sets AS_DIRTY_PER_BLOCK, and
iomap_set_range_dirty() charges ifs->dirty_pages. During writeback,
writeback_iter() calls folio_clear_dirty_for_io(), which skips discharging
the dirty counters because mapping_dirty_per_block(mapping) &&
folio->private is true, and neither ntfs_write_mft_block() nor
attr_data_write_resident() clears ifs dirty state or discharges
ifs->dirty_pages.

Won't this permanently leak NR_FILE_DIRTY, NR_ZONE_WRITE_PENDING,
WB_RECLAIMABLE, and WB_DONTCACHE_DIRTY on writeback, and prevent subsequent
calls to iomap_dirty_folio() on the same folio from charging the counters
because all dirty bits in ifs->state remain set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027112702.3747009-1-dipiets@amazon.it?part=2