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

[Kemeng Shi]

on 3/21/2024 8:10 PM, Brian Foster wrote:
> 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 <shikemeng@xxxxxxxxxxxxxxx>
>>>> ---
>>>>  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?
Sure, I agree with this and will do it in next version. Thansk!

Kemeng
> 
> 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@xxxxxxxxxxxxxxx/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
>>>>
>>>>
>>>
>>>
>>
> 
>