Re: [PATCH 5/5] mm/zswap: cleanup zswap_reclaim_entry()

[Johannes Weiner <hannes@cmpxchg.org>]

On Thu, Dec 14, 2023 at 02:41:26PM -0800, Yosry Ahmed wrote:
> On Thu, Dec 14, 2023 at 2:23 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Wed, 13 Dec 2023 17:02:25 -0800 Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > > On Tue, Dec 12, 2023 at 8:18 PM Chengming Zhou
> > > <zhouchengming@bytedance.com> wrote:
> > > >
> > > > Also after the common decompress part goes to __zswap_load(), we can
> > > > cleanup the zswap_reclaim_entry() a little.
> > >
> > > I think you mean zswap_writeback_entry(), same for the commit title.
> >
> > I updated my copy of the changelog, thanks.
> >
> > > > -       /*
> > > > -        * If we get here because the page is already in swapcache, a
> > > > -        * load may be happening concurrently. It is safe and okay to
> > > > -        * not free the entry. It is also okay to return !0.
> > > > -        */
> > >
> > > This comment should be moved above the failure check of
> > > __read_swap_cache_async() above, not completely removed.
> >
> > This?
> 
> Yes, thanks a lot. Although I think a new version is needed anyway to
> address other comments.
> 
> >
> > --- a/mm/zswap.c~mm-zswap-cleanup-zswap_reclaim_entry-fix
> > +++ a/mm/zswap.c
> > @@ -1457,8 +1457,14 @@ static int zswap_writeback_entry(struct
> >         mpol = get_task_policy(current);
> >         page = __read_swap_cache_async(swpentry, GFP_KERNEL, mpol,
> >                                 NO_INTERLEAVE_INDEX, &page_was_allocated, true);
> > -       if (!page)
> > +       if (!page) {
> > +               /*
> > +                * If we get here because the page is already in swapcache, a
> > +                * load may be happening concurrently. It is safe and okay to
> > +                * not free the entry. It is also okay to return !0.
> > +                */
> >                 return -ENOMEM;
> > +       }
> >
> >         /* Found an existing page, we raced with load/swapin */
> >         if (!page_was_allocated) {

That's the wrong branch, no?

!page -> -ENOMEM

page && !page_was_allocated -> already in swapcache

Personally, I don't really get the comment. What does it mean that
it's "okay" not to free the entry? There is a put, which may or may
not free the entry if somebody else is using it. Is it explaining how
lifetime works for refcounted objects? I'm similarly confused by the
"it's okay" to return non-zero. What is that trying to convey?

Deletion seemed like the right choice here, IMO ;)