[PATCH] drm/i915: look up shmem folios by index for writeback

[PATCH] drm/i915: look up shmem folios by index for writeback

[Matthias Goergens]

Since commit 776a853a43c9 ("i915: Use writeback_iter()"),
__shmem_writeback() starts writeback on no folios at all.  In
WB_SYNC_NONE mode writeback_iter() only returns folios tagged
PAGECACHE_TAG_DIRTY, and shmem never sets that tag: it dirties folios
with noop_dirty_folio(), because shmem relies on LRU-based swap writeout
rather than on the dirty tag (see the comment in __folio_mark_dirty() in
mm/page-writeback.c).  The loop before that commit looked pages up by
index and tested the page dirty flag, so it did write.

This affects the shrinker's I915_SHRINK_WRITEBACK pass and the i915 TTM
backend, which both call __shmem_writeback().  The pages stay dirty and
on the LRU, so reclaim still swaps them out later; what is lost is the
early writeback.

Go back to walking the object by index, with the folio API.  Mapped
folios are still skipped.  Unlike the old loop, do not wait for a folio
lock, since this also runs from the OOM notifier; a locked folio is left
to reclaim.  shmem_write_folio() returns with the folio unlocked except
for AOP_WRITEPAGE_ACTIVATE, so unlock it only in that case.  It may
also split a large folio, so read the folio size only after writing, to
find the next index.

I came across this while measuring an earlier patch of mine to this
loop, which changed nothing because the loop never visits a folio.
This patch was measured without Intel hardware, with a test-only mock
selftest in a QEMU guest with swap.  Dirty, unpinned objects of 1 to
256 MiB, with and without THP, went through i915_gem_shrink() with
I915_SHRINK_WRITEBACK.  Before this patch, no page was written in any
case; with it, every page that was not mmapped was.  The contents read
back intact after swap-in, and the mock selftests give the same results
as before.  This has not been tested on hardware.

Fixes: 776a853a43c9 ("i915: Use writeback_iter()")
Cc: stable@vger.kernel.org # v6.16+
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
This replaces my two earlier patches to this loop, which can be dropped:
the loop they change never visits a folio.

https://lore.kernel.org/all/20260915062924.2410550-1-matthias.goergens@gmail.com/
https://lore.kernel.org/all/20260914105606.3997649-1-matthias.goergens@gmail.com/

In 6.18.y and 7.2.y the call is shmem_writeout(folio, NULL, NULL)
instead of shmem_write_folio(folio), with the same return convention.

In review of 776a853a43c9, Christoph asked for this loop to move behind
a shmem API instead of living in drivers, and Matthew agreed:

https://lore.kernel.org/all/Z--XtaM7Z3zbjzAu@infradead.org/
https://lore.kernel.org/all/Z-_hQwNeiOnNYJVp@casper.infradead.org/

I kept this fix inside i915 so that it is one patch for stable.

The test harness is not part of the patch; I can post it if that helps.
Testing on Intel hardware under memory pressure would be very welcome.

 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 56 ++++++++++++++++++-----
 1 file changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index ef9440166295..54424e434f3b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -304,28 +304,60 @@ shmem_truncate(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+/* Start writing a locked folio to swap.  It is unlocked on return. */
+static void shmem_writeback_folio(struct folio *folio)
+{
+	int ret;
+
+	folio_set_reclaim(folio);
+	ret = shmem_write_folio(folio);
+	if (!folio_test_writeback(folio))
+		folio_clear_reclaim(folio);
+
+	/* shmem_write_folio() unlocks the folio unless it returns this. */
+	if (ret == AOP_WRITEPAGE_ACTIVATE)
+		folio_unlock(folio);
+}
+
 void __shmem_writeback(size_t size, struct address_space *mapping)
 {
-	struct writeback_control wbc = {
-		.sync_mode = WB_SYNC_NONE,
-		.nr_to_write = SWAP_CLUSTER_MAX,
-		.range_start = 0,
-		.range_end = LLONG_MAX,
-	};
-	struct folio *folio = NULL;
-	int error = 0;
+	pgoff_t nr_pages = size >> PAGE_SHIFT;
+	pgoff_t index = 0;
 
 	/*
+	 * shmem marks folios dirty with noop_dirty_folio(), which does not
+	 * set PAGECACHE_TAG_DIRTY, so writeback_iter() would find none of
+	 * them.  Look up each folio of the object by index and test its
+	 * dirty flag.
+	 *
 	 * Leave mmapings intact (GTT will have been revoked on unbinding,
 	 * leaving only CPU mmapings around) and add those folios to the LRU
 	 * instead of invoking writeback so they are aged and paged out
 	 * as normal.
 	 */
-	while ((folio = writeback_iter(mapping, &wbc, folio, &error))) {
-		if (folio_mapped(folio))
-			folio_redirty_for_writepage(&wbc, folio);
+	while (index < nr_pages) {
+		struct folio *folio;
+
+		/*
+		 * Skip folios that are absent or locked: this runs from the
+		 * shrinker and the OOM notifier, so do not wait for a lock.
+		 */
+		folio = __filemap_get_folio(mapping, index,
+					    FGP_LOCK | FGP_NOWAIT, 0);
+		if (IS_ERR(folio)) {
+			index++;
+			continue;
+		}
+
+		index = folio->index;
+		if (!folio_mapped(folio) && folio_clear_dirty_for_io(folio))
+			shmem_writeback_folio(folio);
 		else
-			error = shmem_write_folio(folio);
+			folio_unlock(folio);
+
+		/* Writing may have split the folio, so read its size only now. */
+		index += folio_nr_pages(folio);
+		folio_put(folio);
 	}
 }
 

base-commit: ce1e0223d8ad4211275c82a17ed6d43ab81e13d9
-- 
2.56.0

Re: [PATCH] drm/i915: look up shmem folios by index for writeback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] i915: soft lockup in __shmem_writeback() on large sparse objects
--

commit 13d0555b9d9183775f7e6a194865fdfde399b137
Author: Matthias Goergens <matthias.goergens@gmail.com>

drm/i915: look up shmem folios by index for writeback

This commit reverts to walking the object by index using the folio API
instead of using writeback_iter(), because writeback_iter() misses
shmem folios that do not set PAGECACHE_TAG_DIRTY. It introduces a
manual loop in __shmem_writeback() to find folios by index.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index ef94401662955..54424e434f3b3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c

[ ... ]

>  void __shmem_writeback(size_t size, struct address_space *mapping)
>  {
> -	struct writeback_control wbc = {
> -		.sync_mode = WB_SYNC_NONE,
> -		.nr_to_write = SWAP_CLUSTER_MAX,
> -		.range_start = 0,
> -		.range_end = LLONG_MAX,
> -	};
> -	struct folio *folio = NULL;
> -	int error = 0;
> +	pgoff_t nr_pages = size >> PAGE_SHIFT;
> +	pgoff_t index = 0;
>  
>  	/*
> +	 * shmem marks folios dirty with noop_dirty_folio(), which does not
> +	 * set PAGECACHE_TAG_DIRTY, so writeback_iter() would find none of
> +	 * them.  Look up each folio of the object by index and test its
> +	 * dirty flag.
> +	 *
>  	 * Leave mmapings intact (GTT will have been revoked on unbinding,
>  	 * leaving only CPU mmapings around) and add those folios to the LRU
>  	 * instead of invoking writeback so they are aged and paged out
>  	 * as normal.
>  	 */
> -	while ((folio = writeback_iter(mapping, &wbc, folio, &error))) {
> -		if (folio_mapped(folio))
> -			folio_redirty_for_writepage(&wbc, folio);
> +	while (index < nr_pages) {
> +		struct folio *folio;
> +
> +		/*
> +		 * Skip folios that are absent or locked: this runs from the
> +		 * shrinker and the OOM notifier, so do not wait for a lock.
> +		 */
> +		folio = __filemap_get_folio(mapping, index,
> +					    FGP_LOCK | FGP_NOWAIT, 0);
> +		if (IS_ERR(folio)) {
> +			index++;
> +			continue;
> +		}
                 ^^^^

[Severity: High]
Can this loop cause a soft lockup for large sparse objects?

If the object has a large hole, __filemap_get_folio() will return -ENOENT for
missing folios. The loop then handles this error by stepping forward only 1
page at a time.

For extremely large sparse objects under memory pressure, could this result in
an unbounded CPU stall without yielding, blocking kswapd or direct reclaim,
since there is no cond_resched() in the loop?

> +
> +		index = folio->index;
> +		if (!folio_mapped(folio) && folio_clear_dirty_for_io(folio))
> +			shmem_writeback_folio(folio);
>  		else
> -			error = shmem_write_folio(folio);
> +			folio_unlock(folio);
> +
> +		/* Writing may have split the folio, so read its size only now. */
> +		index += folio_nr_pages(folio);
> +		folio_put(folio);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002073107.2209644-1-matthias.goergens@gmail.com?part=1

✗ LGCI.VerificationFailed: failure for drm/i915: look up shmem folios by index for writeback

[Patchwork]

== Series Details ==

Series: drm/i915: look up shmem folios by index for writeback
URL   : https://patchwork.freedesktop.org/series/175462/
State : failure

== Summary ==

Series author address 'matthias.goergens@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://gfx-ci.igk.intel.com:8080/job/CI_PW_kernel/188956/ (on built-in)

Re: [PATCH] drm/i915: look up shmem folios by index for writeback

[Matthias Goergens]

Thanks, good catch.  v2 looks the folios up in batches with
filemap_get_folios() and calls cond_resched() between batches:
https://lore.kernel.org/all/20261004094827.174193-1-matthias.goergens@gmail.com/