Re: [PATCH v8 5/7] mm, iomap: keep a partly covered page until an uncached write completes it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm, iomap: missing dropbehind in __filemap_get_folio_mpol() for sub-page blocks
- [Medium] mm/filemap: WB_DONTCACHE_DIRTY imbalance in folio_mark_dropbehind()
--

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

mm, iomap: keep a partly covered page until an uncached write completes it

This commit introduces FGP_DONTCACHE_EDGE so that uncached buffered writes
covering only part of a minimum-sized folio leave a newly created folio
without PG_dropbehind, and instead mark PG_dropbehind when a subsequent
neighbouring uncached write finds the folio uptodate and completes it.

> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 0a5ebfda90f12..86f884c9f8f22 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -802,8 +802,22 @@ struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos, size_t len)
>  
>  	if (iter->flags & IOMAP_NOWAIT)
>  		fgp |= FGP_NOWAIT;
> -	if (iter->flags & IOMAP_DONTCACHE)
> +	if (iter->flags & IOMAP_DONTCACHE) {
> +		size_t unit = mapping_min_folio_nrbytes(iter->inode->i_mapping);
> +		size_t off = pos & (unit - 1);
> +
>  		fgp |= FGP_DONTCACHE;
> +		/*
> +		 * The smallest folio the mapping allows, a page or a block
> +		 * where the block is larger, that this write covers only
> +		 * partly is shared with a neighbouring write: get it as just
> +		 * that, which is kept until the write that completes it.
> +		 */
> +		if (off || len < unit) {
> +			fgp |= FGP_DONTCACHE_EDGE;
> +			len = min_t(size_t, len, unit - off);
> +		}
> +	}

[Severity: Medium]
When the filesystem block size is smaller than PAGE_SIZE (for example, 4 KiB
blocks on a 16 KiB or 64 KiB page kernel, or sub-page blocks on a 4 KiB page
kernel), can folios written with RWF_DONTCACHE in sub-page chunks miss
getting marked with PG_dropbehind and remain in the page cache until memory
reclaim?

In iomap_get_folio(), unit is mapping_min_folio_nrbytes(), which is at least
PAGE_SIZE. Any uncached write with off != 0 or len < unit sets
FGP_DONTCACHE_EDGE, so __filemap_get_folio_mpol() allocates the folio
without setting PG_dropbehind.

When i_blocksize(inode) < PAGE_SIZE, __iomap_write_begin() attaches struct
iomap_folio_state and only zeroes or reads blocks outside the block-aligned
range being written. Then __iomap_write_end() calls
iomap_set_range_uptodate():

fs/iomap/buffered-io.c:iomap_set_range_uptodate() {
	if (ifs) {
		spin_lock_irqsave(&ifs->state_lock, flags);
		mark_uptodate = ifs_set_range_uptodate(folio, ifs, off, len) &&
				!ifs->read_bytes_pending;
		spin_unlock_irqrestore(&ifs->state_lock, flags);
	}

	if (mark_uptodate)
		folio_mark_uptodate(folio);
}

Whenever at least one sub-page block in the folio remains unwritten after
the first write, ifs_set_range_uptodate() returns false and PG_uptodate
stays clear on the folio.

When the completing RWF_DONTCACHE write arrives for the remaining block(s)
of that folio, iomap_write_begin() -> iomap_get_folio() ->
__filemap_get_folio_mpol() (and filemap_dontcache_edge_completes() in the
subsequent commit "mm, iomap: let an uncached writer complete its shared
pages itself") checks folio_test_uptodate(folio) before
__iomap_write_begin() and __iomap_write_end() run for the completing write.
Because the blocks about to be written by the completing write are not yet
uptodate in ifs->state, folio_test_uptodate(folio) is still false when the
folio is looked up, so folio_mark_dropbehind(folio) is skipped before
__iomap_write_end() finally sets PG_uptodate.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 0c76ee5750b3b..1494e42e609fa 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c

[ ... ]

> @@ -2063,6 +2075,19 @@ struct folio *__filemap_get_folio_mpol(struct address_space *mapping,
>  
>  	if (fgp_flags & FGP_STABLE)
>  		folio_wait_stable(folio);
> +
> +	/*
> +	 * An uncached write that covers this folio only partly, finding it
> +	 * uptodate, is the write that completes it: the neighbouring write
> +	 * that left it here has written its part.  Mark it now, under the
> +	 * folio lock, so the writeback that cleans both writes' data drops it.
> +	 * Not a folio larger than the edge the two writes share, which holds
> +	 * more than they wrote and cannot be dropped in part.
> +	 */
> +	if ((fgp_flags & FGP_DONTCACHE) && (fgp_flags & FGP_DONTCACHE_EDGE) &&
> +	    (fgp_flags & FGP_LOCK) && folio_test_uptodate(folio) &&
> +	    filemap_dontcache_edge_unit(mapping, folio))
> +		folio_mark_dropbehind(folio);

[Severity: Medium]
Can calling folio_mark_dropbehind() here before the caller dirties the folio
cause an imbalance in WB_DONTCACHE_DIRTY when PAGECACHE_TAG_DIRTY is still
set in mapping->i_pages but PG_dirty has already been cleared on the folio?

In folio_mark_dropbehind() (and similarly in filemap_clear_dropbehind()),
the WB_DONTCACHE_DIRTY adjustment checks PAGECACHE_TAG_DIRTY without checking
folio_test_dirty(folio):

mm/filemap.c:folio_mark_dropbehind() {
	...
	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);
}

By contrast, inode_do_switch_wbs() checks both PAGECACHE_TAG_DIRTY and
folio_test_dirty(folio) under mapping->i_pages.xa_lock, because the paths
that uncount a dirty folio under the folio lock --
folio_clear_dirty_for_io() and __folio_cancel_dirty() ->
folio_account_cleaned() -- clear PG_dirty and decrement WB_DONTCACHE_DIRTY
before PAGECACHE_TAG_DIRTY is cleared.

A folio can be unlocked while remaining resident and uptodate in
mapping->i_pages with PG_dirty == 0 and PAGECACHE_TAG_DIRTY == 1:

1. In iomap_writepages() -> writeback_get_folio() ->
   folio_prepare_writeback(), folio_clear_dirty_for_io(folio) clears
   PG_dirty and decrements WB_DONTCACHE_DIRTY. Then iomap_writeback_folio()
   checks iomap_writeback_handle_eof(); if the folio is at or beyond i_size
   (for example, dirtied at i_size by xfs_zero_range() -> iomap_zero_range()
   before i_size is updated, or raced with truncate_setsize()),
   iomap_writeback_folio() returns 0 without calling
   folio_start_writeback(), unlocking the folio with PG_dirty == 0 and
   PAGECACHE_TAG_DIRTY == 1.

2. On block device mappings using iomap buffered writes with buffer heads
   (block/fops.c sets FOP_DONTCACHE and IOMAP_F_BUFFER_HEAD), if a dirty
   folio's buffers are cleaned and try_to_free_buffers() runs,
   folio_cancel_dirty() clears PG_dirty and decrements WB_DONTCACHE_DIRTY
   while leaving PAGECACHE_TAG_DIRTY set.

If an uncached edge write then looks up such a folio (PG_dirty == 0,
PAGECACHE_TAG_DIRTY == 1, PG_dropbehind == 0) in __filemap_get_folio_mpol(),
folio_mark_dropbehind() sees PAGECACHE_TAG_DIRTY and increments
WB_DONTCACHE_DIRTY. When iomap_write_end() subsequently dirties the folio via
filemap_dirty_folio() or block_dirty_folio(), folio_test_set_dirty(folio)
transitions PG_dirty from 0 to 1 and folio_account_dirtied() sees
PG_dropbehind set, incrementing WB_DONTCACHE_DIRTY a second time for the
same folio (and conversely filemap_clear_dropbehind() would decrement
WB_DONTCACHE_DIRTY a second time if called on a dropbehind folio in that
state).

Should folio_mark_dropbehind() and filemap_clear_dropbehind() also verify
folio_test_dirty(folio) alongside PAGECACHE_TAG_DIRTY under
mapping->i_pages.xa_lock?

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