Re: [PATCH v6.12 hotfix] mm/zswap: fix inconsistent charging when zswap_store_page() fails

[Hyeonggon Yoo <42.hyeyoo@gmail.com>]

On Tue, Jan 28, 2025 at 5:50 PM Hyeonggon Yoo <42.hyeyoo@gmail.com> wrote:
>
> Commit b7c0ccdfbafd ("mm: zswap: support large folios in zswap_store()")
> mistakenly skipped charging any zswapped pages when a single call to
> zswap_store_page() failed, even if some pages in the folio are
> successfully stored in zswap.
>
> Making things worse, these not-charged pages are uncharged in
> zswap_entry_free(), making zswap charging inconsistent.
>
> This inconsistency triggers two warnings when following these steps:
>   # On a machine with 64GiB of RAM and 36GiB of zswap
>   $ stress-ng --bigheap 2 # wait until the OOM-killer kills stress-ng
>   $ sudo reboot
>
>   Two warnings are:
>     in mm/memcontrol.c:163, function obj_cgroup_release():
>       WARN_ON_ONCE(nr_bytes & (PAGE_SIZE - 1));
>
>     in mm/page_counter.c:60, function page_counter_cancel():
>       if (WARN_ONCE(new < 0, "page_counter underflow: %ld nr_pages=%lu\n",
>           new, nr_pages))
>
> Charge zswapped pages even if some pages of the folio are not zswapped.
> After resolving the inconsistency, these warnings disappear.
>
> Fixes: b7c0ccdfbafd ("mm: zswap: support large folios in zswap_store()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
>  mm/zswap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 6504174fbc6a..92752cd05c75 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1568,20 +1568,20 @@ bool zswap_store(struct folio *folio)
>
>                 bytes = zswap_store_page(page, objcg, pool);
>                 if (bytes < 0)
> -                       goto put_pool;
> +                       goto charge_zswap;
>                 compressed_bytes += bytes;
>         }
>
> -       if (objcg) {
> -               obj_cgroup_charge_zswap(objcg, compressed_bytes);
> -               count_objcg_events(objcg, ZSWPOUT, nr_pages);
> -       }
> -
>         atomic_long_add(nr_pages, &zswap_stored_pages);
>         count_vm_events(ZSWPOUT, nr_pages);

Wait, it also does not account for these events/counters.
Will send v2. Sorry for the noise.

If I missed anything else, please let me know.

>         ret = true;
>
> +charge_zswap:
> +       if (objcg) {
> +               obj_cgroup_charge_zswap(objcg, compressed_bytes);
> +               count_objcg_events(objcg, ZSWPOUT, nr_pages);
> +       }
>  put_pool:
>         zswap_pool_put(pool);
>  put_objcg:
> --
> 2.47.1
>