Re: [PATCH v4 2/4] writeback: support retrieving per group debug writeback stats of bdi
From: SeongJae Park
Date: Mon Apr 22 2024 - 20:44:59 EST
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?
--- 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?
> +
> + 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
[...]