Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
MIME-Version: 1.0
References: <20231113130601.3350915-1-hezhongkun.hzk@bytedance.com> <CAJD7tkY8SwROmNEaBAhkS4OKj33g-6fHsKFeYKW3afT+yAbvxA@mail.gmail.com>
In-Reply-To: <CAJD7tkY8SwROmNEaBAhkS4OKj33g-6fHsKFeYKW3afT+yAbvxA@mail.gmail.com>
From:   =?UTF-8?B?6LS65Lit5Z2k?= <hezhongkun.hzk@bytedance.com>
Date:   Wed, 15 Nov 2023 20:53:40 +0800
Message-ID: <CACSyD1P6RwjWpKhvNWEo77LKpTrnKOzT-+TV+GnQu_g-ADYbcQ@mail.gmail.com>
Subject: Re: [External] Re: [PATCH] mm:zswap: fix zswap entry reclamation
 failure in two scenarios
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org, nphamcs@gmail.com,
        sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Ying <ying.huang@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> For case (1), I think a cleaner solution would be to move the
> zswap_invalidate() call from swap_range_free() (which is called after
> the cached slots are freed) to __swap_entry_free_locked() if the usage
> goes to 0. I actually think conceptually this makes not just for
> zswap_invalidate(), but also for the arch call, memcg uncharging, etc.
> Slots caching is a swapfile optimization that should be internal to
> swapfile code. Once a swap entry is freed (i.e. swap count is 0 AND
> not in the swap cache), all the hooks should be called (memcg, zswap,
> arch, ..) as the swap entry is effectively freed. The fact that
> swapfile code internally batches and caches slots should be
> transparent to other parts of MM. I am not sure if the calls can just
> be moved or if there are underlying assumptions in the implementation
> that would be broken, but it feels like the right thing to do.

Good idea,  This is indeed a clear solution.  I'll try it in another
patch later.

>
> For case (2), I don't think zswap can just decide to free the entry.
>
> In that case, the page is in the swap cache pointing to a swp_entry
> which has a corresponding zswap entry, and the page is clean because
> it is already in swap/zswap, so we don't need to write it out again
> unless it is redirtied. If zswap just drops the entry, and reclaim
> tries to reclaim the page in the swap cache, it will drop the page
> assuming that there is a copy in swap/zswap (because it is clean). Now
> we lost all copies of the page.
>
> Am I missing something?
>

Ah, my bad.  Missed the step of marking the page as dirty.
Please have a look,  just like zswap_exclusive_loads_enabled,
set page dity so that it can be pageout again.
       if (!page_was_allocated) {
              if (!count) {
                       set_page_dirty(page);
                       ret = 0;
               } else
                       ret = -EEXIST;
               put_page(page);
}
Thanks  for your feedback, Yosry.