Re: [PATCH v4 2/5] mm/zswap: Factor writeback loop out of shrink_worker()

[Hao Jia <jiahao.kernel@gmail.com>]

On 2026/6/27 01:09, Yosry Ahmed wrote:
>>>> /*
>>>>     * Take one step of a memcg-tree writeback walk driven by the caller's
>>>>     * iterator, and fold the result into @s, the retry bookkeeping shared
>>>>     * across steps. @memcg is the iterator's current memcg, or NULL once
>>>>     * it has wrapped around after a full pass over the tree.
>>>>     *
>>>>     * The function returns -EAGAIN to signal the caller to abort the walk
>>>>     * after encountering the following conditions MAX_RECLAIM_RETRIES times:
>>>>     * - No writeback-candidate memcgs were found in a memcg tree walk.
>>>>     * - Shrinking a writeback-candidate memcg failed.
>>>
>>> Orthogonal to this patch, but I wonder if this can be simplified. I
>>> wonder if these two conditions can be replaced with "shrinking a memcg
>>> that has zswap entries failed". The "no writeback-candidate memcgs in
>>> the tree" case seems like we should abort right away instead of
>>> retrying?
>>>
>>> Nhat, WDYT?
>>>
>>
>> Perhaps something like the following is what you had in mind? I've
>> drafted the implementation below to make it easier for Nhat to compare
>> with the previous behavior.
> 
> Hmm I think if we pursue this it should be in a separate patch or even
> outside of this series, ideally with numbers/proof that it's not
> introducing regressions to the scenario that lead to its introduction.
> 

Got it. I will temporarily leave this part out of the current patch series.

>>
>>
>>>>     *
>>>>     * Return: The number of compressed bytes written back (>= 0), or -EAGAIN
>>>>     * once the retry budget is exhausted and the caller should abort the walk.
>>>>     */
>>>> static long zswap_shrink_one(struct mem_cgroup *memcg,
>>>
>>> Nit: zswap_shrink_one_memcg()
>>>
>>> BTW, the existing writeback logic has been broken for a while now when
>>> memcg is disabled. I think we constantly hit the !memcg case and run
>>> out of retries. Not sure if your patch changes this in any way, or if
>>> you want to fix that while you're at it :)
>>
>> Yes, I'd be happy to do that. However, would it be better to submit a
>> separate fix patch or combine it with this one?
> 
> A separate patch. Feel free to send it with this series to avoid
> conflicts, but probably as patch 1 as we'll want to CC stable on it.
> 

Done, and I've just submitted the v5 patch.

[v5] 
https://lore.kernel.org/all/20260629112032.20423-1-jiahao.kernel@gmail.com

Please take a look when you have a chance. Thank you very much for your 
review!

> [..]
> 
>> /* Track progress of a memcg-tree writeback walk. */
>> struct zswap_shrink_state {
>>       int scans;
>>       int failures;
>> };
>>
>> /*
>>    * Take one step of a memcg-tree writeback walk driven by the caller's
>>    * iterator, and fold the result into @s, the retry bookkeeping shared
>>    * across steps. @memcg is the iterator's current memcg, or NULL once
>>    * it has wrapped around after a full pass over the tree.
>>    *
>>    * The function returns -EBUSY to signal the caller to abort the walk when
>>    * either of the following occurs:
>>    * - A full pass over the tree found no writeback-candidate memcg.
>>    * - Shrinking a writeback-candidate memcg failed MAX_RECLAIM_RETRIES
>> times.
>>    *
>>    * When memory cgroup is disabled, the iterator always yields NULL. All
>>    * zswap entries then live on the root list_lru, so NULL is treated as the
>>    * root memcg and shrunk directly rather than as a completed tree pass.
> 
> I think this chunk should be moved above the code returning -EBUSY
> when mem_cgroup_disabled() is true, and probably made more succinct as
> it should be obvious.

Done in v5.

> 
>>    *
>>    * Return: The number of compressed bytes written back (>= 0), or -EBUSY
>>    * when the caller should abort the walk.
>>    */
>> static long zswap_shrink_one_memcg(struct mem_cgroup *memcg,
>>                      struct zswap_shrink_state *s)
>> {
>>       bool disabled = mem_cgroup_disabled();
> 
> No need to store this in a variable AFAICT, it's a static branch and
> it's clearer to just call it directly in both call sites imo.

Done, and I've just submitted the v5 patch.

Thanks,
Hao
> 
>>       long shrunk;
>>
>>       /*
>>        * If the iterator has completed a full pass, update the shrink state
>>        * and check whether we should keep going.
>>        * With memcg disabled the iterator always yields NULL, so fall through
>>        * and shrink the root memcg directly instead.
>>        */
>>       if (!memcg && !disabled) {
>>           /*
>>            * Abort if no writeback-candidate memcgs in the last tree walk.
>>            * Otherwise reset the scans count and continue.
>>            */
>>           if (!s->scans)
>>               return -EBUSY;
>>           s->scans = 0;
>>           return 0;
>>       }
>>
>>       shrunk = shrink_memcg(memcg, NR_ZSWAP_WB_BATCH);
>>
>>       /*
>>        * There are no writeback-candidate pages in the memcg. With memcg
>>        * enabled this is not an issue as long as we can find another memcg
>>        * with pages in zswap, so skip without counting it as a candidate.
>>        * With memcg disabled the root LRU is the only target, so we should
>>        * abort if it has no writeback-candidate pages.
>>        */
>>       if (shrunk == -ENOENT)
>>           return disabled ? -EBUSY : 0;
>>       s->scans++;
>>
>>       if (shrunk <= 0 && ++s->failures == MAX_RECLAIM_RETRIES)
>>           return -EBUSY;
>>
>>       return shrunk;
>> }