Re: [PATCH v8 6/8] mm: zswap: Support large folios in zswap_store().

[Yosry Ahmed <yosryahmed@google.com>]

On Sat, Sep 28, 2024 at 7:15 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Sep 27, 2024 at 08:42:16PM -0700, Yosry Ahmed wrote:
> > On Fri, Sep 27, 2024 at 7:16 PM Kanchana P Sridhar
> > >  {
> > > +       struct page *page = folio_page(folio, index);
> > >         swp_entry_t swp = folio->swap;
> > > -       pgoff_t offset = swp_offset(swp);
> > >         struct xarray *tree = swap_zswap_tree(swp);
> > > +       pgoff_t offset = swp_offset(swp) + index;
> > >         struct zswap_entry *entry, *old;
> > > -       struct obj_cgroup *objcg = NULL;
> > > -       struct mem_cgroup *memcg = NULL;
> > > -
> > > -       VM_WARN_ON_ONCE(!folio_test_locked(folio));
> > > -       VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
> > > +       int type = swp_type(swp);
> >
> > Why do we need type? We use it when initializing entry->swpentry to
> > reconstruct the swp_entry_t we already have.
>
> It's not the same entry. folio->swap points to the head entry, this
> function has to store swap entries with the offsets of each subpage.

Duh, yeah, thanks.

>
> Given the name of this function, it might be better to actually pass a
> page pointer to it; do the folio_page() inside zswap_store().
>
> Then do
>
>                 entry->swpentry = page_swap_entry(page);
>
> below.

That is indeed clearer.

Although this will be adding yet another caller of page_swap_entry()
that already has the folio, yet it calls page_swap_entry() for each
page in the folio, which calls page_folio() inside.

I wonder if we should add (or replace page_swap_entry()) with a
folio_swap_entry(folio, index) helper. This can also be done as a
follow up.

>
> > >         obj_cgroup_put(objcg);
> > > -       if (zswap_pool_reached_full)
> > > -               queue_work(shrink_wq, &zswap_shrink_work);
> > > -check_old:
> > > +       return false;
> > > +}
> > > +
> > > +bool zswap_store(struct folio *folio)
> > > +{
> > > +       long nr_pages = folio_nr_pages(folio);
> > > +       swp_entry_t swp = folio->swap;
> > > +       struct xarray *tree = swap_zswap_tree(swp);
> > > +       pgoff_t offset = swp_offset(swp);
> > > +       struct obj_cgroup *objcg = NULL;
> > > +       struct mem_cgroup *memcg = NULL;
> > > +       struct zswap_pool *pool;
> > > +       size_t compressed_bytes = 0;
> >
> > Why size_t? entry->length is int.
>
> In light of Willy's comment, I think size_t is a good idea.

Agreed.