Received-SPF: pass (google.com: domain of linux-kernel+bounces-154365-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: SeongJae Park <sj@kernel.org>
To: Kemeng Shi <shikemeng@huaweicloud.com>
Cc: SeongJae Park <sj@kernel.org>,
	akpm@linux-foundation.org,
	willy@infradead.org,
	jack@suse.cz,
	bfoster@redhat.com,
	tj@kernel.org,
	dsterba@suse.com,
	mjguzik@gmail.com,
	dhowells@redhat.com,
	linux-kernel@vger.kernel.org,
	linux-mm@kvack.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v4 2/4] writeback: support retrieving per group debug writeback stats of bdi
Date: Mon, 22 Apr 2024 17:44:30 -0700
Message-Id: <20240423004430.140320-1-sj@kernel.org>
In-Reply-To: <20240422164808.13627-3-shikemeng@huaweicloud.com>
References: 
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Kemeng,

On Tue, 23 Apr 2024 00:48:06 +0800 Kemeng Shi <shikemeng@huaweicloud.com> 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@huaweicloud.com>
> ---
>  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

[...]