[PATCH] drm/i915: unlock shmem folios after skipped writeback

[PATCH] drm/i915: unlock shmem folios after skipped writeback

[Matthias Goergens]

writeback_iter() returns a locked folio.  The mapped-folio path redirties
the folio without unlocking it, so shrinking can leave the folio locked
indefinitely.

shmem_write_folio() can also return AOP_WRITEPAGE_ACTIVATE with the folio
locked.  Passing that positive control value back to writeback_iter()
triggers its warning, stops asynchronous writeback, and leaves the folio
locked.  This can occur when zswap rejects a folio while memcg zswap
writeback is disabled.

Unlock folios on both paths and consume AOP_WRITEPAGE_ACTIVATE so the
best-effort writeback scan can continue.

Fixes: 776a853a43c9 ("i915: Use writeback_iter()")
Cc: stable@vger.kernel.org
Signed-off-by: Matthias Goergens <matthias.goergens@gmail.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 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..e7b18f7f9644 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -322,10 +322,16 @@ void __shmem_writeback(size_t size, struct address_space *mapping)
 	 * as normal.
 	 */
 	while ((folio = writeback_iter(mapping, &wbc, folio, &error))) {
-		if (folio_mapped(folio))
+		if (folio_mapped(folio)) {
 			folio_redirty_for_writepage(&wbc, folio);
-		else
+			folio_unlock(folio);
+		} else {
 			error = shmem_write_folio(folio);
+			if (error == AOP_WRITEPAGE_ACTIVATE) {
+				folio_unlock(folio);
+				error = 0;
+			}
+		}
 	}
 }
 

base-commit: cbd3dafc2003db679ccd2f6c6a2551db79657049
-- 
2.55.0

Re: [PATCH] drm/i915: unlock shmem folios after skipped writeback

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] __shmem_writeback() limits writeback to SWAP_CLUSTER_MAX (32) pages, ignoring the requested size, which causes the shrinker to severely under-reclaim memory while reporting full reclaim success.
--

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

drm/i915: unlock shmem folios after skipped writeback

This commit fixes an issue where shmem folios are left locked indefinitely
when writeback is skipped. It unlocks folios when they are redirtied or when
shmem_write_folio returns AOP_WRITEPAGE_ACTIVATE.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index ef94401662955..e7b18f7f9644c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -322,10 +322,16 @@ void __shmem_writeback(size_t size, struct address_space *mapping)
>  	 * as normal.
>  	 */
>  	while ((folio = writeback_iter(mapping, &wbc, folio, &error))) {

[Severity: High]
This isn't a bug introduced by this patch, but does the hardcoded limit on
wbc.nr_to_write here cause the shrinker to severely under-reclaim?

In drivers/gpu/drm/i915/gem/i915_gem_shmem.c:__shmem_writeback(), the size
parameter is completely ignored and the writeback_control is initialized
earlier in the function with:

    struct writeback_control wbc = {
        .sync_mode = WB_SYNC_NONE,
        .nr_to_write = SWAP_CLUSTER_MAX,
        ...
    };

Because of this, writeback_iter() will terminate after 32 pages (128KB).
However, when the system is under memory pressure and i915_gem_shrinker_scan()
delegates to i915_gem_shrink(), try_to_writeback() eventually reaches
__shmem_writeback().

i915_gem_shrink() assumes the entire object was successfully written back
and increments its freed count by the full object size. If the object is
large, the mm subsystem is misled into thinking massive amounts of memory
were freed when only 128KB actually was. This could degrade memory reclaim
under pressure and lead to system stalls or OOMs.

Should the writeback limit respect the requested size parameter instead of
being hardcoded to SWAP_CLUSTER_MAX?

> -		if (folio_mapped(folio))
> +		if (folio_mapped(folio)) {
>  			folio_redirty_for_writepage(&wbc, folio);
> -		else
> +			folio_unlock(folio);
> +		} else {
>  			error = shmem_write_folio(folio);
> +			if (error == AOP_WRITEPAGE_ACTIVATE) {
> +				folio_unlock(folio);
> +				error = 0;
> +			}
> +		}
>  	}
>  }

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

Re: [PATCH] drm/i915: unlock shmem folios after skipped writeback

[Matthias Goergens]

Yes, the budget should use the supplied object size. The current limit
is a separate regression from
776a853a43c9 ("i915: Use writeback_iter()"). The old loop walked
size >> PAGE_SHIFT pages; the iterator now honours the pre-existing
SWAP_CLUSTER_MAX budget, leaving size unused.

The reclaim-accounting consequence needs some qualification, though.
Before optional writeback, i915 drops its references to the whole object's
pages and clears mapping unevictability. The remaining pages can be
reclaimed by the MM. The shrinker also counts the object when writeback
isn't requested, so that count doesn't mean every page reached swap.

I have sent a separate patch [1], "drm/i915: size shmem writeback budget
to the object", using an object-sized nr_to_write budget to restore the
previous writeback scope. It remains best-effort.
I have not measured the effect of the current limit on reclaim latency
or OOM.

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