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 A5D88C7EE23 for ; Thu, 8 Jun 2023 16:48:49 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1CA3C8E0002; Thu, 8 Jun 2023 12:48:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 17B656B0075; Thu, 8 Jun 2023 12:48:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0415B8E0002; Thu, 8 Jun 2023 12:48:48 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id E8A806B0074 for ; Thu, 8 Jun 2023 12:48:48 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 8E02E40353 for ; Thu, 8 Jun 2023 16:48:48 +0000 (UTC) X-FDA: 80880164736.13.6295284 Received: from mail-qk1-f177.google.com (mail-qk1-f177.google.com [209.85.222.177]) by imf24.hostedemail.com (Postfix) with ESMTP id 6F626180006 for ; Thu, 8 Jun 2023 16:48:46 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=cmpxchg-org.20221208.gappssmtp.com header.s=20221208 header.b=KWBJf47W; spf=pass (imf24.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.177 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1686242926; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eh7ovuRzwnQF+N3S+z1MROh6pC2Xrfb7xbMh6ffJFxQ=; b=sgdEGEgfEFHaLZ/q2SErTPOt6CH7kABsFaWMb6G4/V08YW7CNT8yvSlMZU77EJPO9jbCIl 897iyubyv6ZZjL/MYC2fAat7V5GIS+zsnJaKp4tjlMaIwvYZcz/hjUKC35zBhnxWQcvpgI Mdj49cmv5zXI9AM44sl2Eq+pl1E7+CI= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1686242926; a=rsa-sha256; cv=none; b=rmedIivazfZMH2/JrjIq5FhTc6DR2faZOtyMjkbHUpboy8pgUChaZuC0Q/c610ssYQ2F4Q cquuEGMiNrGSBHa8876PDwfHMmcZ0ZtiR8gbMaWOdDor0qHLKH2xJUPId965JKM+Ho3ces 0f9GCWHPc+FoGUl3krAg5qbxOro/aWU= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=cmpxchg-org.20221208.gappssmtp.com header.s=20221208 header.b=KWBJf47W; spf=pass (imf24.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.222.177 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org Received: by mail-qk1-f177.google.com with SMTP id af79cd13be357-75d5469c856so72287685a.2 for ; Thu, 08 Jun 2023 09:48:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20221208.gappssmtp.com; s=20221208; t=1686242925; x=1688834925; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=eh7ovuRzwnQF+N3S+z1MROh6pC2Xrfb7xbMh6ffJFxQ=; b=KWBJf47Wd4Jr3NOyKj9Bp8reo1vLdCc/4FYcz9oS0+TvYg4TqE18pKlKbGEdaa/iWy zd9Ysx/FHezhwFZ38jIurN40SFgseEyyeXgVODTzfYmPlWqGQ/P4h2SQSurx5iL+KA6B TiAE3RSLOFkG/oM4ptTxIizIwvVSrqKIAdIK2xLD843NdR9fmidBhsoy4dW73Wmhq0Ln GIHBEknDMdrdYaiW1JXf0M5E1xgXDAtRH7KUgw4jHQxD3Ei3ch8OjOyYCwj7U119hkzG mjIXPxMaRJWd2JqLadd7ts1tCmzR+jJ3vwiRCCcCNTjTFPGzNROL/elMH6+j+p+Lz9LF VAiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686242925; x=1688834925; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=eh7ovuRzwnQF+N3S+z1MROh6pC2Xrfb7xbMh6ffJFxQ=; b=BpCOlAzSSC0w9hLlvy22tUuAPwB9bu+aT2eOID+XHNMgQyR0Eczw3wTOhYGlN24mhz NYK5OXRKLKa+Ha337MPN5X2DY4210IgSYkwaYf+fwgkLuq4cogDB7I3nsC8ehUk7OvVq vQ/KaxJk6rOi04M+bLOnfdme5U4+9g4ygJhm3LbuEbOYvpbIFuvzYPMcdEghJDktmS0D B59OmRdM6VcFSnJS22y5NurZZhMdzQQSOqJg5b6IVM3ePXzvMYVm+USar2QtVMxsSlAv MOv6yXS5O2hItKXd/knExSmrObxy6KCEhvTjIwH+aBbetUSyxs5aYYa6geBj2r9El/14 zkOw== X-Gm-Message-State: AC+VfDxyCv+rRrH2eoR3jrl0LGlZU+MSx9siekAXcNjkQ5NeAexm1VcD Q486iyE1omDvQVNjTA4Suh+eCPWiExRIem/nymA= X-Google-Smtp-Source: ACHHUZ4DWP4YHK7s4uiExJLyVWdUVHB0evCNiHq/6Xc0Uvk97WnRft+vlfpDHW4CiR/Aj3scHjMAWQ== X-Received: by 2002:a05:620a:4690:b0:75d:5321:fa40 with SMTP id bq16-20020a05620a469000b0075d5321fa40mr6248864qkb.51.1686242925405; Thu, 08 Jun 2023 09:48:45 -0700 (PDT) Received: from localhost (2603-7000-0c01-2716-8f57-5681-ccd3-4a2e.res6.spectrum.com. [2603:7000:c01:2716:8f57:5681:ccd3:4a2e]) by smtp.gmail.com with ESMTPSA id 22-20020a05620a06d600b0074fafbea974sm457688qky.2.2023.06.08.09.48.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Jun 2023 09:48:45 -0700 (PDT) Date: Thu, 8 Jun 2023 12:48:44 -0400 From: Johannes Weiner To: Domenico Cerasuolo 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 Subject: Re: [RFC PATCH v2 6/7] mm: zswap: simplify writeback function Message-ID: <20230608164844.GF352940@cmpxchg.org> References: <20230606145611.704392-1-cerasuolodomenico@gmail.com> <20230606145611.704392-7-cerasuolodomenico@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230606145611.704392-7-cerasuolodomenico@gmail.com> X-Stat-Signature: bsnu3qcz9xqysti71953c76bpmnygs5e X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 6F626180006 X-Rspam-User: X-HE-Tag: 1686242926-692655 X-HE-Meta: U2FsdGVkX19Vs0bduf7YwVZ53fkO5a42fvdYKnigkUN5EglWTWrmPXWnokGwDj3XOgEo/a7yCVYk+6VzsjsrZ04bJElMr7KuQJG+ZhWn5RwfiWs9dxuxhefqykYa0lGkcJhcTZh2zmjnNXIq4x4BLiRlQzVFwKOx2P+8UB19gQmcsScGUgSEJqFG/M0PpudflitUhqsgilGyDm5P794T0MnS4KoLnHcY9MJ8fuPqC90zrB6k/k7MD8WkmAV2LOrljXW5STszq+dnFs8n1ipp16Pd1bMKsNZYQQFDqWdKAhsbuPoABMc8mKxztgAr5eMGMy51NqHemw00QFbwMgMiXBjzmh9R4hgIrUHlGNu1+/E1ZbDJrajzKAQYUT9FKJUoNMxGBC+2cx3Rn3ZmKcDHFR/9Z4D7fVlNZQm//3cuZtbQIxqKFt8QNPQOFRw7bk1sqgbp6W3985e59wq0UuRpMmGXmz7EsPtLHKuM1AbCIcumwv4Yc0+mTKXQSsaDpK1pw8OzDgYKqyMcijcypZrNjQY1tFmXtqEKFHlhaphzzr2Iqxab97xpk0KfRwnSEGMhKmmUeuGJXHLXjAnWr7dPDX76+X6oBtRIPsy1I/B7sP2V7vodaqpymeJcxGtxRHUCRx7TqHBSn33dSdam23SvIfXaGhLArEeH2cnWLPy4TAHCUJMk40xFW1Fx+Q5yi036G9T9lq4oVRYx+ZFKLTYn3Pbcx5StWfhtiGb5pqyLhmNRwhmZklrhjcRC2+JI1Ex2GT03n7xQyf+nTUU16TNIEIR7NjVyVImKkNl6T+eADB6UeXIx8rARp0pJQ3R4McDNO13YHhJanOj5KE/hQiSL+3FGN9Do2LgV5OyUaDrsOqt8ACbInpOaH6qUnPZSwp8Q+V5DD84Ec7pDkGvfUVmFVK7T6KFfQ7TWnLojvlpskugnclCkeB9XjqRlGS5qEMUPKTnwIXoi2AQ4Y4ymjTp OYAONt1s h4qdjA8EziplPHZ5gfEdr2uWbCMWFHo0hqwN+NFL/OC4Hx8pycPaKGd40qTcUKrVHVkHMWADsmRecNv58y33ygeWFK7JOZlJPnbkqwNrDwYL8yGMHaL+YFfLxnRfs1N0zRibdzmFAlvb4QwaaZnd0VVGy4FfpboHn89j0XSgGzKifCFiOsgjL3W6bDtVTLIQ0WfQuDhMQ5kd9G9wnx0ezkYGhZcloGXiwaNNK1wIi8U1BoP6zLKoqQJHL8CCq5JAyGvVqiYsqEGYfbQrhc8kJu3lU4N46IjT2lDLLGeNC2qXPMKyPVFAeGnzAUH7tpX995sHczpieJps7Kw4zXoIvZh7MY7PbiogeeDUiyta7EDFGIK5Zsdxsx7PyB2pyPd0MbnC8Ve0miUwpEX8CJz/bUpKQlp576O9fxXGGX2SthYpamUZXueQ1FQj0GmThs1Z16AQiAwtsBkJv1J4= 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: 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 drivers > 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. > 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 handle); > +static int zswap_writeback_entry(struct zswap_entry *entry, struct zswap_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 = zswap_writeback_entry(pool->zpool, lru_entry->handle); > + ret = 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 == zswap_rb_search(&tree->rbroot, swpoffset)) > + 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 IO. */ if (lru_entry == 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; 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 already * took it out while we had the tree->lock released for IO. */ if (entry == zswap_rb_search(&tree->rb_root, swpoffset)) zswap_invalidate_entry(tree, entry); Would you care to send a follow-up patch?