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 5A66ECD1284 for ; Sun, 7 Apr 2024 03:13:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A151B6B0082; Sat, 6 Apr 2024 23:13:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9C4AD6B0083; Sat, 6 Apr 2024 23:13:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 88CCF6B0085; Sat, 6 Apr 2024 23:13:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 6B0756B0082 for ; Sat, 6 Apr 2024 23:13:52 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id D8200C07AD for ; Sun, 7 Apr 2024 03:13:51 +0000 (UTC) X-FDA: 81981266262.26.FCE7B90 Received: from dggsgout11.his.huawei.com (unknown [45.249.212.51]) by imf27.hostedemail.com (Postfix) with ESMTP id 2C5ED40008 for ; Sun, 7 Apr 2024 03:13:47 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf27.hostedemail.com: domain of shikemeng@huaweicloud.com designates 45.249.212.51 as permitted sender) smtp.mailfrom=shikemeng@huaweicloud.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712459630; 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; bh=JNOAAdXcqa69QXt8pnHQv05QpznFEg3kuYNfz+UmDVU=; b=STWcOB+8x9+YgmnMKE9+tAkVwu95QKJoO0BJ7cluJ+nOkzl2DQECjuesiCocvf8yBxWw9B Tuh/EvTumEePMpFPdV3/lSz6FRxX3j5BOnIgli8N1eg6rO82evLTNBlZli75sYmy0zB4gz p0FNwsE+9n2j5oJYSc0dMPVETavoyWc= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf27.hostedemail.com: domain of shikemeng@huaweicloud.com designates 45.249.212.51 as permitted sender) smtp.mailfrom=shikemeng@huaweicloud.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712459630; a=rsa-sha256; cv=none; b=YiqYapbQx+7dwK6APENAHPV3FjsMSAi3XMg/WKaWX2XhHolce70mgE5gypH5rRURui5RAy Q+VchXODRRm/RC6YgIFKDoe+kBinP6C4Oo/ksPE8DYCUUlJQhMBKbES9MOduPt1xuG7EOi BHXLQVzSCYF+9cLcrHzx6g4IXKSNlFU= Received: from mail.maildlp.com (unknown [172.19.163.216]) by dggsgout11.his.huawei.com (SkyGuard) with ESMTP id 4VBy3f2PHbz4f3l1y for ; Sun, 7 Apr 2024 11:13:34 +0800 (CST) Received: from mail02.huawei.com (unknown [10.116.40.112]) by mail.maildlp.com (Postfix) with ESMTP id D36CB1A09E9 for ; Sun, 7 Apr 2024 11:13:42 +0800 (CST) Received: from [10.174.178.129] (unknown [10.174.178.129]) by APP1 (Coremail) with SMTP id cCh0CgAnRQ5lDxJmd0M4JQ--.28098S2; Sun, 07 Apr 2024 11:13:42 +0800 (CST) Subject: Re: [PATCH v2 3/6] writeback: support retrieving per group debug writeback stats of bdi To: Jan Kara , Brian Foster Cc: akpm@linux-foundation.org, willy@infradead.org, 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 References: <20240327155751.3536-1-shikemeng@huaweicloud.com> <20240327155751.3536-4-shikemeng@huaweicloud.com> <20240404090753.q3iugmqeeqig64db@quack3> From: Kemeng Shi Message-ID: <6bf2280d-bce1-c1c5-3b25-8cfc7e1fa81d@huaweicloud.com> Date: Sun, 7 Apr 2024 11:13:41 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.0 MIME-Version: 1.0 In-Reply-To: <20240404090753.q3iugmqeeqig64db@quack3> Content-Type: text/plain; charset=gbk Content-Transfer-Encoding: 7bit X-CM-TRANSID:cCh0CgAnRQ5lDxJmd0M4JQ--.28098S2 X-Coremail-Antispam: 1UD129KBjvJXoWxKw1xCF48ZF1rKw4Duw43trb_yoW7AryUp3 Wqg3W7Kr4DXw1IkwnFv34jv34IyrZ5JryUXr9rG345CF90qFn3ZF4rGFW5uFy5ZrW8Aw4U Zw4jyrZxW3y5tFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUvab4IE77IF4wAFF20E14v26r4j6ryUM7CY07I20VC2zVCF04k2 6cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rwA2F7IY1VAKz4 vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_tr0E3s1l84ACjcxK6xIIjxv20xvEc7Cj xVAFwI0_Gr1j6F4UJwA2z4x0Y4vEx4A2jsIE14v26rxl6s0DM28EF7xvwVC2z280aVCY1x 0267AKxVW0oVCq3wAS0I0E0xvYzxvE52x082IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG 6I80ewAv7VC0I7IYx2IY67AKxVWUJVWUGwAv7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFV Cjc4AY6r1j6r4UM4x0Y48IcVAKI48JM4IIrI8v6xkF7I0E8cxan2IY04v7Mxk0xIA0c2IE e2xFo4CEbIxvr21l42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxV Aqx4xG67AKxVWUJVWUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r1q 6r43MIIYrxkI7VAKI48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6x kF7I0E14v26r4j6F4UMIIF0xvE42xK8VAvwI8IcIk0rVWrJr0_WFyUJwCI42IY6I8E87Iv 67AKxVWUJVW8JwCI42IY6I8E87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73UjIFyT uYvjxUrR6zUUUUU X-CM-SenderInfo: 5vklyvpphqwq5kxd4v5lfo033gof0z/ X-Rspamd-Queue-Id: 2C5ED40008 X-Rspam-User: X-Rspamd-Server: rspam02 X-Stat-Signature: xy6abcgtfe8osi7oaggtduapwunf147j X-HE-Tag: 1712459627-378068 X-HE-Meta: U2FsdGVkX1/aFc2YEur8XC2WNrhbq163+DTx4R7PzMa4HmgwPvdTWt9dS/UMJ/3dM9TWU1Vza6xnHPbUkcehSx6/FkG2ZpIcoZ/wpBUsjzC0Cv5Hgjv7gk5MDNKc0zXoYO5M/mDJgb7gJrGdpI1P1m27d9bNzSQcxwKauQiLlBaGSdtyfttj8HsWRspR9xFFJLd/l1cYqwlShdE208WWJTDhcJwB+DCgjn0KWJWNTWa5RxtoRVGeeJ/Gre/mqcJViiJWbAhmMV4oFH3B4gtBndWAH2UVmD02DRP5U0TPiaw8KTNj3bvz7jB1khYrM5O1tRQwul3xZZbgGGReWPbxsxOAbuwTRl/BhiszAADl8scgHFmjWFp3SuVn5bc637nKmOmOpBzhQ5bMUzNQORmkbOYYyH5ZHKIKm6pKx6IW8gJFn6EgQRdeWPyQI6DeHUG9EhmT++ZkoJqIoLPSjvRdFnNRmRYsoDznJi5yn+kjEwjV6zcmGkzBzy0z3aKnrxMmKM6hYlXWR4bpgDN50Hizx/msKAx93j5iPNhXIajPtUBQ5U8OGZR8FtWAR7ZUU1FpkH/r/1tXScszzsKYRmJ1bidUrLcA+pSv1HIA4o7DLM6W9urImFOPFeRPknIc3htc9S9E76K8hX9WchMJk8gu/RlY9By2wgl8gRfft6nazAhhGwz19bhIXWSYFFYLsqwKa8IN6D7A3x1BR7hwaN0W0tX0NCetC5ybgqQCU3bzEWPjuqZXvZdVh/6+9NlRkIE7LJcdmMos+F08OfGvus0/uHTYEz1YAKpoI4P2UK7ZJaVx7K9BS0VmmEYWJwjE4us/ABgqnUQmCd8Ajo37jsb3Pm3lQRkz6sMPXw8ocVDe70iWeSlwgn2OAXkG+DOasepPcrv8zuMNUsfor5rNQ+yG7rhaMznV6wUQgT++zYxEboR6o8amerGDMoNe1NrB3Ef51AALi+o8bmNhPW6KAeM 7ARmSw+3 D/rIPospbg3Y7cOW57ZBRPEBT+fBFmjvckExQHSphgh+CxD4rwNNNsYMp3ZjjhGqM4WNFj9jzLBYcO9I0O6NZA3B1h6DHncsaffB65zOuG8an9XeQi06YSUDnSQR7L1ZIXdYAcK7H4kUrjPQaKKSwHPuy4PPa/r8a9Dg16z6X7GbE6YU= 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 4/4/2024 5:07 PM, Jan Kara wrote: > 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... It's acceptable to me if the extra cost is tolerable. > >>> 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. OK, I will simply keep wb_stats even CGWB is not enabled while keep state in both bdi_stats and wb_stats if Braian doesn't against in recent dasy. Kemeng > > Honza >