Re: [PATCH v2 2/2] btrfs: Expose the BTRFS commit stats through sysfs

[Boris Burkov <boris@bur.io>]

On Thu, Jun 16, 2022 at 06:24:51PM +0300, Nikolay Borisov wrote:
> 
> 
> On 16.06.22 г. 0:32 ч., Boris Burkov wrote:
> > On Wed, Jun 15, 2022 at 03:31:30PM +0200, David Sterba wrote:
> > > On Wed, Jun 15, 2022 at 03:47:51PM +0300, Nikolay Borisov wrote:
> > > > > +
> > > > > +	return sysfs_emit(buf,
> > > > > +		"commits %llu\n"
> > > > > +		"last_commit_dur %llu ms\n"
> > > > > +		"max_commit_dur %llu ms\n"
> > > > > +		"total_commit_dur %llu ms\n",
> > > > > +		fs_info->commit_stats.commit_counter,
> > > > > +		fs_info->commit_stats.last_commit_dur / NSEC_PER_MSEC,
> > > > > +		fs_info->commit_stats.max_commit_dur / NSEC_PER_MSEC,
> > > > > +		fs_info->commit_stats.total_commit_dur / NSEC_PER_MSEC);
> > > > > +}
> > > > > +
> > > > > +static ssize_t btrfs_commit_stats_store(struct kobject *kobj,
> > > > > +						struct kobj_attribute *a,
> > > > > +						const char *buf, size_t len)
> > > > > +{
> > > > 
> > > > Is there really value in being able to zero out the current stats?
> > > 
> > > I think it makes sense for the max commit time, one might want to track
> > > that for some workload and then reset. For the other it can go both
> > > ways, eg. a monitoring tool saves the stats completely and resets them.
> > > OTOH long term stats would be lost in case there's more than one
> > > application reading it.
> > 
> > As far as I can see, our options are roughly:
> > 1. separate files per stat, only max file has clear
> > 2. clear only max when clearing the joint file
> > 3. clear everything in the joint file (current patch)
> > 4. clear bitmap to control which fields to clear
> > 
> > 1 seems the clearest, but is sort of messy in terms of spamming lots of
> > files. There can be a "1b" variant which is one file with
> > count/total/last and a second file with max (rather than one each for all
> > four). 2 is a bit weird, just due to asymmetry. The "multiple separate
> > clearers" problem Dave came up with seems  serious for 3: it means if I
> > want to clear max and you want to clear total, we might make each other
> > lose data. 4 would work around that, but is an untuitive interface.
> > 
> > One other reason clearing total could be good is if we are counting in
> > nanoseconds, overflow becomes a non-trivial risk. For this reason, I
> > think I vote for 1 (separate files), but 2 (only clear max in a single
> > file) seems like a decent compromise. 4 feels overengineered, but is
> > kind of a souped-up version of 2.
> 
> 
> I don't know why but I like 2, even though when I think more about it it
> indeed introduces somewhat non-trivial asymmetry. But given that sysfs
> interfaces aren't considered ABI we can get it wrong the first time and we
> won't have to bother to support until the end of times.
> 
> A different POV would be that those stats would mostly be useful when doing
> measurements of a particular workload and in those cases you can reset the
> stats by remounting the fs, no ? From a monitoring POV I'd expect that the
> most interesting stats would be last_commit_dur as you can read it every x
> seconds, plot it and see how transaction latency varies over time. From such

My 2c: What's most useful for monitoring are total+count and max.

You have a process periodically read the total/count and get an average
over the collection interval, and it reads/clears the max to track the
max over the collection interval. From there it sends what it has
collected off to some DB to be stored/plotted/aggregated/whatever.

Our collection interval is typically 60s. I imagine if you collected
more frequently, your idea of tracking last duration would work a lot
better than in our setup.

With all that said, I think I agree that 2 is the best interface. I
think it also lets us get rid of the lock, since you no longer care
about racing setting the max with clearing it, since it is always
self-consistent.

> stats you can derive a max value, probably not THE max, but it should be
> within the ballpark. Total_commit and commit_counter - yeah, I dunno about
> those.