Re: [PATCH v8 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 3 potential issue(s) to consider:
- [Medium] mm: filemap_dontcache_edge_completes() misses partially uptodate iomap folios
- [Medium] mm: filemap_dontcache_edge_complete() misses writeback kick and racing bio drop
- [Medium] mm: WB_DONTCACHE_DIRTY leak in filemap_dontcache_edge_complete()
--

commit 7d2a5aea41a384cd5ec593563dbc8bd144b6bcce
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 that an uncached writer that syncs
its own writes can defer marking and dropping partially covered edge
folios until after the write and sync complete.

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1494e42e609fa..890913f38c65f 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1695,6 +1695,72 @@ static bool filemap_dontcache_edge_unit(struct address_space *mapping,
>  	return folio_nr_pages(folio) == mapping_min_folio_nrpages(mapping);
>  }
>  
> +/**
> + * filemap_dontcache_edge_completes - will this write complete a shared page?
> + * @mapping: the file's mapping
> + * @pos: any byte offset within the page
> + *
> + * For an uncached writer that completes the pages it covers only partly
> + * itself (IOCB_DONTCACHE_DEFER).  Call right before writing such a page: it
> + * is in the page cache and uptodate when the neighbouring write has already
> + * written its part, and this write then completes it.  Two writes that
> + * arrive together may both find it absent; the page is then left cached,
> + * unmarked, until reclaim.
> + *
> + * Return: true if the caller is to call filemap_dontcache_edge_complete()
> + * once its write, including any sync of it, is done.
> + */
> +bool filemap_dontcache_edge_completes(struct address_space *mapping, loff_t pos)
> +{
> +	struct folio *folio;
> +	bool ret;
> +
> +	/* FGP_DONTCACHE: a plain lookup would clear a drop-behind mark. */
> +	folio = __filemap_get_folio(mapping, pos >> PAGE_SHIFT, FGP_DONTCACHE, 0);
> +	if (IS_ERR(folio))
> +		return false;
> +	ret = folio_test_uptodate(folio) &&
> +	      filemap_dontcache_edge_unit(mapping, folio);
> +	folio_put(folio);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(filemap_dontcache_edge_completes);

[Severity: Medium]
What happens here (and in __filemap_get_folio_mpol()) on iomap filesystems
when the filesystem block size is smaller than the minimum folio size, i.e.
i_blocksize(inode) < PAGE_SIZE (for example, 4 KiB XFS blocks on a 16 KiB or
64 KiB PAGE_SIZE kernel, or sub-page block sizes on a 4 KiB PAGE_SIZE
kernel)?

When the first partial uncached write allocates the shared folio via
iomap_get_folio(), i_blocks_per_folio(inode, folio) is greater than 1, so
__iomap_write_begin() and __iomap_write_end() attach a struct
iomap_folio_state (ifs) and only mark the blocks within
[round_down(pos, block_size), round_up(pos + len, block_size)) uptodate in
ifs->state:

fs/iomap/buffered-io.c:iomap_set_range_uptodate() {
		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);
}

Because the remaining blocks in the folio are not uptodate yet,
folio_mark_uptodate() is not called after the first partial write.

When the neighbouring write arrives to write the rest of the page and calls
filemap_dontcache_edge_completes() before writing (or looks up the folio in
__filemap_get_folio_mpol() before __iomap_write_end() runs),
folio_test_uptodate(folio) is still false.  Won't
filemap_dontcache_edge_completes() return false and leave the completed edge
folio cached until reclaim?

> +
> +/**
> + * filemap_dontcache_edge_complete - drop a shared page both writes have written
> + * @mapping: the file's mapping
> + * @pos: any byte offset within the page
> + *
> + * Mark the page drop-behind.  If it is already written back, drop it now;
> + * otherwise the writeback that cleans it drops it.  A dirty page marked here
> + * counts as DONTCACHE dirty and wakes the DONTCACHE writeback kick, so a
> + * caller that syncs its write calls this after the sync.  A folio larger
> + * than the smallest the mapping allows is left as it is: it holds more than
> + * the two writes' page, and it cannot be dropped in part.
> + */
> +void filemap_dontcache_edge_complete(struct address_space *mapping, loff_t pos)
> +{
> +	struct folio *folio;
> +
> +	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)) {
> +		folio_mark_dropbehind(folio);
> +		/*
> +		 * Drop it now if it is already clean, with the lock still
> +		 * held: folio_end_dropbehind() would take it again with a
> +		 * trylock, and a folio another task locks in between would
> +		 * be left for reclaim, no writeback being left to drop it.
> +		 */
> +		filemap_end_dropbehind(folio);
> +	}
> +	folio_unlock(folio);
> +	folio_put(folio);
> +}
> +EXPORT_SYMBOL_GPL(filemap_dontcache_edge_complete);

[Severity: Medium]
There appear to be a few issues in filemap_dontcache_edge_complete() with
how writeback kicking and in-flight writeback are handled:

1. The kernel-doc states that a dirty page marked here counts as DONTCACHE
dirty and wakes the DONTCACHE writeback kick, and that an uncached writer
using IOCB_DONTCACHE_DEFER avoids per-segment writeback kicks.  However,
generic_write_sync() is not updated for IOCB_DONTCACHE_DEFER:

include/linux/fs.h:generic_write_sync() {
	if (iocb_is_dsync(iocb)) {
		...
	} else if (iocb->ki_flags & IOCB_DONTCACHE) {
		filemap_dontcache_kick_writeback(iocb->ki_filp->f_mapping);
	}
}

During vfs_iocb_iter_write(), generic_write_sync() runs before
filemap_dontcache_edge_complete() is called, while FGP_DONTCACHE_DEFER has
kept the edge folio unmarked.  Then, when filemap_dontcache_edge_complete()
later marks a dirty folio via folio_mark_dropbehind(), it increments
WB_DONTCACHE_DIRTY and filemap_end_dropbehind() returns early because
folio_test_dirty(folio) is true, without calling
filemap_dontcache_kick_writeback(mapping).  If WB_start_dontcache was
already consumed while WB_DONTCACHE_DIRTY was 0, what wakes writeback after
filemap_dontcache_edge_complete() marks the dirty folio?  And shouldn't
generic_write_sync() skip filemap_dontcache_kick_writeback() when
IOCB_DONTCACHE_DEFER is set?

2. If writeback on the shared folio has already started before
filemap_dontcache_edge_complete() locks it, PG_dropbehind was not set when
iomap_add_to_ioend() (or submit_bh_wbc()) submitted the bio:

fs/iomap/ioend.c:iomap_add_to_ioend() {
	if (folio_test_dropbehind(folio))
		bio_set_flag(&ioend->io_bio, BIO_COMPLETE_IN_TASK);
}

Because filemap_dontcache_edge_complete() does not wait for PG_writeback,
it sets PG_dropbehind while the bio is in flight and filemap_end_dropbehind()
returns early because folio_test_writeback(folio) is true.  When that bio
completes in IRQ or softirq context (!in_task()), folio_end_dropbehind()
cannot invalidate the folio; or if the bio completes in task context while
filemap_dontcache_edge_complete() still holds the folio lock,
folio_end_dropbehind()'s folio_trylock() fails.  Won't the folio be left in
the page cache until reclaim in both cases?

[Severity: Medium]
Can calling folio_mark_dropbehind(folio) followed immediately by
filemap_end_dropbehind(folio) in filemap_dontcache_edge_complete() leak
WB_DONTCACHE_DIRTY?

In filemap_dontcache_edge_complete(), folio_mark_dropbehind() is called
unconditionally even when the folio is already clean after the caller's
sync, setting PG_dropbehind under mapping->i_pages.  Immediately after,
filemap_end_dropbehind() checks folio_test_dirty() and clears PG_dropbehind
without holding mapping->i_pages or adjusting WB_DONTCACHE_DIRTY:

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

If a concurrent path that dirties a mapped folio without holding the folio
lock (such as zap_present_folio_ptes(), try_to_unmap_one(), or
follow_page_pte() calling folio_mark_dirty() -> filemap_dirty_folio() under
the PTE lock) runs after filemap_end_dropbehind() sees
!folio_test_dirty(folio) but before folio_test_clear_dropbehind(folio):

1. folio_account_dirtied() runs under mapping->i_pages xa_lock, sees
folio_test_dropbehind(folio) is true, and increments WB_DONTCACHE_DIRTY.

2. filemap_end_dropbehind() clears PG_dropbehind via
folio_test_clear_dropbehind(folio) without decrementing WB_DONTCACHE_DIRTY
(unlike filemap_clear_dropbehind()).

3. The folio is not invalidated (either because folio_mapped(folio) is true
or folio_unmap_invalidate() sees folio_test_dirty(folio) under xa_lock), and
when the folio is later cleaned or truncated, PG_dropbehind is already clear
so WB_DONTCACHE_DIRTY is never decremented.

A similar mismatch occurs if PAGECACHE_TAG_DIRTY is still set in
mapping->i_pages while PG_dirty and PG_writeback are clear (for instance,
after folio_clear_dirty_for_io() when writeback bails out early past EOF, or
after folio_cancel_dirty()): folio_mark_dropbehind() sees PAGECACHE_TAG_DIRTY
and increments WB_DONTCACHE_DIRTY, and then filemap_end_dropbehind() sees
!folio_test_writeback(folio) && !folio_test_dirty(folio) and clears
PG_dropbehind without decrementing WB_DONTCACHE_DIRTY.

Should filemap_dontcache_edge_complete() avoid setting PG_dropbehind on an
already-clean folio (or use filemap_clear_dropbehind() under xa_lock when
clearing it)?

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