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 9E75CCD1284 for ; Thu, 4 Apr 2024 09:08:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EAC026B0082; Thu, 4 Apr 2024 05:07:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E35A46B0083; Thu, 4 Apr 2024 05:07:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CAE936B0087; Thu, 4 Apr 2024 05:07:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id A8A206B0082 for ; Thu, 4 Apr 2024 05:07:59 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 3582C120364 for ; Thu, 4 Apr 2024 09:07:59 +0000 (UTC) X-FDA: 81971272278.22.A1CF3F6 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by imf05.hostedemail.com (Postfix) with ESMTP id 06E9610000D for ; Thu, 4 Apr 2024 09:07:55 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=hi94dDcK; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=VkRVOHT4; spf=pass (imf05.hostedemail.com: domain of jack@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712221676; a=rsa-sha256; cv=none; b=4xyWNkKYR7Hd3JvR4In3zTezQnEchHD42mFcSdBHtMAKV1yKYK5dCt5EVjoWivLdAeMtVg IKgfZEOuzrM+bpicypVKXm5BXAXLdrAx9M/aHjF5hPdZfDPjJ8KVg5Fhy3WrhxlEARDVa/ ULGqeVs7+lPu22rES9mEONLxcHnXFsI= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=hi94dDcK; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=VkRVOHT4; spf=pass (imf05.hostedemail.com: domain of jack@suse.cz designates 195.135.223.130 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712221676; 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=/fyMt9mi2QeseeA79i7XQ5O3GA4jt9iGQ8W/i5gg2EM=; b=iqOi8cyF0ZJVscscnWIICmXC6pddGmuMfwK9+3GgAanHH1XWSPTFHx555MZCdxDpm1XSnu 3bkdp1sH4p8/vvmYE/xZM+hxbqX17cwKh3g8Fr46MnvBaKMits4WDPX+4o7o20R7gAUucm WilQFYQY8hVv3gZZ8EvaDnutZ0/wVno= Received: from imap2.dmz-prg2.suse.org (imap2.dmz-prg2.suse.org [10.150.64.98]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id 2947E379D1; Thu, 4 Apr 2024 09:07:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1712221674; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=/fyMt9mi2QeseeA79i7XQ5O3GA4jt9iGQ8W/i5gg2EM=; b=hi94dDcK+h6tGAZlnkEpRkayNJyik5D1DB64qJBX/33XmvKoNiopZn0tTwPha4ZF+hu+uI rLRcFKKUihWQTBnUa9Uy9OC7PdmEdks6ukgg9Q/PrfYhhr3HZ2sMbx2PURYsSqeZj6oRYc P1F442CqNJKqalruSYD54kf73oHdt54= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1712221674; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=/fyMt9mi2QeseeA79i7XQ5O3GA4jt9iGQ8W/i5gg2EM=; b=VkRVOHT4tDv91fGgYX/LQ85y2rs2YsRBCXmUriVwKaLwCyl4q1vDBWWJg3Fx+JQwjJ4O3y ZFdhAhwO9xHLzGCg== Received: from imap2.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap2.dmz-prg2.suse.org (Postfix) with ESMTPS id 196E9139E8; Thu, 4 Apr 2024 09:07:54 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap2.dmz-prg2.suse.org with ESMTPSA id gxXxBeptDmZRHQAAn2gu4w (envelope-from ); Thu, 04 Apr 2024 09:07:54 +0000 Received: by quack3.suse.cz (Postfix, from userid 1000) id B688DA0816; Thu, 4 Apr 2024 11:07:53 +0200 (CEST) Date: Thu, 4 Apr 2024 11:07:53 +0200 From: Jan Kara To: Brian Foster Cc: Kemeng Shi , akpm@linux-foundation.org, willy@infradead.org, jack@suse.cz, tj@kernel.org, dsterba@suse.com, mjguzik@gmail.com, dhowells@redhat.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v2 3/6] writeback: support retrieving per group debug writeback stats of bdi Message-ID: <20240404090753.q3iugmqeeqig64db@quack3> References: <20240327155751.3536-1-shikemeng@huaweicloud.com> <20240327155751.3536-4-shikemeng@huaweicloud.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 06E9610000D X-Stat-Signature: az3hjrk48muc6pbr11gbrabmj8jxcw7c X-Rspam-User: X-HE-Tag: 1712221675-392690 X-HE-Meta: U2FsdGVkX18qex0nOOeb8c1USgilJDKLxbgQG7GozxgS/Ew1Iyj92sI8P+WwhHo1/3oi71KyaH4zu0bVSbbmJBV1eb9WqWK+ZVzdyfMHhVEaSjBtRZO4ye5li1zxcoLiQJs9lYffbB6ES3dsFMyXdGs6RP8/v8/YA+ExqeG3xwwA3w6bQzvWa7I1NRjOw9ikI6HtmkOKRLSkXMHoMfUPsCRIS0MzsY37xjTQFB/OYN1WucSZmWDttTJmodNJXVmvgotmEBuUYShuCsbqiqodS+3ExHvNt4+P0EyxNiPRPmfAzGH2nBaynoNm7Dz2cZ7CW/KQ1h0ozPVZr8e0yNXJEZBYDjrTdGAXmD7wxulHltyP8N4F6cqJFk0VAPmQmqINP5YFGTEy+d8GZiikOchuF3shMfyiPyl9HplSVchbmKpqs+230yOI4pRTrBerm1N0QLdgPhZ+JHUkgrgoX0cdKTT3TXs9iTnIKs8eIV3N7torvxJbbTNHBSdVZ3nAJIGlGi4D30J/rM/CPY+/2SxtLZRPQFczquxWZmwvALaFMtKe7Q9a7qs8V0hD6tTWEti86irfQVGBiOQYh0jq9AutSDgvwkOt0OX7elx8sqtQ8fwROPvXSqmpf7O3Tk1SUozRycGiGvrENQj1HztYbrFKoT/JAAHxLW8bICiA2Alno6o1SWfB1HzyPy/VLgmZMPP+EzPWLFIbDFJ+3+7tDPlr6pLA6V/AA4puclYX8AozSKMeNsm0wWXm/ctz4n9eY+lLaDqklhQkWCgzAboBAOY3Y7zaNhBGDTyfpapiH8AdxvASbWNvFhxmj2iMOd/oGt0UCC/wJr/bQ5PUIGCJwCQeenwLNYv4/WjWjLY4Rk8ADVKXhet0Y+hMwpfIe7y8hFZXhfJGA8yEs/FyF41Fh7Q9IIKswWBhevftfFSbGIcy2ARd20ZThW8U+6DyltU9h4BehepqQaZGDZXNTsYhLRS 8woBl5md AS8e3C+LPVdPLlcI5JihWs1V9cm00wyB3WthxHPhFikhXhkuYDrhe1tdkEIxuOgfBE0i3R6rbiMDTA++UR2/Vq/DWG9qIItfljfoG3ZR7K4xK2n1oAzrRt+wqved+1MzfQ5YtfSwMNHUUL7c3uZ5dboZeeXez7TeCG0wLsFzCyXU8375IJLbwJ+blL6O1x7lyiSNT0NPZAp7jfk2UNhQUNuBq1NBFKRIVGCZJ8S9rWrPlx2k8OyfMLk2W/yADHEfU18ijEIKZgXM6OPq4x9yOe0uXqA== 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: List-Subscribe: List-Unsubscribe: On Wed 03-04-24 11:04:58, Brian Foster wrote: > On Wed, Apr 03, 2024 at 04:49:42PM +0800, Kemeng Shi wrote: > > on 3/29/2024 9:10 PM, Brian Foster wrote: > > > On Wed, Mar 27, 2024 at 11:57:48PM +0800, Kemeng Shi wrote: > > >> + collect_wb_stats(&stats, wb); > > >> + > > > > > > Also, similar question as before on whether you'd want to check > > > WB_registered or something here.. > > Still prefer to keep full debug info and user could filter out on > > demand. > > Ok. I was more wondering if that was needed for correctness. If not, > then that seems fair enough to me. > > > >> + if (mem_cgroup_wb_domain(wb) == NULL) { > > >> + wb_stats_show(m, wb, &stats); > > >> + continue; > > >> + } > > > > > > Can you explain what this logic is about? Is the cgwb_calc_thresh() > > > thing not needed in this case? A comment might help for those less > > > familiar with the implementation details. > > If mem_cgroup_wb_domain(wb) is NULL, then it's bdi->wb, otherwise, > > it's wb in cgroup. For bdi->wb, there is no need to do wb_tryget > > and cgwb_calc_thresh. Will add some comment in next version. > > > > > > BTW, I'm also wondering if something like the following is correct > > > and/or roughly equivalent: > > > > > > list_for_each_*(wb, ...) { > > > struct wb_stats stats = ...; > > > > > > if (!wb_tryget(wb)) > > > continue; > > > > > > collect_wb_stats(&stats, wb); > > > > > > /* > > > * Extra wb_thresh magic. Drop rcu lock because ... . We > > > * can do so here because we have a ref. > > > */ > > > if (mem_cgroup_wb_domain(wb)) { > > > rcu_read_unlock(); > > > stats.wb_thresh = min(stats.wb_thresh, cgwb_calc_thresh(wb)); > > > rcu_read_lock(); > > > } > > > > > > wb_stats_show(m, wb, &stats) > > > wb_put(wb); > > > } > > It's correct as wb_tryget to bdi->wb has no harm. I have considered > > to do it in this way, I change my mind to do it in new way for > > two reason: > > 1. Put code handling wb in cgroup more tight which could be easier > > to maintain. > > 2. Rmove extra wb_tryget/wb_put for wb in bdi. > > Would this make sense to you? > > Ok, well assuming it is correct the above logic is a bit more simple and > readable to me. I think you'd just need to fill in the comment around > the wb_thresh thing rather than i.e. having to explain we don't need to > ref bdi->wb even though it doesn't seem to matter. > > I kind of feel the same on the wb_stats file thing below just because it > seems more consistent and available if wb_stats eventually grows more > wb-specific data. > > That said, this is subjective and not hugely important so I don't insist > on either point. Maybe wait a bit and see if Jan or Tejun or somebody > has any thoughts..? If nobody else expresses explicit preference then > I'm good with it either way. No strong opinion from me really. > > >> +static void cgwb_debug_register(struct backing_dev_info *bdi) > > >> +{ > > >> + debugfs_create_file("wb_stats", 0444, bdi->debug_dir, bdi, > > >> + &cgwb_debug_stats_fops); > > >> +} > > >> + > > >> static void bdi_collect_stats(struct backing_dev_info *bdi, > > >> struct wb_stats *stats) > > >> { > > >> @@ -117,6 +202,8 @@ static void bdi_collect_stats(struct backing_dev_info *bdi, > > >> { > > >> collect_wb_stats(stats, &bdi->wb); > > >> } > > >> + > > >> +static inline void cgwb_debug_register(struct backing_dev_info *bdi) { } > > > > > > Could we just create the wb_stats file regardless of whether cgwb is > > > enabled? Obviously theres only one wb in the !CGWB case and it's > > > somewhat duplicative with the bdi stats file, but that seems harmless if > > > the same code can be reused..? Maybe there's also a small argument for > > > dropping the state info from the bdi stats file and moving it to > > > wb_stats.In backing-dev.c, there are a lot "#ifdef CGWB .. #else .. #endif" to > > avoid unneed extra cost when CGWB is not enabled. > > I think it's better to avoid extra cost from wb_stats when CGWB is not > > enabled. For now, we only save cpu cost to create and destroy wb_stats > > and save memory cost to record debugfs file, we could save more in > > future when wb_stats records more debug info. Well, there's the other side that you don't have to think whether the kernel has CGWB enabled or not when asking a customer to gather the writeback debug info - you can always ask for wb_stats. Also if you move the wb->state to wb_stats only it will become inaccessible with CGWB disabled. So I agree with Brian that it is better to provide wb_stats also with CGWB disabled (and we can just implement wb_stats for !CGWB case with the same function as bdi_stats). That being said all production kernels I have seen do have CGWB enabled so I don't care that much about this... > > Move state info from bdi stats to wb_stats make senses to me. The only > > concern would be compatibility problem. I will add a new patch to this > > to make this more noticeable and easier to revert. Yeah, I don't think we care much about debugfs compatibility but I think removing state from bdi_stats is not worth the inconsistency between wb_stats and bdi_stats in the !CGWB case. Honza -- Jan Kara SUSE Labs, CR