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 4C7DBC54E58 for ; Thu, 21 Mar 2024 12:08:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 969E36B0083; Thu, 21 Mar 2024 08:08:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 91AA76B0085; Thu, 21 Mar 2024 08:08:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7B96B6B0087; Thu, 21 Mar 2024 08:08:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 69D686B0083 for ; Thu, 21 Mar 2024 08:08:25 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A94D2C1585 for ; Thu, 21 Mar 2024 12:08:24 +0000 (UTC) X-FDA: 81920923728.26.ACC8C81 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf09.hostedemail.com (Postfix) with ESMTP id EB86E140018 for ; Thu, 21 Mar 2024 12:08:22 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=CS1aaWaD; spf=pass (imf09.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711022903; 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=ZmCkXKKBMdn7q3ZGi77a1ZwDnFVVsMns2EKsafaBcTk=; b=zmgRRccyrvzcC6tJuYnL4Lnch5jeaVqiWB+GZx+omgxtx4ZYdgzidrEaw0kx07/+H3gOkz 1mtixzRmNOUvZUn0Q1tSPlJiPfKd2SlxEyVI7SXNOOgoGfEYMt/o7yCACnCKnwgohZCti7 O79slWTvsuwRqN8OHMJUM3dlKzhxLVk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711022903; a=rsa-sha256; cv=none; b=3AX6dioPEtqLx5Jb17YKfnzZub37x4GJEJiW/bL5ismIyQ9Flq8GvMvRDJVpx8RjYyNVmv 4jAjQbWeP1uCKk4oHrVZXff1DnmcDdFJMtAwRXxH4mQrZrw/q2qWZDKjfCgDS5B/FsIqV5 5MdYnWbJdIqvj5ANsRiY4EMxUCnZEdA= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=CS1aaWaD; spf=pass (imf09.hostedemail.com: domain of bfoster@redhat.com designates 170.10.129.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com; dmarc=pass (policy=none) header.from=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1711022902; h=from:from:reply-to:subject:subject: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=ZmCkXKKBMdn7q3ZGi77a1ZwDnFVVsMns2EKsafaBcTk=; b=CS1aaWaD6bQ2y31l3ZhM16VgjyiUPf48NAZVlEM0baAsEpGbf8CGiATpAYSXoxk2jOaPa9 tNpnrKv8QfpYwaabsYP2P7XRD8uPBNLY2sW312UfLv7YJt5ZoO/5PrTPdkC29UT8FzH+80 58cNBAzf4UNydjQiDpbqtiXs+dG8a50= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-467-XYKpxY26OWGyZ1_q7N7kEA-1; Thu, 21 Mar 2024 08:08:18 -0400 X-MC-Unique: XYKpxY26OWGyZ1_q7N7kEA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E7D2F3C0E44C; Thu, 21 Mar 2024 12:08:17 +0000 (UTC) Received: from bfoster (unknown [10.22.16.57]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4E71E492BCA; Thu, 21 Mar 2024 12:08:17 +0000 (UTC) Date: Thu, 21 Mar 2024 08:10:11 -0400 From: Brian Foster To: Kemeng Shi Cc: akpm@linux-foundation.org, tj@kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, willy@infradead.org, jack@suse.cz, dsterba@suse.com, mjguzik@gmail.com, dhowells@redhat.com, peterz@infradead.org Subject: Re: [PATCH 1/6] writeback: collect stats of all wb of bdi in bdi_debug_stats_show Message-ID: References: <20240320110222.6564-1-shikemeng@huaweicloud.com> <20240320110222.6564-2-shikemeng@huaweicloud.com> <3d08c249-1b12-f82b-2662-a6fa2b888011@huaweicloud.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3d08c249-1b12-f82b-2662-a6fa2b888011@huaweicloud.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.9 X-Stat-Signature: xyasoqfkm6xrnaw57rkim9y5eyb3q98f X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: EB86E140018 X-Rspam-User: X-HE-Tag: 1711022902-682469 X-HE-Meta: U2FsdGVkX1+b0P34/T3mfNvqgTK4F0kGivcRCXVrncA6PEYeV2VM3jruGxL1bU/rFmWGHefGpGbrhGJ0is6Ypv/c+uW77Bp/NGbhC6M9A3KRKeKqNlAtuRO+EQfVvkUHjlSIDX3CESp2WmrMIYzVyFY/TnFfztPQHAfw+dCNprNQn/1cNu3Up3B1ZGVU/7NIlFEj5kzCMfjXs+gz30VSTMNdZaBtYNbUNDU61OuFvfYhj5sERVZW2t2R8YKNR2HJ0y34kZbDwSJk9xCTYvJxaTiZbKiCQVmtuoA3hexbYRKp/j+AgyNzAaFdojttKMgmZkmpunVuM8nAdRXBd/Yjwn0Tdg+BAVgiU/5bo/VSdiFw9bz3hU+atYO8kigbUrb1Dy6vPAxrZef9twbM6sW2cFy05xHDTzFspXFQCa03fUN9mPRQPR8uCRy2duEUGrMEsclTwk6BXCPhexnXs6UA27XBR9GcEqndyiAEb1lkUTYxwZC4u4wCaMsXxlqB97p55+MIeMONT/jAzN3hhRIpFIRqGhL4OIGtEsIWCOpRvNtWjEfF7kvzeWpwU1BEBscdN3ZzzzVgHHGJP0aCpkaXfki2+81xXBXba2bKWvItSzZQxQ4lW7LTwPC/Zozp4OdYmhHoszHmCnaBK+voULfIA3Khm1M4jPBTK31MLU2T1GkIAwO3HSL9HZoYj9Y+HOsggkgf2iNoZ2gHxrs0wb44Q/vVC450sGL+2mX/6wDc7t2Q6nHhExL24yzp8NjLxR6yvW4IEmJKSMOxvolboX3oDagreMbqSY0FRk30eg/elx9yMQVFZddXdEJf39voCZvR/AS2ivN0PInY7jwnNVqaBTaV/MgO0Xv9v3kCbZsa32wQbxEN9e90zB34HaMwzE4A+z22LgRo2GrQAGWxAWmVNotajJyTg4/H 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 Thu, Mar 21, 2024 at 11:44:40AM +0800, Kemeng Shi wrote: > > > on 3/20/2024 9:21 PM, Brian Foster wrote: > > On Wed, Mar 20, 2024 at 07:02:17PM +0800, Kemeng Shi wrote: > >> /sys/kernel/debug/bdi/xxx/stats is supposed to show writeback information > >> of whole bdi, but only writeback information of bdi in root cgroup is > >> collected. So writeback information in non-root cgroup are missing now. > >> To be more specific, considering following case: > >> > >> /* create writeback cgroup */ > >> cd /sys/fs/cgroup > >> echo "+memory +io" > cgroup.subtree_control > >> mkdir group1 > >> cd group1 > >> echo $$ > cgroup.procs > >> /* do writeback in cgroup */ > >> fio -name test -filename=/dev/vdb ... > >> /* get writeback info of bdi */ > >> cat /sys/kernel/debug/bdi/xxx/stats > >> The cat result unexpectedly implies that there is no writeback on target > >> bdi. > >> > >> Fix this by collecting stats of all wb in bdi instead of only wb in > >> root cgroup. > >> > >> Signed-off-by: Kemeng Shi > >> --- > >> mm/backing-dev.c | 93 ++++++++++++++++++++++++++++++++++++------------ > >> 1 file changed, 70 insertions(+), 23 deletions(-) > >> > >> diff --git a/mm/backing-dev.c b/mm/backing-dev.c > >> index 5f2be8c8df11..788702b6c5dd 100644 > >> --- a/mm/backing-dev.c > >> +++ b/mm/backing-dev.c > > ... > >> @@ -46,31 +59,65 @@ static void bdi_debug_init(void) > >> bdi_debug_root = debugfs_create_dir("bdi", NULL); > >> } > >> > > ... > >> +#ifdef CONFIG_CGROUP_WRITEBACK > >> +static void bdi_collect_stats(struct backing_dev_info *bdi, > >> + struct wb_stats *stats) > >> +{ > >> + struct bdi_writeback *wb; > >> + > >> + /* protect wb from release */ > >> + mutex_lock(&bdi->cgwb_release_mutex); > >> + list_for_each_entry(wb, &bdi->wb_list, bdi_node) > >> + collect_wb_stats(stats, wb); > >> + mutex_unlock(&bdi->cgwb_release_mutex); > >> +} > >> +#else > >> +static void bdi_collect_stats(struct backing_dev_info *bdi, > >> + struct wb_stats *stats) > >> +{ > >> + collect_wb_stats(stats, &bdi->wb); > >> +} > >> +#endif > >> + > > > > I'm not familiar enough with the cgwb code to say for sure (and I'd > > probably wait for more high level feedback before worrying too much > > about this), but do we need the ifdef here just to iterate ->wb_list? > >>From looking at the code, it appears bdi->wb ends up on the list while > > the bdi is registered for both cases, so that distinction seems > > unnecessary. WRT to wb release protection, I wonder if this could use a > Currently, we have ifdef trying to remove unnecessary cost when > CONFIG_CGROUP_WRITEBACK is not enabled, see defination of cgwb_bdi_register > and cgwb_remove_from_bdi_list for example. So I try to define bdi_collect_stats > in similar way. > > combination of rcu_read_lock()/list_for_each_safe() and wb_tryget() on > > each wb before collecting its stats..? See how bdi_split_work_to_wbs() > > works, for example. > The combination of rcu_read_lock()/list_for_each_safe() and wb_tryget() > should work fine. > With ifdef, bdi_collect_stats takes no extra cost when CONFIG_CGROUP_WRITEBACK > is not enabled and is consistent with existing code style, so I still prefer > this way. Yes, The extra cost is not a big deal as it only exists in debug mode, > so it's acceptable to use the suggested combination in next version if you are > still strongly aganst this. > Ok. I also previously missed that there are two implementations of bdi_split_work_to_wbs() based on CGROUP_WRITEBACK. It seems reasonable enough to me to follow that precedent for the !CGROUP_WRITEBACK case. It still seems to make more sense to me to walk the list similar to how bdi_split_work_to_wbs() does for the CGROUP_WRITEBACK enabled case. Do you agree? Brian > > > > Also I see a patch conflict/compile error on patch 2 due to > > __wb_calc_thresh() only taking one parameter in my tree. What's the > > baseline commit for this series? > > > Sorry for missing this, this seris is based on another patchset [1] which is still > under review. > Look forward to your reply! > > Thansk > Kemeng > > [1] https://lore.kernel.org/lkml/20240123183332.876854-1-shikemeng@huaweicloud.com/T/#mc6455784a63d0f8aa1a2f5aff325abcdf9336b76 > > > Brian > > > >> +static int bdi_debug_stats_show(struct seq_file *m, void *v) > >> +{ > >> + struct backing_dev_info *bdi = m->private; > >> + unsigned long background_thresh; > >> + unsigned long dirty_thresh; > >> + struct wb_stats stats; > >> + unsigned long tot_bw; > >> + > >> global_dirty_limits(&background_thresh, &dirty_thresh); > >> - wb_thresh = wb_calc_thresh(wb, dirty_thresh); > >> + > >> + memset(&stats, 0, sizeof(stats)); > >> + stats.dirty_thresh = dirty_thresh; > >> + bdi_collect_stats(bdi, &stats); > >> + > >> + tot_bw = atomic_long_read(&bdi->tot_write_bandwidth); > >> > >> seq_printf(m, > >> "BdiWriteback: %10lu kB\n" > >> @@ -87,18 +134,18 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v) > >> "b_dirty_time: %10lu\n" > >> "bdi_list: %10u\n" > >> "state: %10lx\n", > >> - (unsigned long) K(wb_stat(wb, WB_WRITEBACK)), > >> - (unsigned long) K(wb_stat(wb, WB_RECLAIMABLE)), > >> - K(wb_thresh), > >> + K(stats.nr_writeback), > >> + K(stats.nr_reclaimable), > >> + K(stats.wb_thresh), > >> K(dirty_thresh), > >> K(background_thresh), > >> - (unsigned long) K(wb_stat(wb, WB_DIRTIED)), > >> - (unsigned long) K(wb_stat(wb, WB_WRITTEN)), > >> - (unsigned long) K(wb->write_bandwidth), > >> - nr_dirty, > >> - nr_io, > >> - nr_more_io, > >> - nr_dirty_time, > >> + K(stats.nr_dirtied), > >> + K(stats.nr_written), > >> + K(tot_bw), > >> + stats.nr_dirty, > >> + stats.nr_io, > >> + stats.nr_more_io, > >> + stats.nr_dirty_time, > >> !list_empty(&bdi->bdi_list), bdi->wb.state); > >> > >> return 0; > >> -- > >> 2.30.0 > >> > >> > > > > >