Re: [PATCH v2 2/3] mm/zswap: fix race between lru writeback and swapoff

[Johannes Weiner <hannes@cmpxchg.org>]

On Tue, Jan 30, 2024 at 10:30:20AM +0800, Chengming Zhou wrote:
> On 2024/1/30 08:22, Yosry Ahmed wrote:
> > On Sun, Jan 28, 2024 at 01:28:50PM +0000, Chengming Zhou wrote:
> >> @@ -860,40 +839,47 @@ static enum lru_status shrink_memcg_cb(struct list_head *item, struct list_lru_o
> >>  {
> >>  	struct zswap_entry *entry = container_of(item, struct zswap_entry, lru);
> >>  	bool *encountered_page_in_swapcache = (bool *)arg;
> >> -	struct zswap_tree *tree;
> >> -	pgoff_t swpoffset;
> >> +	swp_entry_t swpentry;
> >>  	enum lru_status ret = LRU_REMOVED_RETRY;
> >>  	int writeback_result;
> >>  
> >> +	/*
> >> +	 * Rotate the entry to the tail before unlocking the LRU,
> >> +	 * so that in case of an invalidation race concurrent
> >> +	 * reclaimers don't waste their time on it.
> >> +	 *
> >> +	 * If writeback succeeds, or failure is due to the entry
> >> +	 * being invalidated by the swap subsystem, the invalidation
> >> +	 * will unlink and free it.
> >> +	 *
> >> +	 * Temporary failures, where the same entry should be tried
> >> +	 * again immediately, almost never happen for this shrinker.
> >> +	 * We don't do any trylocking; -ENOMEM comes closest,
> >> +	 * but that's extremely rare and doesn't happen spuriously
> >> +	 * either. Don't bother distinguishing this case.
> >> +	 *
> >> +	 * But since they do exist in theory, the entry cannot just
> >> +	 * be unlinked, or we could leak it. Hence, rotate.
> > 
> > The entry cannot be unlinked because we cannot get a ref on it without
> > holding the tree lock, and we cannot deref the tree before we acquire a
> > swap cache ref in zswap_writeback_entry() -- or if
> > zswap_writeback_entry() fails. This should be called out explicitly
> > somewhere. Perhaps we can describe this whole deref dance with added
> > docs to shrink_memcg_cb().
> 
> Maybe we should add some comments before or after zswap_writeback_entry()?
> Or do you have some suggestions? I'm not good at this. :)

I agree with the suggestion of a central point to document this.

How about something like this:

/*
 * As soon as we drop the LRU lock, the entry can be freed by
 * a concurrent invalidation. This means the following:
 *
 * 1. We extract the swp_entry_t to the stack, allowing
 *    zswap_writeback_entry() to pin the swap entry and
 *    then validate the zwap entry against that swap entry's
 *    tree using pointer value comparison. Only when that
 *    is successful can the entry be dereferenced.
 *
 * 2. Usually, objects are taken off the LRU for reclaim. In
 *    this case this isn't possible, because if reclaim fails
 *    for whatever reason, we have no means of knowing if the
 *    entry is alive to put it back on the LRU.
 *
 *    So rotate it before dropping the lock. If the entry is
 *    written back or invalidated, the free path will unlink
 *    it. For failures, rotation is the right thing as well.
 *
 *    Temporary failures, where the same entry should be tried
 *    again immediately, almost never happen for this shrinker.
 *    We don't do any trylocking; -ENOMEM comes closest,
 *    but that's extremely rare and doesn't happen spuriously
 *    either. Don't bother distinguishing this case.
 */

> > We could also use a comment in zswap_writeback_entry() (or above it) to
> > state that we only deref the tree *after* we get the swapcache ref.
> 
> I just notice there are some comments in zswap_writeback_entry(), should
> we add more comments here?
> 
> 	/*
> 	 * folio is locked, and the swapcache is now secured against
> 	 * concurrent swapping to and from the slot. Verify that the
> 	 * swap entry hasn't been invalidated and recycled behind our
> 	 * backs (our zswap_entry reference doesn't prevent that), to
> 	 * avoid overwriting a new swap folio with old compressed data.
> 	 */

The bit in () is now stale, since we're not even holding a ref ;)

Otherwise, a brief note that entry can not be dereferenced until
validation would be helpful in zswap_writeback_entry(). The core of
the scheme I'd probably describe in shrink_memcg_cb(), though.

Can I ask a favor, though?

For non-critical updates to this patch, can you please make them
follow-up changes? I just sent out 20 cleanup patches on top of this
patch which would be super painful and error prone to rebase. I'd like
to avoid that if at all possible.