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 7184CC7EE2E for ; Fri, 9 Jun 2023 11:06:08 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BEDF38E0002; Fri, 9 Jun 2023 07:06:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B9CA58E0001; Fri, 9 Jun 2023 07:06:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A64388E0002; Fri, 9 Jun 2023 07:06:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 984EC8E0001 for ; Fri, 9 Jun 2023 07:06:07 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 63E971201F7 for ; Fri, 9 Jun 2023 11:06:07 +0000 (UTC) X-FDA: 80882929974.19.711DDC2 Received: from mail-pg1-f170.google.com (mail-pg1-f170.google.com [209.85.215.170]) by imf01.hostedemail.com (Postfix) with ESMTP id 7D42E40004 for ; Fri, 9 Jun 2023 11:06:05 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="oz03+L/a"; spf=pass (imf01.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=cerasuolodomenico@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686308765; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=YXzRh8LgZwDLUj+D3WXWLnveS92UjU0wAmFYLn9Q2uE=; b=Ye+ONxdUGirZQmtWhC3on1KwermI2tXr++WRZFBAN1W0/65u33QStBcnQePYdlSjTcldXI mdAi1c5YWYmccd6swVYXdIbgD4Sbcl24/ZvNbdFC1Mo33aHe33Mj0RBz0qujJzB00FDL2s LUwB7TszgV8Y4Bax2eey9c/bTEAVevk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686308765; a=rsa-sha256; cv=none; b=P7VvpSuFh+/eEp4jSWsCbUNGwsHRVZ6P1SuiUbH+LLsJtsbWp2afsMPxxDUVJsAd3XLEoP PrAb/s2VCPbueBUME5GdJ5mYzoK4V4CcnI4y2rejwJRmEaeaoyAEiebL7fHH6Hd4aY6U6R 8NCzwU0dOgsgeOOO5IWa483pbvXS8fg= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b="oz03+L/a"; spf=pass (imf01.hostedemail.com: domain of cerasuolodomenico@gmail.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=cerasuolodomenico@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pg1-f170.google.com with SMTP id 41be03b00d2f7-5144a9c11c7so670942a12.2 for ; Fri, 09 Jun 2023 04:06:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1686308764; x=1688900764; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=YXzRh8LgZwDLUj+D3WXWLnveS92UjU0wAmFYLn9Q2uE=; b=oz03+L/a+HXd9AqnlLpWxFJBLErTROuoyRbOGTCc4mPeadAzAMxHGjFiJdzb+SO26+ hKybnppI5RC9s5g8XflvMMGOLdhDxzUYsXoRJP8NES7NIKadRLZ1BqEOAI9tfJ623AVN GSD98x3EgSMzruZ02FFLQyfgl12RnNVyb1PIILiq4R3VgoQhmhQ+KA1jNAkf6KgYu0Qw IEdirRVEY7bDHJ2m6MJ00fT4Q98yw0bIl6GLlUqS2+Z7/kOZOcaeYlCdv4syTEPEL7Fu y3AsgFpCx5RMeuEf4bde+ggN80HMs40iIvs4j7QMoRvSdODVaPOuLAc9XeKJQZvO37M2 LZfA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686308764; x=1688900764; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YXzRh8LgZwDLUj+D3WXWLnveS92UjU0wAmFYLn9Q2uE=; b=gTRP4X3pYLuVlyTL4aenNaToHCA228L8K4bbfwzPlczypA/+I4CfWrMx1lD9J2/ETF uhGq+3EXSOYk3PkiWau9jY+seF3Jgq+2aiYiBEPA8P7O2UxX+b3hvO1HAwR8nbg3KQcz dy7BUY8fnbFSp9jjCdRQxoJjfW5/fe3fNd8k2L9M00iKf/N0V8XMWp4nPCSUH8r4qPXy v67DaikDMWe+I1CKVu3W3XYpoaAVQ1/Cqf6VeS6yD5nuWBCOpcJRlMEfbN7gTXpGJ0Z3 9AG7UoDHtCfWIvnfiQVTIHUsb/KGWzII9y94MzeDE3Z5OO5fB0opqSPP2EtxUJpY9rj4 gZsg== X-Gm-Message-State: AC+VfDxcyvwymy72ziKlUK31yLy4+R5ZqjD6b6DgTJbtGCr++xroMzrN u7BnnlYMMO7UrRw0Zcq3xroD5cg1TYe1JmcCgz0= X-Google-Smtp-Source: ACHHUZ7mkF6BFo9oUI+M4s7/lJCw04PgEQ1o0U/tWtR0tgzPuUCTZtjUKrbW28hjvytcgK+jAAk0tbSx1eWSk8ak20o= X-Received: by 2002:a17:90a:2d8e:b0:256:8e2d:1ec8 with SMTP id p14-20020a17090a2d8e00b002568e2d1ec8mr918208pjd.41.1686308764285; Fri, 09 Jun 2023 04:06:04 -0700 (PDT) MIME-Version: 1.0 References: <20230606145611.704392-1-cerasuolodomenico@gmail.com> <20230606145611.704392-7-cerasuolodomenico@gmail.com> <20230608164844.GF352940@cmpxchg.org> In-Reply-To: <20230608164844.GF352940@cmpxchg.org> From: Domenico Cerasuolo Date: Fri, 9 Jun 2023 13:05:53 +0200 Message-ID: Subject: Re: [RFC PATCH v2 6/7] mm: zswap: simplify writeback function To: Johannes Weiner Cc: vitaly.wool@konsulko.com, minchan@kernel.org, senozhatsky@chromium.org, yosryahmed@google.com, linux-mm@kvack.org, ddstreet@ieee.org, sjenning@redhat.com, nphamcs@gmail.com, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 7D42E40004 X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: jzto5yneuuiueonugtzzaabiwefeqmhz X-HE-Tag: 1686308765-74807 X-HE-Meta: U2FsdGVkX19jAZIhO7S3fEs26JX2QyV8IpOMNoxtv7TBMEj/WxeEvwC+CCwvMKsPbJODdO9eJnqumNSeaJnIILN4mCqtV8cwHD6r3jr53PbrvvP3LoC6NTCjvA2P/pmF/90J1F2r2oRwDnDiQrd+Ft9SZpCmApwYoSdYFQci0hCulLA/WZywknVVEbsrzYuBISk3JXQlDlf8jgFVYEt6YQOO55/4T+pfG+f+ZNcmTjfgccB5IdIq1T0wKCHL8VR/V2DDW7z1IJ+29GP5h2vi7ZlHW8oQx9QTfh4aGlkiedLTEsngUBeMZ7+yYNL5olEGriv15YqJhLLuCFSj8DJIwutakIMWffqZtrgwyhu4RUJ1yogtTkyjN7CAF3kS5PAGitsfuAUECMgt0NOpXwXFSMTEvc9eGfsIKOfcmnEbGeXodk1dlx9Sy3bj0G1JOU59MDzU+7dE1h4mWb7Zb3cPhxrxKIo/vukWGxsSOqf86NE71uXFfqkMhfjFy50a3+gnIUeNh8t5NQViClZwfFdvSmMw1LhFLS32oKQ+6axZ2tV3fRDVe3hDmCiAv4kX15G5Qmr3I7ts8yeqY1jTDXerOpRODIEnHwrso7HISIlinOS1mIqRaW4b991th2YL1NFYkjgbI5nEOViMHemyz98CcOZ7O87+WqrZ9Z+0xslnZAudrESc15aO2FEnAk9N0xUxs6HZfZKIMtEQkQxL5+3bmv72d1HReNjxQjgJjikuDlSZLlPkTCLXmtS+n0V+H7fCMdpnHKuse+9nC0Am0DQboD+HNzglaOgeieC0QdzR0E1WHw6+7cocO7X/N0nh0mXcdhOCwN7me6ISBrFoLjz++DeDf7FyP5VTaWdMFJP9PCepc/isjI69PRhK2yjAzdF1+NY7cbkM1s+OUvRbSqh4LjhrTqWdNhwa1I5cuyI18WlVGS+IkXBGtWcPYuGmfAdWiUiA6D0y8jHoql0ai7/ nvgyaDy9 BulAuTi8pstDPtcuiMVokwsdyeapNsLMM5SHlvCWWSp5+5MQnCERnlmiN0E17YtJ4cCDJHanOxRA1qf5HD4kq8kFYEVjtEP/D7JOQMIWEfwSyT9WGey98CqgEVCO5kPcfVyJjDShiy0+8PfjTJgZOcqGfgvaLWvIYUTQ5cys/4zNKR3QFecHHsgsowxTjFS3d3U/n9bYw10egOVc1514oK51lT4xG+JX+vNA1whkfn0YCgbDBiyHEHbdsayufN1+lHPLoKvLNCkP+e9zNXhFf58msjQb+uFiutScpf+Gi+qPjpOFRwGC0Ad7yLf1GI5vDxUT1FcL5ouHrKMl4so6LproZKfVkvkuEdXco98QrvAz/+M88AGFlTo2vvIuV4MJ9N8npy4HCJ3SQheH+FDTKxVdLWYmYA0sMoZoJwWZkXpNo4fuOM4MEYzRVjw== 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 Thu, Jun 8, 2023 at 6:48=E2=80=AFPM Johannes Weiner = wrote: > > Hi Domenico, > > On Tue, Jun 06, 2023 at 04:56:10PM +0200, Domenico Cerasuolo wrote: > > Previously, in zswap, the writeback function was passed to zpool driver= s > > for their usage in calling the writeback operation. However, since the > > drivers did not possess references to entries and the function was > > specifically designed to work with handles, the writeback process has > > been modified to occur directly within zswap. > > I'm having trouble parsing this sentence. > > > Consequently, this change allows for some simplification of the > > writeback function, taking into account the updated workflow. > > How about: > > zswap_writeback_entry() used to be a callback for the backends, which > don't know about struct zswap_entry. > > Now that the only user is the generic zswap LRU reclaimer, it can be > simplified: pass the pinned zswap_entry directly, and consolidate the > refcount management in the shrink function. Sounds clearer, will update. > > > Signed-off-by: Domenico Cerasuolo > > --- > > mm/zswap.c | 69 ++++++++++++++---------------------------------------- > > 1 file changed, 17 insertions(+), 52 deletions(-) > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 2831bf56b168..ef8604812352 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -250,7 +250,8 @@ static bool zswap_has_pool; > > pr_debug("%s pool %s/%s\n", msg, (p)->tfm_name, \ > > zpool_get_type((p)->zpool)) > > > > -static int zswap_writeback_entry(struct zpool *pool, unsigned long han= dle); > > +static int zswap_writeback_entry(struct zswap_entry *entry, struct zsw= ap_header *zhdr, > > + struct zswap_tree *tree); > > static int zswap_pool_get(struct zswap_pool *pool); > > static void zswap_pool_put(struct zswap_pool *pool); > > > > @@ -632,15 +633,21 @@ static int zswap_shrink(struct zswap_pool *pool) > > } > > spin_unlock(&tree->lock); > > > > - ret =3D zswap_writeback_entry(pool->zpool, lru_entry->handle); > > + ret =3D zswap_writeback_entry(lru_entry, zhdr, tree); > > > > spin_lock(&tree->lock); > > if (ret) { > > spin_lock(&pool->lru_lock); > > list_move(&lru_entry->lru, &pool->lru); > > spin_unlock(&pool->lru_lock); > > This could use a comment. > > /* Writeback failed, put entry back on LRU */ > > > + zswap_entry_put(tree, tree_entry); > > This put is a common factor in both branches, so you can consolidate. > > > + } else { > > + /* free the local reference */ > > + zswap_entry_put(tree, tree_entry); > > + /* free the entry if it's not been invalidated*/ > > Missing space between text and */ > > > + if (lru_entry =3D=3D zswap_rb_search(&tree->rbroot, swpof= fset)) > > + zswap_entry_put(tree, tree_entry); > > } > > - zswap_entry_put(tree, tree_entry); > > spin_unlock(&tree->lock); > > The success path freeing (hopefully the common path) is now > unfortunately hidden in fairly deep indentation. I think this would be > better with a goto for the error case. > > All together, something like this? > > if (ret) { > /* Writeback failed, put entry back on LRU */ > ... > goto put_unlock; > } > > /* > * Writeback started successfully, the page now belongs to the > * swapcache. Drop the base ref from the tree - unless invalidate > * already took it out while we had the tree->lock released for I= O. > */ > if (lru_entry =3D=3D zswap_rb_search(&tree->rb_root, swpoffset)) > zswap_entry_put(tree, entry); > > put_unlock: > /* Drop local reference */ > zswap_entry_put(tree, tree_entry); > spin_unlock(&tree->lock); > > return ret ? -EAGAIN : 0; > This feedback overlaps with the on in patch 1/7, I'm integrating them so that in patch #1, the invalidation check is done only with rb search and a first `goto unlock` for error. Then here the base reference-drop is added after the `ret` check, and errors go to `put_unlock`: if (ret) { /* Writeback failed, put entry back on LRU */ spin_lock(&pool->lru_lock); list_move(&entry->lru, &pool->lru); spin_unlock(&pool->lru_lock); goto put_unlock; } /* Check for invalidate() race */ if (entry !=3D zswap_rb_search(&tree->rbroot, swpoffset)) goto put_unlock; /* Drop base reference */ zswap_entry_put(tree, entry); put_unlock: /* Drop local reference */ zswap_entry_put(tree, entry); unlock: spin_unlock(&tree->lock); return ret ? -EAGAIN : 0; > Btw, it's unsettling that we drop the tree reference without > explicitly removing the entry for the tree. We rely on the final put > that frees the entry to do tree removal, but this doesn't happen if > somebody else is holding a reference; the entry can remain in the tree > long after we've officially handed over ownership of the data to > swapcache. TTBOMK there are currently no bugs because of that, but > it's a data corruption waiting to happen. > > This should be: > > /* > * Writeback started successfully, the page now belongs to the > * swapcache. Drop the entry from zswap - unless invalidate alrea= dy > * took it out while we had the tree->lock released for IO. > */ > if (entry =3D=3D zswap_rb_search(&tree->rb_root, swpoffset)) > zswap_invalidate_entry(tree, entry); > > Would you care to send a follow-up patch? Makes total sense, thanks will send a patch for this.