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

[SeongJae Park <sj@kernel.org>]

On Wed, 27 Aug 2025 10:33:38 -0700 Chris Li <chrisl@kernel.org> wrote:

> On Wed, Aug 27, 2025 at 9:18 AM SeongJae Park <sj@kernel.org> wrote:
> >
> > On Tue, 26 Aug 2025 08:52:35 -0700 Chris Li <chrisl@kernel.org> wrote:
> >
> > > Hi SeongJae,
> > >
> > > I did another pass review on it. This time with the editor so I saw
> > > more source code context and had more feedback.
> > > Mostly just nitpicks. See the detailed comments below.
> >
> > Thank you for your review!
> 
> Thank you for the good work.
> 
> >
> > >
> > > On Fri, Aug 22, 2025 at 12:08 PM SeongJae Park <sj@kernel.org> wrote:
[...]
> > """
> > --- a/mm/zswap.c
> > +++ b/mm/zswap.c
> > @@ -952,6 +952,7 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry,
> >         struct zpool *zpool;
> >         gfp_t gfp;
> >         u8 *dst;
> > +       bool dst_need_unmap = false;
> 
> A bit nitpicky. That variable name is too long as a C local variable.
> We want local auto variables to be short and sweet. That is why you
> have "dst" rather than  "u8 *destination_compressed_buffer;"
> The local variable name is too long and it can hurt the reading as well.
> Can we have something shorter? e.g. "mapped" or "has_map"

I agree your points, and thank you for suggestions.  I will take "mapped".

[...]
> > > > @@ -1007,6 +1031,14 @@ static bool zswap_decompress(struct zswap_entry *entry, struct folio *folio)
> > > >         acomp_ctx = acomp_ctx_get_cpu_lock(entry->pool);
> > > >         obj = zpool_obj_read_begin(zpool, entry->handle, acomp_ctx->buffer);
> > > >
> > > > +       /* zswap entries of length PAGE_SIZE are not compressed. */
> > > > +       if (entry->length = PAGE_SIZE) {
> > > > +               memcpy_to_folio(folio, 0, obj, entry->length);
> > >
> > > The following read_end() followed by acomp unlock() duplicates the
> > > normal decompress ending sequence. It will create complications when
> > > we modify the normal ending sequence in the future, we need to match
> > > it here.How about just goto the ending sequence and share the same
> > > return path as normal:
> > >
> > >  +                  goto read_done;
> > >
> > > Then insert the read_done label at ending sequence:
> > >
> > >         dlen = acomp_ctx->req->dlen;
> > >
> > > + read_done:
> > >         zpool_obj_read_end(zpool, entry->handle, obj);
> > >         acomp_ctx_put_unlock(acomp_ctx);
> >
> > I agree your concern and this looks good to me :)
> >
> > >
> > > If you adopt that, you also will need to init the comp_ret to "0"
> > > instead of no init value in the beginning of the function:
> > >
> > >         struct crypto_acomp_ctx *acomp_ctx;
> > > -        int decomp_ret, dlen;
> > > +       int decomp_ret = 0, dlen;
> > >         u8 *src, *obj;
> >
> > We may also need to initialize 'dlen' as PAGE_SIZE ?
> 
> If there is a code path can lead to dlen use not initialized value? If
> not then we don't have to assign it.

The success return path of zswap_decompress() checks dlen together with
decomp_ret as below.  So I think we need to initialize dlen, too.  Please let
me know if I'm missing something.

    if (!decomp_ret && dlen == PAGE_SIZE)
            return true;

[...]
> > Thank you for your kind review and nice suggestions!  Since the change is
> > simple, I will post a fixup patch as reply to this, for adopting your
> > suggestions with my additional changes (adding dst_need_unmap bool variable on
> > zswap_compress(), and initializing dlen on zswap_decompress()) if you have no
> > objection or different suggestions for the my addition of the changes.  Please
> > let me know if you have any concern or other suggestions for my suggested
> > additional changes.
> 
> I am fine with a fix up patch or a new version. Does not matter to me
> in the slightest. I care more about the final landing code itself more
> than which vehicle to carry the code.  Assume you do all those fix up
> you mention above, you can have my Ack in your fix up:
> 
> Acked-by: Chris Li <chrisl@kernel.org>

Thank you, Chris!

I will post the fixup by tomorrow morning (Pacific time) unless I
hear other opinions or find my mistakes on the above plan by tonight.


Thanks,
SJ

[...]