Re: [RFC PATCH v2 3/7] mm, swap: support physical swap as a vswap backend

[Yosry Ahmed]

> > > +       if (swap_is_vswap(si)) {
> > > +               if (entry != vswap_zswap_load(swpentry)) {
> > > +                       ret = -ENOMEM;
> > > +                       goto out;
> > > +               }
> > > +               /*
> > > +                * Allocate physical backing BEFORE decompress - if it fails,
> > > +                * no wasted work. folio_realloc_swap sets vtable to PHYS,
> > > +                * overwriting ZSWAP - the old entry pointer is only held
> > > +                * by the caller now.
> > > +                */
> > > +               phys = folio_realloc_swap(folio);
> > > +               if (!phys.val) {
> > > +                       ret = -ENOMEM;
> > > +                       goto out;
> > > +               }
> > > +       } else {
> > > +               tree = swap_zswap_tree(swpentry);
> > > +               if (entry != xa_load(tree, offset)) {
> > > +                       ret = -ENOMEM;
> > > +                       goto out;
> > > +               }
> >
> > There's a lot of divergence in the code (in this patch and previous
> > ones). Seems like a lot of it is to do xarray operations vs vswap
> > operations. I wonder if we can abstract these into helpers, e.g.
> > zswap_tree_store(), zswap_tree_load(), etc. Maybe the name is not the
> > best, but you get the point :)
>
> How about zswap_entry_load() and zswap_entry_store()? :)

Even better!


> > > -       xa_erase(tree, offset);
> > > +       if (!swap_is_vswap(si))
> > > +               xa_erase(tree, offset);
> >
> > Maybe this can also be abstracted into a helper, but I wonder what the
> > corresponding vswap operation would be. I think folio_realloc_swap()
> > will have already "erased" the zswap entry from vswap. Maybe have a
>
> Yup that's the right logic. We already change the backend to physical
> swap slot here, so there's no real "erase".
>
> > vswap helper that will only remove it if it's a zswap entry? We can
> > probably do a lockless check first to make it cheap?
> >
> > It's probably silly to do this, and maybe there's a better way.
> > Generally, I think the code would be easier to follow if we abstract
> > away the xarray vs. vswap stuff into helpers (where it's reasonable).
>
> I'm not entirely sure if its worth it either, yeah. Unlike load and
> store, erase seems a bit asymmetric in the sense that we only need to
> do it for non-vswap cases.

Yeah :/

Maybe just add a comment why no erase is needed for the vswap case.