Re: [PATCH v2 3/6] writeback: support retrieving per group debug writeback stats of bdi

[Jan Kara]

On Wed 03-04-24 11:04:58, Brian Foster wrote:
> On Wed, Apr 03, 2024 at 04:49:42PM +0800, Kemeng Shi wrote:
> > on 3/29/2024 9:10 PM, Brian Foster wrote:
> > > On Wed, Mar 27, 2024 at 11:57:48PM +0800, Kemeng Shi wrote:
> > >> +		collect_wb_stats(&stats, wb);
> > >> +
> > > 
> > > Also, similar question as before on whether you'd want to check
> > > WB_registered or something here..
> > Still prefer to keep full debug info and user could filter out on
> > demand.
> 
> Ok. I was more wondering if that was needed for correctness. If not,
> then that seems fair enough to me.
> 
> > >> +		if (mem_cgroup_wb_domain(wb) == NULL) {
> > >> +			wb_stats_show(m, wb, &stats);
> > >> +			continue;
> > >> +		}
> > > 
> > > Can you explain what this logic is about? Is the cgwb_calc_thresh()
> > > thing not needed in this case? A comment might help for those less
> > > familiar with the implementation details.
> > If mem_cgroup_wb_domain(wb) is NULL, then it's bdi->wb, otherwise,
> > it's wb in cgroup. For bdi->wb, there is no need to do wb_tryget
> > and cgwb_calc_thresh. Will add some comment in next version.
> > > 
> > > BTW, I'm also wondering if something like the following is correct
> > > and/or roughly equivalent:
> > > 	
> > > 	list_for_each_*(wb, ...) {
> > > 		struct wb_stats stats = ...;
> > > 
> > > 		if (!wb_tryget(wb))
> > > 			continue;
> > > 
> > > 		collect_wb_stats(&stats, wb);
> > > 
> > > 		/*
> > > 		 * Extra wb_thresh magic. Drop rcu lock because ... . 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);
> > > 	}
> > It's correct as wb_tryget to bdi->wb has no harm. I have considered
> > to do it in this way, I change my mind to do it in new way for
> > two reason:
> > 1. Put code handling wb in cgroup more tight which could be easier
> > to maintain.
> > 2. Rmove extra wb_tryget/wb_put for wb in bdi.
> > Would this make sense to you?
> 
> Ok, well assuming it is correct the above logic is a bit more simple and
> readable to me. I think you'd just need to fill in the comment around
> the wb_thresh thing rather than i.e. having to explain we don't need to
> ref bdi->wb even though it doesn't seem to matter.
> 
> I kind of feel the same on the wb_stats file thing below just because it
> seems more consistent and available if wb_stats eventually grows more
> wb-specific data.
> 
> That said, this is subjective and not hugely important so I don't insist
> on either point. Maybe wait a bit and see if Jan or Tejun or somebody
> has any thoughts..? If nobody else expresses explicit preference then
> I'm good with it either way.

No strong opinion from me really.

> > >> +static void cgwb_debug_register(struct backing_dev_info *bdi)
> > >> +{
> > >> +	debugfs_create_file("wb_stats", 0444, bdi->debug_dir, bdi,
> > >> +			    &cgwb_debug_stats_fops);
> > >> +}
> > >> +
> > >>  static void bdi_collect_stats(struct backing_dev_info *bdi,
> > >>  			      struct wb_stats *stats)
> > >>  {
> > >> @@ -117,6 +202,8 @@ static void bdi_collect_stats(struct backing_dev_info *bdi,
> > >>  {
> > >>  	collect_wb_stats(stats, &bdi->wb);
> > >>  }
> > >> +
> > >> +static inline void cgwb_debug_register(struct backing_dev_info *bdi) { }
> > > 
> > > Could we just create the wb_stats file regardless of whether cgwb is
> > > enabled? Obviously theres only one wb in the !CGWB case and it's
> > > somewhat duplicative with the bdi stats file, but that seems harmless if
> > > the same code can be reused..? Maybe there's also a small argument for
> > > dropping the state info from the bdi stats file and moving it to
> > > wb_stats.In backing-dev.c, there are a lot "#ifdef CGWB .. #else .. #endif" to
> > avoid unneed extra cost when CGWB is not enabled.
> > I think it's better to avoid extra cost from wb_stats when CGWB is not
> > enabled. For now, we only save cpu cost to create and destroy wb_stats
> > and save memory cost to record debugfs file, we could save more in
> > future when wb_stats records more debug info.

Well, there's the other side that you don't have to think whether the
kernel has CGWB enabled or not when asking a customer to gather the
writeback debug info - you can always ask for wb_stats. Also if you move
the wb->state to wb_stats only it will become inaccessible with CGWB
disabled. So I agree with Brian that it is better to provide wb_stats also
with CGWB disabled (and we can just implement wb_stats for !CGWB case with
the same function as bdi_stats).

That being said all production kernels I have seen do have CGWB enabled so
I don't care that much about this...

> > Move state info from bdi stats to wb_stats make senses to me. The only
> > concern would be compatibility problem. I will add a new patch to this
> > to make this more noticeable and easier to revert.

Yeah, I don't think we care much about debugfs compatibility but I think
removing state from bdi_stats is not worth the inconsistency between
wb_stats and bdi_stats in the !CGWB case.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR