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

[sashiko-bot@kernel.org]

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