From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from va-2-48.ptr.blmpb.com (va-2-48.ptr.blmpb.com [209.127.231.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7839B3264F5 for ; Sat, 12 Sep 2026 14:21:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.127.231.48 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789222907; cv=none; b=ZO6PT3/ucBipFErskWom1TDT8o4878m5cTePkA2vzIBjYeyJGsmAokPG7/+1HTLbWfbXYDpuz420Skdf2QlxnU0SE4DPh/eg2LHxTNNgoku0RWUvHfdOM12YNQVGM1MDQu7xRGYV1Qc13zupvllMxixFrPslRrqNmjDtR+xTHTw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789222907; c=relaxed/simple; bh=0wX6hOxSyzv13NfzbyN+AEPTOtfdE4C+hpqJPodyllQ=; h=From:References:Date:Subject:Message-Id:Mime-Version:In-Reply-To: To:Cc:Content-Type; b=mRulw5jL4jBJSxhzMoftTHGe8+BB3leq5W5zc/RN06Sa5G+lkfYVGhF/vkT/IN+qc49owqsrbQBeyaCySKQcQU9KGt6cXBvjTCFzXfsRUErT1VKQOQoibj/q3yidXS9G9n3Pat+WjTrR7fDhvf86CD3DwZKPY+q5b8RkZEQT5Y0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fygo.io; spf=pass smtp.mailfrom=fygo.io; dkim=pass (2048-bit key) header.d=fygo-io.20200929.dkim.larksuite.com header.i=@fygo-io.20200929.dkim.larksuite.com header.b=UGldNLs5; arc=none smtp.client-ip=209.127.231.48 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fygo.io Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fygo.io Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=fygo-io.20200929.dkim.larksuite.com header.i=@fygo-io.20200929.dkim.larksuite.com header.b="UGldNLs5" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; s=s1; d=fygo-io.20200929.dkim.larksuite.com; t=1789222897; h=from:subject:mime-version:from:date:message-id:subject:to:cc: reply-to:content-type:mime-version:in-reply-to:message-id; bh=m4/pESkF5aOOEVFDthMDfJ73cy1uGXqcU5VDQUIbEYU=; b=UGldNLs5R+8hoPWBCmTD+McM5Lu8GHuFFxsmAmDuGrVLgTG7E9C0LBRQSirs7xt/2EJDwa MpkQdDwP6+tg/xRfQJz06LqsuuwEGE1G8V1M/vDR/gVJeGWeWyVYUi8CbcSAiYrs8dwL1S H5WrG0RhB1nnSdMfCNZEjrEgqOrXZ4rWyP4w8cUygCNxY3Sa97c10ob+L8OOlkylzKKnxQ /B0Ky1v9kHB/rknQECUEC8/bx/mXrCnIkoe8x/3T/U9gowiymL3x1DGXVF9dQGIqNWLTBq bMThFbPAPYcXsLS9ztGL8N/N5LLnQZsBtnSl3BTxJry0yWA7JeGYzv2Md+2UrQ== From: "yu kuai" X-Lms-Return-Path: X-Original-From: yu kuai References: <20260823133045.970199-1-yukuai@kernel.org> <20260823133045.970199-3-yukuai@kernel.org> <49d724b8-57c6-48fc-8099-69cd1b072351@linux.ibm.com> Date: Sat, 12 Sep 2026 22:21:28 +0800 Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH 2/3] blk-cgroup: store blkcg in bio instead of blkg Message-Id: <4701cbcb-b877-4902-a5b6-59836c345ae1@fygo.io> Precedence: bulk X-Mailing-List: linux-raid@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 User-Agent: Mozilla Thunderbird Received: from [192.168.1.104] ([39.182.0.161]) by smtp.larksuite.com with ESMTPS; Sat, 12 Sep 2026 14:21:35 +0000 In-Reply-To: Reply-To: yukuai@fygo.io To: "Nilay Shroff" , "Jens Axboe" , "Tejun Heo" , "Josef Bacik" , "Johannes Weiner" , =?utf-8?q?Michal_Koutn=C3=BD?= , "yu kuai" Cc: "Christoph Hellwig" , "Tao Cui" , "Jan Kara" , "Jonathan Corbet" , "Shuah Khan" , "Coly Li" , "Kent Overstreet" , "Alasdair Kergon" , "Mike Snitzer" , "Mikulas Patocka" , "Benjamin Marzinski" , "Song Liu" , "Li Nan" , "Xiao Ni" , "Pankaj Gupta" , "Dan Williams" , "Vishal Verma" , "Dave Jiang" , "Alison Schofield" , "Ira Weiny" , "Andreas Gruenbacher" , "Matthew Wilcox" , "Andrew Morton" , "Chris Li" , "Kairui Song" , "Kemeng Shi" , "Nhat Pham" , "Baoquan He" , "Barry Song" , "Youngjun Park" , , , , , , , , , , , , Content-Type: text/plain; charset=UTF-8 Hi, =E5=9C=A8 2026/9/7 14:02, Nilay Shroff =E5=86=99=E9=81=93: > On 9/6/26 5:09 PM, yu kuai wrote: >> Hi, >> >> =E5=9C=A8 2026/9/6 18:56, Nilay Shroff =E5=86=99=E9=81=93: >>> On 8/23/26 7:00 PM, Yu Kuai wrote: >>>> @@ -257,20 +262,21 @@ static inline bool >>>> bio_issue_as_root_blkg(struct bio *bio) >>>> =C2=A0=C2=A0=C2=A0 * @blkcg: blkcg of interest >>>> =C2=A0=C2=A0=C2=A0 * @q: request_queue of interest >>>> =C2=A0=C2=A0=C2=A0 * >>>> =C2=A0=C2=A0=C2=A0 * Lookup a blkg for the @blkcg - @q pair, whether i= t is online or >>>> dying. >>>> =C2=A0=C2=A0=C2=A0 * >>>> - * Must be called in a RCU critical section. >>>> + * Must be called with either RCU read lock or queue_lock held. >>>> =C2=A0=C2=A0=C2=A0 * >>>> =C2=A0=C2=A0=C2=A0 * This does not acquire a reference.=C2=A0 The call= er must already hold >>>> one, or >>>> =C2=A0=C2=A0=C2=A0 * have the blkg pinned by I/O. >>>> =C2=A0=C2=A0=C2=A0 */ >>>> =C2=A0=C2=A0 static inline struct blkcg_gq *blkg_lookup_any(struct blk= cg *blkcg, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct request_queue *q) >>>> =C2=A0=C2=A0 { >>>> -=C2=A0=C2=A0=C2=A0 RCU_LOCKDEP_WARN(!rcu_read_lock_held(), >>>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 "blkg_lookup_any() requires an RCU read lock"); >>>> +=C2=A0=C2=A0=C2=A0 RCU_LOCKDEP_WARN(!rcu_read_lock_held() && >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 !lockdep_is_held(&q->queue_lock), >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 "blkg_lookup_any() requires an RCU read lock or >>>> queue_lock"); >>>> =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return rhashtable_lookup(&= q->blkg_hash, &blkcg->css.id, >>>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 blkg_hash_params); >>>> =C2=A0=C2=A0 } >>> >>> The changes look good to me. However, rhashtable_lookup() is annotated >>> with __must_hold_shared(RCU). Since we now officially support Clang >>> context annotations in the block layer, compiling with the latest >>> Clang/LLVM generates a number of context warnings, for example: >>> >>> block/blk-cgroup.h:277:9: warning: calling function 'rhashtable_lookup' >>> requires holding __ctx_lock_RCU 'RCU' [-Wthread-safety-analysis] >>> =C2=A0=C2=A0 277 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 retu= rn rhashtable_lookup(&q->blkg_hash,=20 >>> &blkcg->css.id, >>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ^ >>> >>> The same warning is reported from several other block layer source >>> files that include blk-cgroup.h. >>> >>> This indicates that rhashtable_lookup() requires the RCU read lock >>> from Clang's context-analysis perspective. Looking at the call paths of >>> blkg_lookup_any(), it appears that not all callers hold >>> rcu_read_lock() when invoking it. >> >> I was not aware of the above clang warning, this should not be a real=20 >> problem >> because queue_lock should have the same protection as rcu, and I was=20 >> trying >> not to make this patch too complex. >> > I agree that this is not a runtime correctness issue when the lookup=20 > is already > protected by q->queue_lock. > >> I add the rcu_read_lock() in the following patch to covert queue_lock=20 >> to blkcg_mutex: >> >> [RFC PATCH v3 3/6] blk-cgroup: protect blkgs with blkcg_mutex - Yu=20 >> Kuai=20 >> >> >> Since this is related to compile warning, I'll move the rcu=20 >> protection to this >> patch. >> > Since rhashtable_lookup() is annotated with __must_hold_shared(RCU),=20 > Clang's > context analysis still reports warnings for these call paths. While this > doesn't cause a build failure (and only generates warning) when=20 > CONFIG_WERROR > is not enabled, it could do so for configurations that enable=20 > CONFIG_WERROR=3Dy. > > So IMO, moving the rcu_read_lock() protection into this patch makes=20 > sense. > This would avoid introducing the Clang warnings/errors in this patch,=20 > and then > you may accordingly adjust your subsequent series where the=20 > q->queue_lock protection > is removed and the blkg lookup paths are protected by RCU. > > I also noticed that some paths already take q->queue_lock followed by=20 > rcu_read_lock(), > so adding the RCU protection here should be consistent with those=20 > existing paths. Turns out Clang will still warn after adding rcu_read_lock(). I'm not famil= iar how this works, but AI told me this is related to inline helpers, And I'll have to a= dd the same __must_hold_shared(RCU) in rhashtable_lookup() for following inline callers= to kill the warning: static inline struct blkcg_gq *blkg_lookup_any(...) __must_hold_shared(RCU); static inline struct blkcg_gq *blkg_lookup(...) __must_hold_shared(RCU); > > Thanks, > --Nilay --=20 Thanks, Kuai