Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted

[Yosry Ahmed]

On Fri, Jun 7, 2024 at 4:28 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> On Sat, Jun 8, 2024 at 7:17 AM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >
> > [..]
> > > > One problem is that even if zswap was never enabled, the warning will
> > > > be emitted just if CONFIG_ZSWAP is on. Perhaps we need a variable or
> > > > static key if zswap was "ever" enabled.
> > > >
> > > > Barry, I suspect your is_zswap_enabled() check is deficient for
> > > > similar reasons, zswap could have been enabled before then became
> > > > disabled.
> > >
> > > I don't understand this. if zswap was enabled before but is disabled when
> > > I am loading data, will I get corrupted data before zswap was once enabled?
> > > If not, it seems nothing important.
> >
> > If zswap was enabled and then disabled, some pages may still be in
> > zswap. We do not load the pages from zswap when it is disabled, we
> > just stop storing new pages.
> >
> > So if you just rely in checking whether zswap is enabled at swapin
> > time to decide whether to use large folios, you may end up with a
> > situation where zswap is disabled, yet parts of the large folio you
> > are trying to swapin (or all of it) is in zswap.
> >
> > This is why I think we'll need to track whether zswap was ever enabled
> > instead (or if a page was ever stored).
>
> Thanks! It doesn't seem good. Do we have a simple way to clean zswap
> when it is disabled? seems not easy? Just like we do swapoff, or disable
> cache, we ensure they are clean - this is a real "disable".

I think it's just simpler, and disabling zswap at runtime is not that
common. Keep in mind that if zswap being disabled takes time, then
we'll want to handle races between zswap being disabled and incoming
swapins, it will probably end up being a state machine or so.

Without a valid use case, I think the complexity is not justified.
It's probably simpler to just track if zswap was ever used, and
disable large folio swapin. This should be a transitional state
anyway.