Re: [PATCH 1/6] writeback: collect stats of all wb of bdi in bdi_debug_stats_show

[Kemeng Shi]

on 3/22/2024 7:58 PM, Brian Foster wrote:
> 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 <shikemeng@xxxxxxxxxxxxxxx>
>>>
>>> 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!
>>
> 
Hi Brian
> 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 killedFor bdi->wb, tryget seems not helpful to protect race as wb_tryget does
nothing for bdi->wb. For wb in cgroup, this works fine.
> 
> 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.
Sure, I will protect race with rcu in next version!
> 
> 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?
Agreed, I also prefer simple debug code with no need_resched. Will do
it unless someone is against this.

Thansk a lot for the helpful information!
Kemeng
> 
> Brian
> 
>>>
>>> 								Honza
>>>
>>
>