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
