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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 12AE9C77B73 for ; Wed, 31 May 2023 23:12:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230458AbjEaXMl (ORCPT ); Wed, 31 May 2023 19:12:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35856 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230414AbjEaXMk (ORCPT ); Wed, 31 May 2023 19:12:40 -0400 Received: from mail-pf1-x435.google.com (mail-pf1-x435.google.com [IPv6:2607:f8b0:4864:20::435]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0F869A0 for ; Wed, 31 May 2023 16:12:37 -0700 (PDT) Received: by mail-pf1-x435.google.com with SMTP id d2e1a72fcca58-64d247a023aso198944b3a.2 for ; Wed, 31 May 2023 16:12:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20221208.gappssmtp.com; s=20221208; t=1685574756; x=1688166756; 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=NdrJ9MIJBRF6SJ1238zAPHjVfaouK03YhdkmBFLWYHU=; b=ID8921t0pn2fvFZSPX1ZCn10WYK0inJi+Xd18q6u9I2qj9Ua8QL/IeltWfVKU1549H 6t+FxjDrPHAhQhSB9JUjWETqcJHxAf+v7YLxwCZGsAuIUxonZkkFq7XdK1EhZEmH5lAp qYAaH4CiK8x4svKHXlCp+v2jNkN4mWFjTzogGYkohIubyyxVVOxf3JLsLl9kzOJPg/7J xxINvt2pxEnRtAX/ARJyUgMTJORyZOEFcNCRW7fuDHrc0oUvVcU2wtsStCp4UpkvLpMr DSNeoqv+/XYJerWdG5jeUU5678yDxAHf9x68P+rXEC/Q/fIjVLu3koKY+XmRzyE7za4f 4q6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685574756; x=1688166756; 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=NdrJ9MIJBRF6SJ1238zAPHjVfaouK03YhdkmBFLWYHU=; b=frH0V7MMMhOD/JlNfoszb/zuyMlfcLi+nXKerqrh5Ym8z260ErhH66UvGC4RLWo+1x KipEdpAQjBWKUR0Ja4+zwLEnN3XWg9+UZiz6diZ3QWXpcWyhYkyqXUWgGMcOeUvR5BsT SbATkmYTHFaC7+2B/rPkmxrMK8Uegzfl3mXg7OAthQUp3bkKIBtHxCkC6CLVPZTO3Ex6 J/VR6SMdgLDE6BcnGGS3H0Yg4bQd+3laoMireICa/AzJsZtS4CEzvy+Phbi4jvJa+Jdg ISc2GuohdGbRDGwKt73+gM9JaJVlVhIbsDbWL59qv3t7A8e3JNSJso7K7NCh84Cv6AYV hdzg== X-Gm-Message-State: AC+VfDyT8HG5jkCLarxNSCnz7sRRBCxQb/wgZiBxh1sSiYWFm0xOs+dx NoyOtGxllZvJsy2A4k9sG4CoFQ== X-Google-Smtp-Source: ACHHUZ6t82xplazJPqCDO0qJl0sdq89xmVCN5MjD9hEMlaWGG60n1KteElROnWc1nPytTu2S5dCmtw== X-Received: by 2002:a05:6a20:914e:b0:10f:500b:18a2 with SMTP id x14-20020a056a20914e00b0010f500b18a2mr5804009pzc.48.1685574756329; Wed, 31 May 2023 16:12:36 -0700 (PDT) Received: from dread.disaster.area (pa49-179-0-188.pa.nsw.optusnet.com.au. [49.179.0.188]) by smtp.gmail.com with ESMTPSA id s17-20020a170902ea1100b001b077301a58sm1965299plg.79.2023.05.31.16.12.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 31 May 2023 16:12:35 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1q4UzZ-006HqL-0r; Thu, 01 Jun 2023 09:12:33 +1000 Date: Thu, 1 Jun 2023 09:12:33 +1000 From: Dave Chinner To: Qi Zheng Cc: akpm@linux-foundation.org, tkhai@ya.ru, roman.gushchin@linux.dev, vbabka@suse.cz, viro@zeniv.linux.org.uk, brauner@kernel.org, djwong@kernel.org, hughd@google.com, paulmck@kernel.org, muchun.song@linux.dev, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-kernel@vger.kernel.org, Qi Zheng Subject: Re: [PATCH 4/8] fs: shrink only (SB_ACTIVE|SB_BORN) superblocks in super_cache_scan() Message-ID: References: <20230531095742.2480623-1-qi.zheng@linux.dev> <20230531095742.2480623-5-qi.zheng@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230531095742.2480623-5-qi.zheng@linux.dev> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 31, 2023 at 09:57:38AM +0000, Qi Zheng wrote: > From: Kirill Tkhai > > This patch prepares superblock shrinker for delayed unregistering. > It makes super_cache_scan() avoid shrinking of not active superblocks. > SB_ACTIVE is used as such the indicator. In case of superblock is not > active, super_cache_scan() just exits with SHRINK_STOP as result. > > Note, that SB_ACTIVE is cleared in generic_shutdown_super() and this > is made under the write lock of s_umount. Function super_cache_scan() > also takes the read lock of s_umount, so it can't skip this flag cleared. > > SB_BORN check is added to super_cache_scan() just for uniformity > with super_cache_count(), while super_cache_count() received SB_ACTIVE > check just for uniformity with super_cache_scan(). > > After this patch super_cache_scan() becomes to ignore unregistering > superblocks, so this function is OK with splitting unregister_shrinker(). > Next patches prepare super_cache_count() to follow this way. > > Signed-off-by: Kirill Tkhai > Signed-off-by: Qi Zheng > --- > fs/super.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/fs/super.c b/fs/super.c > index 2ce4c72720f3..2ce54561e82e 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -79,6 +79,11 @@ static unsigned long super_cache_scan(struct shrinker *shrink, > if (!trylock_super(sb)) > return SHRINK_STOP; > > + if ((sb->s_flags & (SB_BORN|SB_ACTIVE)) != (SB_BORN|SB_ACTIVE)) { > + freed = SHRINK_STOP; > + goto unlock; > + } This should not be here - the check to determine if the shrinker should run is done in the ->count method. If we removed the SB_ACTIVE flag between ->count and ->scan, then the superblock should be locked and the trylock_super() call above should fail.... Indeed, the unregister_shrinker() call in deactivate_locked_super() is done with the sb->s_umount held exclusively, and this happens before we clear SB_ACTIVE in the ->kill_sb() -> kill_block_super() -> generic_shutdown_super() path after the shrinker is unregistered. Hence we can't get to this check without SB_ACTIVE being set - the trylock will fail and then the shrinker_unregister() call will do it's thing to ensure the shrinker is never called again. If the change to the shrinker code allows the shrinker to still be actively running when we call ->kill_sb(), then that needs to be fixed. THe superblock shrinker must be stopped completely and never run again before we call ->kill_sb(). > if (sb->s_op->nr_cached_objects) > fs_objects = sb->s_op->nr_cached_objects(sb, sc); > > @@ -110,6 +115,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink, > freed += sb->s_op->free_cached_objects(sb, sc); > } > > +unlock: > up_read(&sb->s_umount); > return freed; > } > @@ -136,7 +142,7 @@ static unsigned long super_cache_count(struct shrinker *shrink, > * avoid this situation, so do the same here. The memory barrier is > * matched with the one in mount_fs() as we don't hold locks here. > */ > - if (!(sb->s_flags & SB_BORN)) > + if ((sb->s_flags & (SB_BORN|SB_ACTIVE)) != (SB_BORN|SB_ACTIVE)) > return 0; This is fine because it's an unlocked check, but I don't think it's actually necessary given the above. Indeed, if you are adding this, you need to expand the comment above on why SB_ACTIVE needs checking, and why the memory barrier doesn't actually apply to that part of the check.... -Dave. -- Dave Chinner david@fromorbit.com