From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 23091C77B7C for ; Thu, 4 May 2023 07:22:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 752F9900003; Thu, 4 May 2023 03:22:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 70358900002; Thu, 4 May 2023 03:22:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 61B4C900003; Thu, 4 May 2023 03:22:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by kanga.kvack.org (Postfix) with ESMTP id 4A8A6900002 for ; Thu, 4 May 2023 03:22:56 -0400 (EDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A38F760D57; Thu, 4 May 2023 07:22:55 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ACA23C433EF; Thu, 4 May 2023 07:22:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1683184975; bh=qJE+0VDf65DzGrnz6OhSis5f42jRxQcKqBnVkpXUuFQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GcTtTYdDdvzZ5IUfTarqxjQUJBbA04A876488HjBrItLI98yBEMu7NXhfb1VVc3kS WCwtcsbw1q5JbWNaUquAm5MeftwfQYwm/F1Wrv1srXVO7z6UwsKtsja+KcEvZUYLCy kR+LWYRs5cvd7pwzuZ7i3t+tnOOIXlF2K6gK9Ax57OPlTtPHudCVtS2vcY5ydzGO3R wqS4fUCqNtZ2ludeB1h4V/HdQAESIiYL7NDDOfba5zoYDsPjG8k4TdZCVveEU3qCLD FW4BsbiYcNh9L38Bz9WmSWO8ld6SQlYgNrCRPIO5WylgHcErDyDJHbxn2VRZtD9s8E yp2wAtscjDRJg== Date: Thu, 4 May 2023 00:22:53 -0700 From: Chris Li To: Johannes Weiner Cc: Domenico Cerasuolo , sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com, minchan@kernel.org, ngupta@vflare.org, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] mm: fix zswap writeback race condition Message-ID: References: <20230503151200.19707-1-cerasuolodomenico@gmail.com> <20230504022904.GA202700@cmpxchg.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230504022904.GA202700@cmpxchg.org> X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, May 03, 2023 at 10:29:04PM -0400, Johannes Weiner wrote: > > > > > > case ZSWAP_SWAPCACHE_NEW: /* page is locked */ > > ^^^^^^^^^^^^^^^^^^^^ > > > > + /* > > > + * Having a local reference to the zswap entry doesn't exclude > > > + * swapping from invalidating and recycling the swap slot. Once > > > + * the swapcache is secured against concurrent swapping to and > > > + * from the slot, recheck that the entry is still current before > > > + * writing. > > > + */ > > > + spin_lock(&tree->lock); > > > + if (zswap_rb_search(&tree->rbroot, entry->offset) != entry) { > > > + spin_unlock(&tree->lock); > > > + delete_from_swap_cache(page_folio(page)); > > > + ret = -ENOMEM; > > > + goto fail; > > > + } > > > + spin_unlock(&tree->lock); > > > + > > > > The race condition is still there, just making it much harder to hit. > > What happens after you perform the rb tree search, release tree lock. > > Then the entry gets invalid and recycled right here before the decompress > > step? > > Recyling can only happen up until we see ZSWAP_SWAPCACHE_NEW. > > Once we see it, we're holding the page lock* on a new swapcache page > for a valid, in-use** swp_entry_t. > > The lock of the swapcache page prevents swapin, which would be > required for the count to drop and the entry to be recycled. Thanks for the explain. I miss the locked page will prevent swapin part. > __read_swap_cache_async() checked that the entry is valid, so the slot > cannot be allocated to someone else. > > Now we just have to check if that entry is the right one, iow the slot > wasn't recycled. > > If the slot wasn't recycled, we know we have the right data and we can > start the IO and unlock the page. (After that swapins can continue and > the data can change, but regular writeback vs redirtying rules apply.) > > If the slot was indeed recycled before we get ZSWAP_SWAPCACHE_NEW, we > see the mismatch, delete the page from the swapcache and unlock it. A > racing do_swap_page() may have found and reffed the page in swapcache, > and acquire the page lock after us; but it'll see it's no longer in > the swapcache, drop the reference (free the page) and retry the fault. LGTM then. Please feel free to add: Reviewed-by: Chris Li (Google) Chris