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 49F58C54E71 for ; Fri, 22 Mar 2024 11:56:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A58276B007B; Fri, 22 Mar 2024 07:56:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A08006B0082; Fri, 22 Mar 2024 07:56:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8CF1D6B0083; Fri, 22 Mar 2024 07:56:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 7C1F56B007B for ; Fri, 22 Mar 2024 07:56:58 -0400 (EDT) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 1820DC16BA for ; Fri, 22 Mar 2024 11:56:58 +0000 (UTC) X-FDA: 81924523716.08.22572F6 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf30.hostedemail.com (Postfix) with ESMTP id 45B5380002 for ; Fri, 22 Mar 2024 11:56:56 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=K3YZdzl4; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf30.hostedemail.com: domain of bfoster@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711108616; 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=BRTRiL57H/xzh7fXeNRidNb3wxYqBup/qLAl32c8wE8=; b=cWsvUNcrA8EMQsMt9dPiAKNcgY1tQV2xKA2WGZEnAYjnLzYhdl9ox7OlJSrfwBrsaU77lQ eJd06AKPLUDNnbphQp+CEEHzj/JLM2xOs6Gs94ggvpxSDsN8uD4zJLjeJGdyQw8J6CAHm1 v7Iu8ChAauiTHPl2tS8mErIU6TNKqN0= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=K3YZdzl4; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf30.hostedemail.com: domain of bfoster@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=bfoster@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711108616; a=rsa-sha256; cv=none; b=FtRXIGZbbea5WN5oKDigOcrCAecYXM24I3gp2dTEhgpoL32jsO+cxez0ecqVCmScvLqU6Y 7Cjs4jmrZ8r/opm2KhT7FyhCX7GLeqYVTQkEcJ/b4+cBA3i5YTireq5Oj6QYCo/b8LaG8u ILxh3PM5fqoBNY1GWvl7RX+J0gONTEI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1711108615; 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=BRTRiL57H/xzh7fXeNRidNb3wxYqBup/qLAl32c8wE8=; b=K3YZdzl4PNd/dSEEeV84jjvzua3XWXuV80tRZ2Qi1aPSWR97udIhoeRLEkzm+JjiGwQFu4 0nLeuhjBN2XE5XerFerbyHsSU6JSKOncVF1RohruZ+zxPh5bFQRhutRaIK0kzqqQ9chDx5 +Wk4gR6weKZ+zpFC/IGozv7vEdtFa2g= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-168-iecBo9tsOPuuuefjyQjlKA-1; Fri, 22 Mar 2024 07:56:47 -0400 X-MC-Unique: iecBo9tsOPuuuefjyQjlKA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 37E8585CD5B; Fri, 22 Mar 2024 11:56:47 +0000 (UTC) Received: from bfoster (unknown [10.22.16.57]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A2E422022C24; Fri, 22 Mar 2024 11:56:46 +0000 (UTC) Date: Fri, 22 Mar 2024 07:58:40 -0400 From: Brian Foster To: Kemeng Shi Cc: Jan Kara , akpm@linux-foundation.org, tj@kernel.org, linux-mm@kvack.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, willy@infradead.org, 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> <20240321180620.mbint45pbyc74vpg@quack3> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Rspamd-Queue-Id: 45B5380002 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: bpc7o3gqqq5g8wdnakooqq4qrrpdzrwo X-HE-Tag: 1711108616-82857 X-HE-Meta: U2FsdGVkX19EWG5jV7uQPW4TZAm/2rfsJYsY3k1mseAIZWDuMFSFQKIMuz276jYSnWleLt+jqRZ/dsN/NoFzWd2I2Y/rYiTaFdWQx92qgaddUdTOlz0PsjMmu0RrDyJ6WWnCrUYo8I+zaEEBTU0N+UIxUsrSotNRnhgOyWqyZJrjKfdRNDygvhedjA+HExPB8c0lAbvHORhu2xfHZkWz3L5tCNKmx/43hlnZd8S8g62Vo+dyg/PkAkAkvlINIl86ansqIxaZP4l4Q2cGZXAugCvVZA6xb2QdVq1xo+UvauNjjwCK8J6LMtxK5kywCBtbI1UjLl9CMdAanw3w3bhxWPGhCLNdEjn2MNOI82k6tP4gbITm8/6nyJzkkBBsAve1nvLF1TTmMe9vGAPbJFL3iukLRpON7KcxyHZbVmmBAsoa3ydcFFPnd9pKc0eWtKwZ/WXtH53wycUIkObnFUy/b8AKzkJu1iaFbeS2eZUT3PpyBHHbSF6u7xpooejXq+S6M+ZCHrV/4XIsmKD5FnBysUG56/z6ZHkhvp97f2hRwIdHNUhCAnk3y3w0d3SES1D6k80TUIRp6HNvYoJgP73W9VUZu5ByAofLUB3YhJdSl3wuZpMkBqrwrohugID5DQGA4GvrNzgYxRADWrk3H/silhOzzpAIXBi5+F5X04N/TMgvzycBsxYlzWqAPxe9p6fMYQ3/XcIFlvRAXb9xMqfW9RPlRGRRTBDZO55dUX20yJInT1rjIHk+TjWupUzw7nX1kUgCTjYlJizj2VwAtP+IUOwwLNxJO16C4BgsRChmjOMjhsV0KEGHj4pLnWBaXZtNbSg5R+hK0zITzPpMTz11EueItspUY8rD8PNvduSoAvmkMmJo7KytNMv/S87YjQMbg4C6kYJuwZKX3wdwd5mwdj/e6ZI56S95mfeGxZZa5w0IZMePF23rVLeBZt1L0XTtx3nqeXDXy/KwLcPcH1J NCRd3thV 6K9K/UmHXlrQNRcEPPIqq/RXSTWUnNrPaS5UsCftWG9GmGeTfRdlYnGefl9GCAvYP00utaBVCA2cx3bkB7MtkU8OXYBb0PbysoDu5HEL+5+SeplOwr8BNhKYr8o5obaDdWSnAsFKFvI7c5+ah9u73saXwanEKbpFojsoioC/qpdBgV3ijuozEvcHNa7m9O10z+jbhqwiq/p0zKmaU04+m92euNPITOs8+dcJox61PVBR2FsUp6ZfcFyQzPKUlz97lorvt3O2jb/BfTM0= 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 Fri, Mar 22, 2024 at 03:51:27PM +0800, Kemeng Shi wrote: > > > on 3/22/2024 2:06 AM, Jan Kara wrote: > > On Wed 20-03-24 19:02:17, 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 > > > > Looks mostly good, one comment below: > > > >> --- > >> 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 > >> @@ -39,6 +39,19 @@ struct workqueue_struct *bdi_wq; > >> #include > >> #include > >> > >> +struct wb_stats { > >> + unsigned long nr_dirty; > >> + unsigned long nr_io; > >> + unsigned long nr_more_io; > >> + unsigned long nr_dirty_time; > >> + unsigned long nr_writeback; > >> + unsigned long nr_reclaimable; > >> + unsigned long nr_dirtied; > >> + unsigned long nr_written; > >> + unsigned long dirty_thresh; > >> + unsigned long wb_thresh; > >> +}; > >> + > >> static struct dentry *bdi_debug_root; > >> > >> static void bdi_debug_init(void) > >> @@ -46,31 +59,65 @@ static void bdi_debug_init(void) > >> bdi_debug_root = debugfs_create_dir("bdi", NULL); > >> } > >> > >> -static int bdi_debug_stats_show(struct seq_file *m, void *v) > >> +static void collect_wb_stats(struct wb_stats *stats, > >> + struct bdi_writeback *wb) > >> { > >> - struct backing_dev_info *bdi = m->private; > >> - struct bdi_writeback *wb = &bdi->wb; > >> - unsigned long background_thresh; > >> - unsigned long dirty_thresh; > >> - unsigned long wb_thresh; > >> - unsigned long nr_dirty, nr_io, nr_more_io, nr_dirty_time; > >> struct inode *inode; > >> > >> - nr_dirty = nr_io = nr_more_io = nr_dirty_time = 0; > >> spin_lock(&wb->list_lock); > >> list_for_each_entry(inode, &wb->b_dirty, i_io_list) > >> - nr_dirty++; > >> + stats->nr_dirty++; > >> list_for_each_entry(inode, &wb->b_io, i_io_list) > >> - nr_io++; > >> + stats->nr_io++; > >> list_for_each_entry(inode, &wb->b_more_io, i_io_list) > >> - nr_more_io++; > >> + stats->nr_more_io++; > >> list_for_each_entry(inode, &wb->b_dirty_time, i_io_list) > >> if (inode->i_state & I_DIRTY_TIME) > >> - nr_dirty_time++; > >> + stats->nr_dirty_time++; > >> spin_unlock(&wb->list_lock); > >> > >> + stats->nr_writeback += wb_stat(wb, WB_WRITEBACK); > >> + stats->nr_reclaimable += wb_stat(wb, WB_RECLAIMABLE); > >> + stats->nr_dirtied += wb_stat(wb, WB_DIRTIED); > >> + stats->nr_written += wb_stat(wb, WB_WRITTEN); > >> + stats->wb_thresh += wb_calc_thresh(wb, stats->dirty_thresh); > >> +} > >> + > >> +#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); > >> +} > > > > So AFAICT this function can race against > > bdi_unregister() -> wb_shutdown(&bdi->wb) > > > > because that doesn't take the cgwb_release_mutex. So we either need the RCU > > protection as Brian suggested or cgwb_lock or something. But given > > collect_wb_stats() can take a significant amount of time (traversing all > > the lists etc.) I think we'll need something more clever. > Sorry for missing this. I only pay attention to wb in cgroup as there is no > much change to how we use bdi->wb. > It seems that there was always a race between orginal bdi_debug_stats_show and > release of bdi as following > cat /.../stats > bdi_debug_stats_show > bdi_unregister > bdi_put > release_bdi > kfree(bdi) > use after free > > I will fix this in next version. Thanks! > BTW, I thought this was kind of the point of the tryget in the writeback path. I.e., the higher level loop runs under rcu_read_lock(), but in the case it needs to cycle the rcu lock it acquires a reference to the wb, and then can use that wb to continue the loop once the rcu lock is reacquired. IIUC, this works because the rcu list removal keeps the list ->next pointer sane and then the ref keeps the wb memory from being freed. A tryget of any wb's that have been shutdown fails because the percpu ref counter has been killed. So I _think_ this means that for the stat collection use case, you could protect the overall walk with rcu as Jan alludes above, but then maybe use a combination of need_resched()/cond_resched_rcu() and wb_tryget() to introduce resched points and avoid holding lock(s) for too long. Personally, I wonder if since this is mainly debug code we could just get away with doing the simple thing of trying for a ref on each wb unconditionally rather than only in a need_resched() case, but maybe there are reasons not to do that... hm? Brian > > > > Honza > > >