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

[Kaitao Cheng]

在 2026/11/3 00:58, 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.

My v2 version is almost ready, please give me a moment.

-- 
Thanks
Kaitao Cheng