Re: [PATCH v4] mm/zswap: store <PAGE_SIZE compression failed page as-is

[SeongJae Park]

On Thu, 21 Aug 2025 14:21:11 -0700 Chris Li <chrisl@xxxxxxxxxx> wrote:

> On Tue, Aug 19, 2025 at 12:34 PM SeongJae Park <sj@xxxxxxxxxx> wrote:
[...]
> > Historically, writeback disabling was introduced partially as a way to
> > solve the LRU order issue.  Yosry pointed out[4] this is still suboptimal
> > when the incompressible pages are cold, since the incompressible pages
> > will continuously be tried to be zswapped out, and burn CPU cycles for
> > compression attempts that will anyway fail.  One imaginable solution for
> > the problem is reusing the swapped-out page and its struct page to store
> > in the zswap pool.  But that's out of the scope of this patch.
> >
> > [1] https://github.com/sjp38/eval_zswap/blob/master/run.sh
> > [2] https://lore.kernel.org/20231017003519.1426574-3-nphamcs@xxxxxxxxx
> > [3] https://lore.kernel.org/20240706022523.1104080-6-flintglass@xxxxxxxxx
> > [4] https://lore.kernel.org/CAJD7tkZXS-UJVAFfvxJ0nNgTzWBiqepPYA4hEozi01_qktkitg@xxxxxxxxxxxxxx
[...]
> > +       /*
> > +        * If a page cannot be compressed into a size smaller than PAGE_SIZE,
> > +        * save the content as is without a compression, to keep the LRU order
> > +        * of writebacks.  If writeback is disabled, reject the page since it
> > +        * only adds metadata overhead.  swap_writeout() will put the page back
> > +        * to the active LRU list in the case.
> > +        */
> > +       if (comp_ret || !dlen) {
> 
> Looks good other than the feedback provided by Barry as well. Need to
> handle the -ENOSPC.
> Other errors will depend on your plan to drop this counter or not. I
> will wait for your next version.

Ack.  The next version will keep -ENOSPC comp_ret value so that
reject_compress_poor counter is not broken, like I replied to Barry.

> 
> 
> > +               zswap_crypto_compress_fail++;
> > +               dlen = PAGE_SIZE;
> > +       }
> > +       if (dlen >= PAGE_SIZE) {
> > +               if (!mem_cgroup_zswap_writeback_enabled(
> > +                                       folio_memcg(page_folio(page)))) {
> > +                       comp_ret = -EINVAL;
> > +                       goto unlock;
> I saw you mention this in the cover letter, so just to confirm we are
> on the same page. Current patch still has the issue [4] of write back
> disabled cases, the incompressible page will stay in the page LRU and
> possibly attempt to reclaim over and over again, right?

You are correct.  This patch is not making a change for writeback disabled
cases.


Thanks,
SJ

[...]