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

[Eric Sandeen <sandeen@redhat.com>]

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.  ;)

> 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.

>> 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)
>>