Received-SPF: pass (google.com: domain of linux-kernel+bounces-111080-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Subject: Re: [PATCH 1/6] writeback: collect stats of all wb of bdi in
 bdi_debug_stats_show
To: Jan Kara <jack@suse.cz>
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, bfoster@redhat.com, dsterba@suse.com,
 mjguzik@gmail.com, dhowells@redhat.com, peterz@infradead.org
References: <20240320110222.6564-1-shikemeng@huaweicloud.com>
 <20240320110222.6564-2-shikemeng@huaweicloud.com>
 <20240321180620.mbint45pbyc74vpg@quack3>
From: Kemeng Shi <shikemeng@huaweicloud.com>
Message-ID: <a684ccdb-372f-b9e6-7239-ddb42a3f5f28@huaweicloud.com>
Date: Fri, 22 Mar 2024 15:51:27 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <20240321180620.mbint45pbyc74vpg@quack3>
Content-Type: text/plain; charset=gbk
Content-Transfer-Encoding: 7bit


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 <shikemeng@huaweicloud.com>
> 
> 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 <linux/debugfs.h>
>>  #include <linux/seq_file.h>
>>  
>> +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!

> 
> 								Honza
>