Re: [PATCH v4 2/4] writeback: support retrieving per group debug writeback stats of bdi
From: Kemeng Shi
Date: Mon Apr 22 2024 - 21:29:23 EST
Hello
on 4/23/2024 8:44 AM, SeongJae Park wrote:
> Hi Kemeng,
>
> On Tue, 23 Apr 2024 00:48:06 +0800 Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx> wrote:
>
>> Add /sys/kernel/debug/bdi/xxx/wb_stats to show per group writeback stats
>> of bdi.
>>
>> Following domain hierarchy is tested:
>> global domain (320G)
>> / \
>> cgroup domain1(10G) cgroup domain2(10G)
>> | |
>> bdi wb1 wb2
>>
>> /* per wb writeback info of bdi is collected */
>> cat /sys/kernel/debug/bdi/252:16/wb_stats
>> WbCgIno: 1
>> WbWriteback: 0 kB
>> WbReclaimable: 0 kB
>> WbDirtyThresh: 0 kB
>> WbDirtied: 0 kB
>> WbWritten: 0 kB
>> WbWriteBandwidth: 102400 kBps
>> b_dirty: 0
>> b_io: 0
>> b_more_io: 0
>> b_dirty_time: 0
>> state: 1
>> WbCgIno: 4094
>> WbWriteback: 54432 kB
>> WbReclaimable: 766080 kB
>> WbDirtyThresh: 3094760 kB
>> WbDirtied: 1656480 kB
>> WbWritten: 837088 kB
>> WbWriteBandwidth: 132772 kBps
>> b_dirty: 1
>> b_io: 1
>> b_more_io: 0
>> b_dirty_time: 0
>> state: 7
>> WbCgIno: 4135
>> WbWriteback: 15232 kB
>> WbReclaimable: 786688 kB
>> WbDirtyThresh: 2909984 kB
>> WbDirtied: 1482656 kB
>> WbWritten: 681408 kB
>> WbWriteBandwidth: 124848 kBps
>> b_dirty: 0
>> b_io: 1
>> b_more_io: 0
>> b_dirty_time: 0
>> state: 7
>>
>> Signed-off-by: Kemeng Shi <shikemeng@xxxxxxxxxxxxxxx>
>> ---
>> include/linux/writeback.h | 1 +
>> mm/backing-dev.c | 78 ++++++++++++++++++++++++++++++++++++++-
>> mm/page-writeback.c | 19 ++++++++++
>> 3 files changed, 96 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
>> index 9845cb62e40b..112d806ddbe4 100644
>> --- a/include/linux/writeback.h
>> +++ b/include/linux/writeback.h
>> @@ -355,6 +355,7 @@ int dirtytime_interval_handler(struct ctl_table *table, int write,
>>
>> void global_dirty_limits(unsigned long *pbackground, unsigned long *pdirty);
>> unsigned long wb_calc_thresh(struct bdi_writeback *wb, unsigned long thresh);
>> +unsigned long cgwb_calc_thresh(struct bdi_writeback *wb);
>>
>> void wb_update_bandwidth(struct bdi_writeback *wb);
>>
>> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
>> index 089146feb830..6ecd11bdce6e 100644
>> --- a/mm/backing-dev.c
>> +++ b/mm/backing-dev.c
>> @@ -155,19 +155,93 @@ static int bdi_debug_stats_show(struct seq_file *m, void *v)
>> }
>> DEFINE_SHOW_ATTRIBUTE(bdi_debug_stats);
>>
>> +static void wb_stats_show(struct seq_file *m, struct bdi_writeback *wb,
>> + struct wb_stats *stats)
>> +{
>> +
>> + seq_printf(m,
>> + "WbCgIno: %10lu\n"
>> + "WbWriteback: %10lu kB\n"
>> + "WbReclaimable: %10lu kB\n"
>> + "WbDirtyThresh: %10lu kB\n"
>> + "WbDirtied: %10lu kB\n"
>> + "WbWritten: %10lu kB\n"
>> + "WbWriteBandwidth: %10lu kBps\n"
>> + "b_dirty: %10lu\n"
>> + "b_io: %10lu\n"
>> + "b_more_io: %10lu\n"
>> + "b_dirty_time: %10lu\n"
>> + "state: %10lx\n\n",
>> + cgroup_ino(wb->memcg_css->cgroup),
>
> I'm getting below kunit build failure from the latest mm-unstable tree, and
> 'git bisect' points this patch.
>
> ERROR:root:.../linux/mm/backing-dev.c: In function ‘wb_stats_show’:
> .../linux/mm/backing-dev.c:175:20: error: implicit declaration of function ‘cgroup_ino’; did you mean ‘cgroup_init’? [-Werror=implicit-function-declaration]
> 175 | cgroup_ino(wb->memcg_css->cgroup),
> | ^~~~~~~~~~
> | cgroup_init
> .../linux/mm/backing-dev.c:175:33: error: ‘struct bdi_writeback’ has no member named ‘memcg_css’
> 175 | cgroup_ino(wb->memcg_css->cgroup),
> | ^~
>
> The kunit build config is not having CONFIG_CGROUPS. I guess we need to check
> the case? I confirmed below dumb change is fixing the issue, but I guess it
> could be cleaner. May I ask your opinion?
Thanks for report the issue. As discussed before, we try to keep the same output
whether CGROUP is enable or not. So we'd better show cgroup number 0 for bdi->wb
instead remove cgroup number from output.
>
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -160,7 +160,9 @@ static void wb_stats_show(struct seq_file *m, struct bdi_writeback *wb,
> {
>
> seq_printf(m,
> +#ifdef CONFIG_CGROUPS
> "WbCgIno: %10lu\n"
> +#endif
> "WbWriteback: %10lu kB\n"
> "WbReclaimable: %10lu kB\n"
> "WbDirtyThresh: %10lu kB\n"
> @@ -172,7 +174,9 @@ static void wb_stats_show(struct seq_file *m, struct bdi_writeback *wb,
> "b_more_io: %10lu\n"
> "b_dirty_time: %10lu\n"
> "state: %10lx\n\n",
> +#ifdef CONFIG_CGROUPS
> cgroup_ino(wb->memcg_css->cgroup),
> +#endif
> K(stats->nr_writeback),
> K(stats->nr_reclaimable),
> K(stats->wb_thresh),
>
>
>> + K(stats->nr_writeback),
>> + K(stats->nr_reclaimable),
>> + K(stats->wb_thresh),
>> + K(stats->nr_dirtied),
>> + K(stats->nr_written),
>> + K(wb->avg_write_bandwidth),
>> + stats->nr_dirty,
>> + stats->nr_io,
>> + stats->nr_more_io,
>> + stats->nr_dirty_time,
>> + wb->state);
>> +}
>> +
>> +static int cgwb_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 bdi_writeback *wb;
>> + struct wb_stats stats;
>
> Kunit build also shows below warning:
>
> .../linux/mm/backing-dev.c: In function ‘cgwb_debug_stats_show’:
> .../linux/mm/backing-dev.c:195:25: warning: unused variable ‘stats’ [-Wunused-variable]
> 195 | struct wb_stats stats;
> | ^~~~~
>
> I guess above line can simply removed?
Yes, this could be simply removed.
I'd submit a new series to fix this very soon.
Thanks.
>
>> +
>> + global_dirty_limits(&background_thresh, &dirty_thresh);
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(wb, &bdi->wb_list, bdi_node) {
>> + struct wb_stats stats = { .dirty_thresh = dirty_thresh };
>> +
>> + if (!wb_tryget(wb))
>> + continue;
>> +
>> + collect_wb_stats(&stats, wb);
>> +
>> + /*
>> + * Calculate thresh of wb in writeback cgroup which is min of
>> + * thresh in global domain and thresh in cgroup domain. Drop
>> + * rcu lock because cgwb_calc_thresh may sleep in
>> + * cgroup_rstat_flush. We can do so here because we have a ref.
>> + */
>> + if (mem_cgroup_wb_domain(wb)) {
>> + rcu_read_unlock();
>> + stats.wb_thresh = min(stats.wb_thresh, cgwb_calc_thresh(wb));
>> + rcu_read_lock();
>> + }
>> +
>> + wb_stats_show(m, wb, &stats);
>> +
>> + wb_put(wb);
>> + }
>> + rcu_read_unlock();
>> +
>> + return 0;
>> +}
>
>
> Thanks,
> SJ
>
> [...]
>