Re: [PATCH] xfs: show build options in sysfs

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Jun 07, 2019 at 08:41:09AM -0500, Eric Sandeen wrote:
> On 6/7/19 8:20 AM, Brian Foster wrote:
> > On Thu, Jun 06, 2019 at 10:30:09PM -0500, Eric Sandeen wrote:
> >> This adds the "build options" string to a sysfs entry:
> >>
> >> # cat /sys/fs/xfs/features/build_opts 
> >> ACLs, security attributes, realtime, scrub, no debug
> >>
> >> because right now we only get it in dmesg and scraping dmesg
> >> is not a great option.
> >>
> >> This is really /build options/, not features as in "on-disk,
> >> superblock features" like XFS_SB_FEAT_* - putting this under a
> >> features/ dir will leave the window open open to do supported
> >> superblock features ala ext4 & f2fs in the future if desired.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> No sure if it would make sense to have i.e.
> >>
> >> /sys/fs/xfs/features/build_options
> >> /sys/fs/xfs/features/finobt
> >> /sys/fs/xfs/features/rmapbt
> >> ...
> >>
> >> all in the same features/ dir ?
> >>
> > 
> > What's the purpose of the features dir, and why an entry per feature as
> > opposed to a single 'features' file like we're adding for build_opts?
> 
> just because ext4 and f2fs did it that way, and supposedly sysfs
> is one value per file.
> 
> also our entire sysfs structure is based around having dirs under xfs/
> - tbh I haven't yet sorted out how to actually ad a bare file under
> xfs/ ;)  Of course it's possible but existing infra in our code is friendlier
> with subdirs of files.  ;)

sysfs_create_file(&xfs_kset.kobj, ATTR_LIST(build_opts)); ?

sysfs is such a pain... I'm pretty sure that's a gross hack since a kset
doesn't look like it's supposed to have attributes.

> 
> > Would those per-feature files present any data? The patch seems
> > reasonable to me in general, but I'd prefer to see the directory
> > structure thing at least hashed out before we decide on this kind of
> > placement (as opposed to something like /sys/fs/xfs/build_opts, if that
> > is possible).
> 
> Yeah, that's why I raised the question above, stupid me ;)
> 
> > I see that ext4 has a per-file feature dir along these lines:
> > 
> > $ ls /sys/fs/ext4/features/
> > batched_discard  encryption  lazy_itable_init  meta_bg_resize  metadata_csum_seed
> > $ cat /sys/fs/ext4/features/*
> > supported
> > supported
> > supported
> > supported
> > supported
> > 
> > I'm not sure if those files disappear when a feature is not available or
> > persist and return something other than "supported?" Are those files
> > used by anything in userspace?
> 
> It's based on what the running code can support, i.e. the total of its
> COMPAT_FEATURES stuff.

If you do, can we please build a feature flags to strings decoder ring
so that xfs_db/kernel/mkfs/repair can standardize the names for all the
features?  I'd like to avoid the spinodes/sparse confusion again.

--D

> 
> >> Also I didn't test module unload/teardown as I'm testing on xfs root.
> >>
> > 
> > insmod/rmmod works on a quick test on one of my VMs.
> 
> thanks, sorry for being lazy there.
> 
> -Eric
> 
> > Brian
> > 
> >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >> index a14d11d78bd8..bc0e7fd63567 100644
> >> --- a/fs/xfs/xfs_super.c
> >> +++ b/fs/xfs/xfs_super.c
> >> @@ -55,9 +55,10 @@
> >>  static const struct super_operations xfs_super_operations;
> >>  struct bio_set xfs_ioend_bioset;
> >>  
> >> -static struct kset *xfs_kset;		/* top-level xfs sysfs dir */
> >> +static struct kset *xfs_kset;			/* top-level xfs sysfs dir */
> >> +static struct xfs_kobj xfs_features_kobj;	/* global features */
> >>  #ifdef DEBUG
> >> -static struct xfs_kobj xfs_dbg_kobj;	/* global debug sysfs attrs */
> >> +static struct xfs_kobj xfs_dbg_kobj;		/* global debug sysfs attrs */
> >>  #endif
> >>  
> >>  /*
> >> @@ -2134,11 +2135,16 @@ init_xfs_fs(void)
> >>  	if (error)
> >>  		goto out_free_stats;
> >>  
> >> +	xfs_features_kobj.kobject.kset = xfs_kset;
> >> +	error = xfs_sysfs_init(&xfs_features_kobj, &xfs_features_ktype,
> >> +				NULL, "features");
> >> +	if (error)
> >> +		goto out_remove_stats_kobj;
> >>  #ifdef DEBUG
> >>  	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_features_kobj;
> >>  #endif
> >>  
> >>  	error = xfs_qm_init();
> >> @@ -2155,8 +2161,10 @@ init_xfs_fs(void)
> >>   out_remove_dbg_kobj:
> >>  #ifdef DEBUG
> >>  	xfs_sysfs_del(&xfs_dbg_kobj);
> >> - out_remove_stats_kobj:
> >> + out_remove_features_kobj:
> >>  #endif
> >> +	xfs_sysfs_del(&xfs_features_kobj);
> >> + out_remove_stats_kobj:
> >>  	xfs_sysfs_del(&xfsstats.xs_kobj);
> >>   out_free_stats:
> >>  	free_percpu(xfsstats.xs_stats);
> >> @@ -2186,6 +2194,7 @@ exit_xfs_fs(void)
> >>  #ifdef DEBUG
> >>  	xfs_sysfs_del(&xfs_dbg_kobj);
> >>  #endif
> >> +	xfs_sysfs_del(&xfs_features_kobj);
> >>  	xfs_sysfs_del(&xfsstats.xs_kobj);
> >>  	free_percpu(xfsstats.xs_stats);
> >>  	kset_unregister(xfs_kset);
> >> diff --git a/fs/xfs/xfs_sysfs.c b/fs/xfs/xfs_sysfs.c
> >> index cabda13f3c64..98f36ad16237 100644
> >> --- a/fs/xfs/xfs_sysfs.c
> >> +++ b/fs/xfs/xfs_sysfs.c
> >> @@ -222,6 +222,28 @@ struct kobj_type xfs_dbg_ktype = {
> >>  
> >>  #endif /* DEBUG */
> >>  
> >> +/* features */
> >> +
> >> +STATIC ssize_t
> >> +build_opts_show(
> >> +	struct kobject	*kobject,
> >> +	char		*buf)
> >> +{
> >> +	return snprintf(buf, PAGE_SIZE, "%s\n", XFS_BUILD_OPTIONS);
> >> +}
> >> +XFS_SYSFS_ATTR_RO(build_opts);
> >> +
> >> +static struct attribute *xfs_features_attrs[] = {
> >> +	ATTR_LIST(build_opts),
> >> +	NULL,
> >> +};
> >> +
> >> +struct kobj_type xfs_features_ktype = {
> >> +	.release = xfs_sysfs_release,
> >> +	.sysfs_ops = &xfs_sysfs_ops,
> >> +	.default_attrs = xfs_features_attrs,
> >> +};
> >> +
> >>  /* stats */
> >>  
> >>  static inline struct xstats *
> >> diff --git a/fs/xfs/xfs_sysfs.h b/fs/xfs/xfs_sysfs.h
> >> index e9f810fc6731..e475f6b7eb91 100644
> >> --- a/fs/xfs/xfs_sysfs.h
> >> +++ b/fs/xfs/xfs_sysfs.h
> >> @@ -11,6 +11,7 @@ extern struct kobj_type xfs_mp_ktype;	/* xfs_mount */
> >>  extern struct kobj_type xfs_dbg_ktype;	/* debug */
> >>  extern struct kobj_type xfs_log_ktype;	/* xlog */
> >>  extern struct kobj_type xfs_stats_ktype;	/* stats */
> >> +extern struct kobj_type xfs_features_ktype;	/* features*/
> >>  
> >>  static inline struct xfs_kobj *
> >>  to_kobj(struct kobject *kobject)
> >>
>