Re: [PATCH 5/7] xfs: incorporate sysfs/kobject in xfsstats: handlers take kobjects.

[Eric Sandeen <sandeen@sandeen.net>]

On 9/25/15 6:22 PM, Bill O'Donnell wrote:
> This patch is the next step toward per-fs xfs stats. Allocate &
> deallocate per-fs stats structures and set up the sysfs entries for them.
> 
> Instead of a single global xfsstats structure, add kobject and a pointer
> to a per-cpu struct xfsstats. Modify the macros that manipulate the stats
> accordingly: XFS_STATS_INC, XFS_STATS_DEC, and XFS_STATS_ADD now access
> xfsstats->xs_stats.
> 
> The sysfs functions need to get from the kobject back to the xfsstats
> structure which contains it, and pass the pointer to the ->xs_stats
> percpu structure into the show & clear routines.
> 
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>

<snip>

> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 0dfc53b..23e5681 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -61,7 +61,6 @@ static kmem_zone_t *xfs_ioend_zone;
>  mempool_t *xfs_ioend_pool;
>  
>  static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
> -static struct xfs_kobj xfs_stats_kobj;	/* global stats sysfs attrs */
>  #ifdef DEBUG
>  static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
>  #endif
> @@ -1802,6 +1801,7 @@ xfs_destroy_workqueues(void)
>  	destroy_workqueue(xfs_alloc_wq);
>  }
>  
> +
>  STATIC int __init
>  init_xfs_fs(void)
>  {
> @@ -1842,8 +1842,9 @@ init_xfs_fs(void)
>  		goto out_sysctl_unregister;
>  	}
>  
> -	xfs_stats_kobj.kobject.kset = xfs_kset;
> -	error = xfs_sysfs_init(&xfs_stats_kobj, &xfs_stats_ktype, NULL,
> +	xfsstats.xs_stats = alloc_percpu(struct xfsstats);

alloc_percpu can fail, so:

if (!xfsstats.xs_stats) {
	error = -ENOMEM;
	goto out_kset_unregister ...
}

> +	xfsstats.xs_kobj.kobject.kset = xfs_kset;
> +	error = xfs_sysfs_init(&xfsstats.xs_kobj, &xfs_stats_ktype, NULL,
>  			       "stats");
>  	if (error)
>  		goto out_kset_unregister;

If this fails, you need to free the percpu allocation, so this would
be "goto out_free_stats" I think

> @@ -1852,7 +1853,7 @@ init_xfs_fs(void)
>  	xfs_dbg_kobj.kobject.kset = xfs_kset;
>  	error = xfs_sysfs_init(&xfs_dbg_kobj, &xfs_dbg_ktype, NULL, "debug");
>  	if (error)
> -		goto out_remove_stats_kobj;
> +		goto out_remove_stats;

And this remains as out_remove_stats_kobj, I think.

>  #endif
>  
>  	error = xfs_qm_init();
> @@ -1869,9 +1870,9 @@ init_xfs_fs(void)
>   out_remove_dbg_kobj:
>  #ifdef DEBUG
>  	xfs_sysfs_del(&xfs_dbg_kobj);
> - out_remove_stats_kobj:
> + out_remove_stats:
>  #endif
> -	xfs_sysfs_del(&xfs_stats_kobj);
> +	free_percpu(xfsstats.xs_stats);

hm, now nothing deletes the stats kobject?

xfs_sysfs_del(&xfsstats.xs_kobj);

but there should be another goto target ...
there's one more thing to unwind (the percpu allocation) so you should
end up with more unwind goto target (i.e. out_free_stats), but you still
need the unwind target for the stats kobject sysfs deletion.

something like:

out_remove_dbg_kobj:
#ifdef DEBUG
	xfs_sysfs_del(&xfs_dbg_kobj);
out_remove_stats_kobj:
#endif
	xfs_sysfs_del(&xfs_stats_kobj);
out_free_stats:
	free_percpu(xfsstats.xs_stats);
out_kset_unregister:
	kset_unregister(xfs_kset);
out_sysctl_unregister:

but double-check me ;)

>   out_kset_unregister:
>  	kset_unregister(xfs_kset);
>   out_sysctl_unregister:
> @@ -1898,7 +1899,7 @@ exit_xfs_fs(void)
>  #ifdef DEBUG
>  	xfs_sysfs_del(&xfs_dbg_kobj);
>  #endif
> -	xfs_sysfs_del(&xfs_stats_kobj);
> +	free_percpu(xfsstats.xs_stats);


here as well, you still need to delete the stats kobject:

	xfs_sysfs_del(&xfsstats.xs_kobj); 

>  	kset_unregister(xfs_kset);
>  	xfs_sysctl_unregister();
>  	xfs_cleanup_procfs();

-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs