'Re: [PATCH 0/7 v8] xfs: stats in sysfs'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-xfs
Subject:    Re: [PATCH 0/7 v8] xfs: stats in sysfs
From:       "Bill O'Donnell" <billodo () redhat ! com>
Date:       2015-09-28 15:06:48
Message-ID: 20150928150647.GA17514 () redhat ! com
[Download RAW message or body]

On Mon, Sep 28, 2015 at 10:00:44AM -0500, Eric Sandeen wrote:
> On 9/25/15 6:22 PM, Bill O'Donnell wrote:
> > 
> > Hello-
> > 
> > Following is the next iteration of the series to add xfs stats to
> > sysfs. This iteration adds patches 6 and 7 to complete the incorporation
> > of sysfs/kobject into xfsstats, including working global and per-fs stats.
> 
> Hi Bill - as I mentioned on IRC, it looks like a lot of the patches in this
> series fix up review comments for *prior* patches; i.e. Patch X implements
> a change X, and got review comments when originally submitted.
> 
> Then Patch Y introduces change Y, but also fixes up comments in Patch X.
> 
> When you get review comments, they really need to be fixed up in that
> patch, not later in the patch series.  This is for two reasons; for one,
> we really want each committed patch to be correct stylistically and 
> functionally.  And two, it simplifies review of the series; one doesn't
> need to look forward in the series to find fixes for prior patches, and
> each patch contains only its own functional changes, not unrelated fixups
> for prior patches.
> 
> So for example, if you have a patch which is fixing whitespace on code
> introduced in a previous patch, it's in the wrong patch.
> 
> Or for a more concrete example, in patch 5 you do:
> 
>  for_each_possible_cpu(cpu)
> -		val += *(((__u32 *)&per_cpu(xfsstats, cpu) + idx));
> +		val += *(((__u32 *)&per_cpu(*stats, cpu) + idx));
>  return val;
> 
> and then in patch 6 you do:
> 
>  	for_each_possible_cpu(cpu)
> -		val += *(((__u32 *)&per_cpu(*stats, cpu) + idx));
> 
>  	return val;
> 
> But this is unrelated to the purpose of patch 6; it should just be fixed
> up in a modified patch 5, i.e.
> 
>  	for_each_possible_cpu(cpu)
> -		val += *(((__u32 *)&per_cpu(xfsstats, cpu) + idx));
> +		val += *(((__u32 *)per_cpu_ptr(stats, cpu) + idx));
> 	return val;
> 
> So I'll restrict review comments to the actual end product, rather than
> patch by patch, because it's tough to review patch X when it has issues
> whichi are fixed up by patch X+1 or X+2 or ...  :)
> 
> If you have any questions about all this, please let me know.

I'm doing another iteration of the series, and it will reflect your comments
and advice.
Thanks-
Bill

> 
> Thanks,
> -Eric
> 
> 
>  
> > ----------history---------------
> > v8: (add patches 6 and 7)
> > -patch 6: per-filesystem stats in sysfs.
> > Implement per-filesystem stats objects in sysfs. Stats objects are
> > instantiated when an xfs filesystem is mounted and deleted on unmount.
> > With this patch, the stats directory is created and populated with
> > the familiar stats and stats_clear files.
> >     Example:
> >             /sys/fs/xfs/sda9/stats/stats
> >             /sys/fs/xfs/sda9/stats/stats_clear
> > 
> > With this patch, the individual counts within the new per-fs
> > stats file(s) remain at zero. Functions that use the the macros
> > to increment, decrement, and add-to the per-fs stats counts will
> > be covered in the next patch (7).
> > 
> > Also, this patch includes some minor fixups for style issues in
> > patch 5.
> > 
> > 
> > -patch 7: per-filesystem stats counter implementation
> > Modify the stats counting macros and the callers
> > to those macros to properly increment, decrement, and add-to
> > the xfs stats counts. The counts for global and per-fs stats
> > are correctly advanced, and cleared by writing a "1" to the
> > corresponding clear file.
> > 
> > global counts: /sys/fs/xfs/stats/stats
> > per-fs counts: /sys/fs/xfs/sda*/stats/stats
> > 
> > global clear:  /sys/fs/xfs/stats/stats_clear
> > per-fs clear:  /sys/fs/xfs/sda*/stats/stats_clear
> > 
> > 
> > v7:
> > add patch 5/5: incorporate sysfs/kobject in xfsstats: handlers
> > take kobjects. Allocate & deallocate per-fs stats structures
> > and set up the sysfs entries for them. Add kobject and a pointer
> > to a per-cpu struct xfsstats. Modify the macros that manipulate
> > the stats accordingly.
> > 
> > v6:
> > -add patch 4/4: move to_xlog(kobject) to the relevant show/store
> > operations. This keeps the xfs_sysfs_object_show/store functions
> > generic. Also, with the change, there can be some cleanup of the
> > show/store function arguments.
> > 
> > v5:
> > -optimization of sysfs_ops function.
> > -style fixups
> > 
> > v4:
> > -whitespace fixup of patch 1
> > -add patch 4 (sysfs ops consolidation - dbg, stats, log)
> > 
> > v3:
> > -style fixups and removal of extraneous printk.
> > 
> > v2:
> > -style fixups.
> > v1:
> > --------------------------------
> > 
> > We already have per-fs information in /sys, so it makes sense to
> > have per-fs stats there too.  The series moves existing
> > global stats infrastructure to /sys and reuses that code to
> > create per-fs stats in /sys.
> > 
> > Patch 1 handles the bring-up and tear down of xfs/stats directory
> > structure in sysfs when an fs is mounted. The directory contains
> > the stats file and the stats_clear file. The stats file contents mimic
> > those of /proc/fs/xfs/stat. The stats_clear file is empty, and much
> > like the current stat_clear command, handles the zeroing of the stats
> > file when a "1" is echoed to the stats_clear file.
> > 
> > Patch 2 creates the symlink for stats from procfs to sysfs.
> > 
> > Patch 3 removes the now unused portions of procfs for stat.
> > 
> > Patch 4 consolidates the sysfs ops for dbg, stats, log.
> > 
> > Patch 5 allocates and deallocates per-fs stats structures and
> > sets up the sysfs entries for them. Add kobject and a pointer
> > to a per-cpu struct xfsstats. Modify the macros that manipulate
> > the stats accordingly.
> > 
> > Patch 6 implements per-filesystem stats objects in sysfs. Stats
> > objects are instantiated when an xfs filesystem is mounted and
> > deleted on unmount.
> > 
> > Patch 7 modifies the stats counting macros and the callers
> > to those macros to properly increment, decrement, and add-to
> > the xfs stats counts. The counts for global and per-fs stats
> > are correctly advanced, and cleared by writing a "1" to the
> > corresponding clear file.
> > 
> > Once again, comments and questions are welcome.
> > 
> > Thanks-
> > Bill
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> > 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
[prev in list] [next in list] [prev in thread] [next in thread] 