Re: [PATCH v10 6/7] mm, iomap: let an uncached writer complete its shared pages itself

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iomap: iomap_get_folio() leaves full multi-extent folios unmarked on defer
- [Medium] mm: filemap_dontcache_edge_complete() marks already-clean mapped folios
--

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

mm, iomap: let an uncached writer complete its shared pages itself

This commit introduces IOCB_DONTCACHE_DEFER, IOMAP_DONTCACHE_DEFER, and
FGP_DONTCACHE_DEFER along with filemap_dontcache_edge_completes() and
filemap_dontcache_edge_complete() so an uncached writer can defer marking or
dropping shared boundary folios until after its write and sync finish.

> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 919e4d6c3c972..c47fa683aaa8a 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -812,6 +812,8 @@ struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos, size_t len)
>  		/* A partly covered unit is shared with the neighbour. */
>  		if (off || len < unit) {
>  			fgp |= FGP_DONTCACHE_EDGE;
> +			if (iter->flags & IOMAP_DONTCACHE_DEFER)
> +				fgp |= FGP_DONTCACHE_DEFER;
>  			len = min_t(size_t, len, unit - off);
>  		}
>  	}
> @@ -823,7 +825,7 @@ struct folio *iomap_get_folio(struct iomap_iter *iter, loff_t pos, size_t len)
>  	 * With blocks smaller than the folio, an edge is not uptodate
>  	 * until this write completes it.
>  	 */
> -	if ((fgp & FGP_DONTCACHE_EDGE) &&
> +	if ((fgp & FGP_DONTCACHE_EDGE) && !(fgp & FGP_DONTCACHE_DEFER) &&
>  	    !IS_ERR(folio) && !folio_test_uptodate(folio) &&
>  	    folio_dontcache_edge_written(folio, pos, len))
>  		folio_mark_dropbehind(folio);

[Severity: Medium]
Can this leave a newly allocated folio that a single write covers in full
without PG_dropbehind when the folio spans multiple extents or hits a
short-copy retry?

In iomap_file_buffered_write() -> iomap_write_iter() -> iomap_write_begin()
-> __iomap_get_folio() -> iomap_get_folio(), the len passed to
iomap_get_folio() has already been clamped to the current extent length
iomap_length(iter) (or narrowed to copied on a short-copy retry) rather than
the remaining write length iter->len.

When i_blocksize(inode) < mapping_min_folio_nrbytes(mapping) and a unit that
the write covers in full (off == 0 and iter->len >= unit) spans two extents:

1. On the first extent, iomap_get_folio() sees len < unit and sets
   FGP_DONTCACHE_EDGE | FGP_DONTCACHE_DEFER, so __filemap_get_folio_mpol()
   allocates the folio without PG_dropbehind.

2. On the subsequent extent covering the rest of the unit, off != 0, so
   iomap_get_folio() again sets FGP_DONTCACHE_EDGE | FGP_DONTCACHE_DEFER.

Previously, iomap_get_folio() called folio_dontcache_edge_written() on the
final extent of the folio, saw that the earlier blocks were uptodate, and
called folio_mark_dropbehind(). With !(fgp & FGP_DONTCACHE_DEFER) skipping
folio_mark_dropbehind(), and because the unit is completely covered by this
single write rather than being a shared boundary edge passed to
filemap_dontcache_edge_complete(), won't the folio stay in the page cache
unmarked until reclaim?

[ ... ]

> diff --git a/mm/filemap.c b/mm/filemap.c
> index ec52e4c960849..d540df29870a9 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1721,6 +1721,76 @@ bool folio_dontcache_edge_written(struct folio *folio, loff_t pos, size_t len)

[ ... ]

> +void filemap_dontcache_edge_complete(struct address_space *mapping, loff_t pos)
> +{
> +	struct folio *folio;
> +	bool kick = false;
> +
> +	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT,
> +				    FGP_LOCK | FGP_DONTCACHE, 0);
> +	if (IS_ERR(folio))
> +		return;
> +	if (!filemap_dontcache_edge_unit(mapping, folio))
> +		goto out;
> +	/* Writeback in flight was submitted unmarked and would not drop it. */
> +	folio_wait_writeback(folio);
> +	if (folio_test_dirty(folio)) {
> +		folio_mark_dropbehind(folio);
> +		kick = true;
> +	} else if (folio_mapped(folio)) {
> +		folio_mark_dropbehind(folio);
> +	} else {
> +		/*
> +		 * Drop it now, unmarked: a mark cleared outside the i_pages
> +		 * lock could miss a store that counts the folio.
> +		 */
> +		folio_unmap_invalidate(mapping, folio, 0);
> +	}

[Severity: Medium]
Should filemap_dontcache_edge_complete() really set PG_dropbehind on an
already-clean mapped folio?

After folio_wait_writeback(folio), if folio_test_dirty(folio) is false and
folio_mapped(folio) is true, the folio is already clean and has no writeback
in flight or scheduled for it. Even if it is dirtied later via mmap and
written back, filemap_end_dropbehind() clears PG_dropbehind without
invalidating when the folio is mapped:

mm/filemap.c:filemap_end_dropbehind() {
	...
	if (folio_test_writeback(folio) || folio_test_dirty(folio))
		return;
	if (!folio_test_clear_dropbehind(folio))
		return;
	if (mapping && !folio_mapped(folio))
		folio_unmap_invalidate(mapping, folio, 0);
}

Leaving PG_dropbehind set on a clean mapped folio seems to cause two problems:

1. If the folio is mapped read-only or never dirtied again, PG_dropbehind
   remains set on the clean folio (suppressing folio_mark_accessed()), and if
   the VMA is later unmapped, a subsequent normal cached filemap_read() will
   drop the folio via filemap_end_dropbehind_read() ->
   filemap_end_dropbehind().

2. As noted in the comment right below in filemap_dontcache_edge_complete(),
   clearing PG_dropbehind outside the mapping->i_pages lock can miss a store
   that dirties and counts the folio in WB_DONTCACHE_DIRTY. On a mapped folio,
   zap_pte_range() can call folio_mark_dirty() -> filemap_dirty_folio() ->
   __folio_mark_dirty() under the PTE lock without holding the folio lock. If
   filemap_end_dropbehind() runs on this clean folio (for example from a
   concurrent filemap_read() -> filemap_end_dropbehind_read()), can
   zap_pte_range() set PG_dirty and increment WB_DONTCACHE_DIRTY under
   mapping->i_pages lock right after filemap_end_dropbehind() checks
   folio_test_dirty(folio) and before it clears PG_dropbehind with
   folio_test_clear_dropbehind(folio), leaking WB_DONTCACHE_DIRTY?

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