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

[Kaitao Cheng <kaitao.cheng@linux.dev>]

在 2026/10/29 23:10, Yosry Ahmed 写道:
> On Wed, Oct 28, 2026 at 9:23 PM Kaitao Cheng <kaitao.cheng@linux.dev> 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@linux.dev> wrote:
>>>>>
>>>>> From: Kaitao Cheng <chengkaitao@kylinos.cn>
>>>>>
>>>>> 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.

In v2 (https://lore.kernel.org/all/aupITfr9pBKPrU20@dread/), Dave Chinner
explicitly objected to changing the common interface, so we need to revisit
the fix. Otherwise, progress on resolving this issue will remain blocked.

> 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.

-- 
Thanks
Kaitao Cheng