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

[sashiko-bot@kernel.org]

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