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

[Yosry Ahmed]

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.