Re: [PATCH] mm/zswap: fix double decrement of the LRU node count on writeback

[Johannes Weiner]

On Thu, Oct 29, 2026 at 08:10:54AM -0700, Yosry Ahmed wrote:
> On Wed, Oct 28, 2026 at 9:23 PM Kaitao Cheng <kaitao.cheng@xxxxxxxxx> wrote:
> >
> >
> >
> > 在 2026/10/29 05:10, Johannes Weiner 写道:
> > > On Wed, Oct 28, 2026 at 12:22:43PM -0700, Yosry Ahmed wrote:
> > >> On Wed, Oct 28, 2026 at 4:36 AM Kaitao Cheng <kaitao.cheng@xxxxxxxxx> wrote:
> > >>>
> > >>> From: Kaitao Cheng <chengkaitao@xxxxxxxxxx>
> > >>>
> > >>> Successful writeback calls zswap_entry_free(), which removes the entry
> > >>> from the LRU and decrements both the sublist and node item counts.
> > >>> shrink_memcg_cb() then returns LRU_REMOVED_RETRY, causing the LRU walker
> > >>> to decrement the node count again.
> > >>
> > >> I wonder if we have a valid use case for adding LRU_REMOVED_COUNTED or
> > >> LRU_DELETED instead to simply avoid the decrement.
> > >
> > > It bugs me that it's the walker, not the callback, that adjusts the
> > > count. That composes poorly with the normal list manipulation API.
> > >
> > > How about pushing the accounting into the isolation callback, which
> > > can then simply use __list_lru_del()?
> > >
> > > The tradeoff is that more things need to be passed into the isolation
> > > callback, but that seems reasonable. It does make the change noisy,
> > > however, and we might want something like Kaitao's change as a
> > > backportable fix first regardless.
> >
> > I tried three approaches.
> >
> > The first was "pushing the accounting into the isolation callback", as
> > Johannes Weiner suggested. However, as Johannes's example below also shows,
> > this would require extensive changes across multiple subsystems. I doubted
> > that such a broad change would be accepted, so I dropped this approach.
> >
> > The second was to add a new flag to enum lru_status, as Yosry Ahmed suggested,
> > to handle cases like zswap where the callback has already decremented the
> > count. However, the incorrect lru->node[nid].nr_items value in zswap is only
> > an accounting issue: I could not find any code that reads this counter for
> > zswap. Changing a generic interface to fix an accounting error with no
> > apparent functional impact did not seem worthwhile.
> >
> > The third approach is the one implemented in this patch. It keeps the changes
> > within zswap and fixes the accounting issue, so I think it is appropriate.
> 
> I honestly think it's a problem with the list_lru API, as Johannes
> said, walkers compose poorly with the API. So I prefer we either take
> Johannes's suggestion or add LRU_DELETED, unless there's strong
> objection to both ideas.
> 
> FWIW, we don't need a minimal fix for backports, I don't think this
> needs to be backported at all. As Kaitao said, I don't think the wrong
> list_lru count is being used anywhere for zswap.

Good point.

Ok I will send a proper patch and we can discuss it from there.