Date: Sat, 30 May 2026 01:54:55 +0000
From: Yosry Ahmed <yosry@kernel.org>
To: Nhat Pham <nphamcs@gmail.com>
Cc: sashiko-reviews@lists.linux.dev, akpm@linux-foundation.org, 
	Hao Jia <jiahao.kernel@gmail.com>, hannes@cmpxchg.org, mkoutny@suse.com, tj@kernel.org
Subject: Re: [PATCH v2 2/4] mm/zswap: Implement proactive writeback
Message-ID: <ahpDDwtBE-MG_lmO@google.com>
References: <20260525122242.36127-3-jiahao.kernel@gmail.com>
 <20260525135046.413871F000E9@smtp.kernel.org>
 <b1ca372d-530a-228f-c888-d6d04a5c133c@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b1ca372d-530a-228f-c888-d6d04a5c133c@gmail.com>

On Tue, May 26, 2026 at 07:47:48PM +0800, Hao Jia wrote:
> 
> 
> On 2026/5/25 21:50, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> > 
> > New issues:
> > - [High] Premature abort of proactive writeback due to flawed failure tracking across the memcg hierarchy.
> > - [Medium] zswap_proactive_writeback incorrectly returns 0 (success) on partial writebacks, breaking memory.reclaim semantics.
> > 
> > Pre-existing issues:
> > - [High] Double decrement of `nlru->nr_items` in `list_lru` when `shrink_memcg_cb` successfully writes back a page.
> > --
> > 
> > commit df0f700584d1310ed3a326adfe34b6963c7222ce
> > Author: Hao Jia <jiahao1@lixiang.com>
> > 
> > mm/zswap: Implement proactive writeback
> > 
> > This commit introduces a "zswap_writeback_only" key to the memory.reclaim
> > cgroup interface. It allows users to proactively write back zswap pages
> > to the backing swap up to a specified budget without invoking standard
> > memory reclaim.
> > 
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 6519f646b496b..947507b9a1852 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > 
> > [ ... ]
> > 
> > > @@ -1684,6 +1684,144 @@ int zswap_load(struct folio *folio)
> > >   	return 0;
> > >   }
> > > +/*
> > 
> > [ ... ]
> > 
> > > +static long zswap_proactive_shrink_memcg(struct mem_cgroup *memcg,
> > > +					 unsigned long nr_to_write)
> > > +{
> > 
> > [ ... ]
> > 
> > > +		while (nr_scanned < nr_to_scan) {
> > > +			unsigned long nr_to_walk = min(ZSWAP_PROACTIVE_WB_BATCH,
> > > +						       nr_to_scan - nr_scanned);
> > > +
> > > +			if (signal_pending(current))
> > > +				return nr_written;
> > > +
> > > +			/*
> > > +			 * Account for the committed budget rather than the walker's
> > > +			 * actual delta. If the list is emptied concurrently, the
> > > +			 * walker visits nothing and nr_scanned would never advance.
> > > +			 */
> > > +			nr_scanned += nr_to_walk;
> > > +
> > > +			nr_written += list_lru_walk_one(&zswap_list_lru, nid, memcg,
> > > +							&shrink_memcg_cb,
> > > +							&encountered_page_in_swapcache,
> > > +							&nr_to_walk);
> > 
> > [Severity: High]
> > This is a pre-existing issue, but proactive writeback heavily triggers it on
> > demand. When shrink_memcg_cb() initiates a successful writeback, it calls
> > zswap_writeback_entry() which frees the entry. This removes the entry from
> > the list and decrements the LRU item count. Then shrink_memcg_cb() returns
> > LRU_REMOVED_RETRY.
> > 
> > Does __list_lru_walk_one() fall through to the LRU_REMOVED case and perform a
> > second decrement of nlru->nr_items? If so, will this double decrement corrupt
> > the global node LRU count and cause underflows?
> > 
> 
> As for this pre-existing issue, nlru->nr_items could indeed be
> double-decremented. IIRC, we don't have any code reading nlru->nr_items for
> the zswap LRU; zswap only uses list_lru_count_one() (l->nr_items), so things
> are working fine for now.


Yes, but it seems like we're misusing the list_lru infra here. I think
we shouldn't end up calling list_lru_del() from the list_lru_walk
callback?

Nhat, WDYT?

> 
> This issue wasn't introduced by this patchset, but it will cause problems if
> nlru->nr_items is ever used in the future. Therefore, I will try to address
> it in a separate patch later, or ask for help if needed.