Re: [PATCH] btrfs: qgroup: add sysfs interface for debug

From: Qu Wenruo <wqu@suse.com>
To: dsterba@suse.cz, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH] btrfs: qgroup: add sysfs interface for debug
Date: Fri, 26 Jun 2020 07:21:31 +0800	[thread overview]
Message-ID: <5b2a9ae4-115c-28dd-affa-92b238929a15@suse.com> (raw)
In-Reply-To: <20200625202120.GZ27795@twin.jikos.cz>

On 2020/6/26 上午4:21, David Sterba wrote:
> On Fri, Jun 19, 2020 at 09:59:46AM +0800, Qu Wenruo wrote:
>> This patch will add the following sysfs interface:
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/rfer
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/excl
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/max_rfer
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/max_excl
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/lim_flags
>>  ^^^ Above are already in "btrfs qgroup show" command output ^^^
>>
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/rsv_data
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/rsv_meta_pertrans
>> /sys/fs/btrfs/<UUID>/qgroups/<qgroup_id>/rsv_meta_prealloc
>>
>> The last 3 rsv related members are not visible to users, but can be very
>> useful to debug qgroup limit related bugs.
> 
> I think debugging should not be the only usecase. The qgroups
> information can be accessed via ioctls or parsing output of the 'btrfs
> qgroup' commands. For that reason I suggest to pick better names of the
> fields, rfer, excl are not self explanatory and we can't change them in
> the structures. But we can for the sysfs interface.

Sure, I would go "reference" and "exclusive" then.

Although for rsv_* they are really for debug purpose, do they need
rename too?

> 
>> Also, to avoid '/' used in <qgroup_id>, the seperator between qgroup
>> level and qgroup id is changed to '_'.
> 
> This seems fine.
> 
>> The interface is not hidden behind 'debug' as I want this interface to
>> be included into production build so we could have an easier life to
>> debug qgroup rsv related bugs.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
...
>>  	u64 new_refcnt;
>> +
>> +	/*
>> +	 * Sysfs kobjectid
>> +	 */
>> +	struct kobject kobj;
>> +	struct completion kobj_unregister;
> 
> Why do you add the unregister completion to each qgroup? There's a
> pattern where the parent directory wait's until all its
> files/directories are released but I'm not sure if we need it here.

It looks like I'm a little paranoid here.
Since for qgroup we always release all qgroups then the parent
directory, the wait is not needed in theory.

> 
> The size of each qgroup would increase by about 100 bytes, which is not
> much but it adds up.
> 
>>  };
>>  
...
>> +#define QGROUP_ATTR(_member)						\
>> +static ssize_t btrfs_qgroup_show_##_member(struct kobject *qgroup_kobj,	\
>> +				      struct kobj_attribute *a, char *buf) \
>> +{									\
>> +	struct kobject *fsid_kobj = qgroup_kobj->parent->parent;	\
>> +	struct btrfs_fs_info *fs_info = to_fs_info(fsid_kobj);		\
>> +	struct btrfs_qgroup *qgroup = container_of(qgroup_kobj,		\
>> +			struct btrfs_qgroup, kobj);			\
>> +	u64 val;							\
>> +									\
>> +	spin_lock(&fs_info->qgroup_lock);				\
>> +	val = qgroup->_member;						\
>> +	spin_unlock(&fs_info->qgroup_lock);				\
>> +	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);		\
>> +}									\
>> +BTRFS_ATTR(qgroup, _member, btrfs_qgroup_show_##_member)
> 
> The macros need to follow the patterns we already have in sysfs.c,
> please have a look at eg. global_rsv_size_show or SPACE_INFO_ATTR. It
> reads the main pointers and calls btrfs_show_u64.

Oh, that's great, no need to bother the locking part.

But do I really need to follow the to_space_info() macro? Those macros
really bothers me, as it's not that clear what we're converting from.

> 
>> +
>> +#define QGROUP_RSV_ATTR(_name, _type)					\
>> +static ssize_t btrfs_qgroup_rsv_show_##_name(struct kobject *qgroup_kobj,\
>> +				      struct kobj_attribute *a, char *buf) \
>> +{									\
>> +	struct kobject *fsid_kobj = qgroup_kobj->parent->parent;	\
>> +	struct btrfs_fs_info *fs_info = to_fs_info(fsid_kobj);		\
>> +	struct btrfs_qgroup *qgroup = container_of(qgroup_kobj,		\
>> +			struct btrfs_qgroup, kobj);			\
>> +	u64 val;							\
>> +									\
>> +	spin_lock(&fs_info->qgroup_lock);				\
>> +	val = qgroup->rsv.values[_type];					\
>> +	spin_unlock(&fs_info->qgroup_lock);				\
>> +	return scnprintf(buf, PAGE_SIZE, "%llu\n", val);		\
>> +}									\
>> +BTRFS_ATTR(qgroup, rsv_##_name, btrfs_qgroup_rsv_show_##_name)
>> +
>> +QGROUP_ATTR(rfer);
>> +QGROUP_ATTR(excl);
>> +QGROUP_ATTR(max_rfer);
>> +QGROUP_ATTR(max_excl);
>> +QGROUP_ATTR(lim_flags);
>> +QGROUP_RSV_ATTR(data, BTRFS_QGROUP_RSV_DATA);
>> +QGROUP_RSV_ATTR(meta_pertrans, BTRFS_QGROUP_RSV_META_PERTRANS);
>> +QGROUP_RSV_ATTR(meta_prealloc, BTRFS_QGROUP_RSV_META_PREALLOC);
>> +
>> +static struct attribute *qgroup_attrs[] = {
>> +	BTRFS_ATTR_PTR(qgroup, rfer),
>> +	BTRFS_ATTR_PTR(qgroup, excl),
>> +	BTRFS_ATTR_PTR(qgroup, max_rfer),
>> +	BTRFS_ATTR_PTR(qgroup, max_excl),
>> +	BTRFS_ATTR_PTR(qgroup, lim_flags),
>> +	BTRFS_ATTR_PTR(qgroup, rsv_data),
>> +	BTRFS_ATTR_PTR(qgroup, rsv_meta_pertrans),
>> +	BTRFS_ATTR_PTR(qgroup, rsv_meta_prealloc),
>> +	NULL
>> +};
>> +ATTRIBUTE_GROUPS(qgroup);
>> +static void qgroup_release(struct kobject *kobj)
>> +{
>> +	struct btrfs_qgroup *qgroup = container_of(kobj, struct btrfs_qgroup,
>> +			kobj);
>> +	memset(&qgroup->kobj, 0, sizeof(*kobj));
>> +	complete(&qgroup->kobj_unregister);
>> +}
>> +
>> +static struct kobj_type qgroup_ktype = {
>> +	.sysfs_ops = &kobj_sysfs_ops,
>> +	.release = qgroup_release,
>> +	.default_groups = qgroup_groups,
>> +};
>> +
>> +/*
>> + * Needed string buffer size for qgroup, including tailing \0
>> + *
>> + * This includes U48_MAX + 1 + U16_MAX + 1.
> 
> What is U48_MAX?

For qgroup id, the upper 16 bits are for level and the the lower 48 bit
are for subvolume id.
So here we use U48 here.

But since the define is unused, it shouldn't be a problem any more.

> 
>> + * U48_MAX in dec can be 15 digits at, and U16_MAX can be 6 digits.
>> + * Rounded up to 32 to provide some buffer.
>> + */
>> +#define QGROUP_STR_LEN	32
> 
> Unused define
> 
>> +int btrfs_sysfs_add_one_qgroup(struct btrfs_fs_info *fs_info,
>> +				struct btrfs_qgroup *qgroup)
>> +{
>> +	struct kobject *qgroups_kobj = fs_info->qgroup_kobj;
>> +	int ret;
>> +
>> +	init_completion(&qgroup->kobj_unregister);
>> +	ret = kobject_init_and_add(&qgroup->kobj, &qgroup_ktype, qgroups_kobj,
>> +			"%u_%llu", (u16)btrfs_qgroup_level(qgroup->qgroupid),
> 
> %u does not match u16

But my compiler doesn't complain about this.

Thanks,
Qu

> 
>> +			btrfs_qgroup_subvolid(qgroup->qgroupid));
>> +	if (ret < 0)
>> +		kobject_put(&qgroup->kobj);
>> +
>> +	return ret;
>> +}
>> +
>> +void btrfs_sysfs_del_qgroups(struct btrfs_fs_info *fs_info)
>> +{
>> +	struct btrfs_qgroup *qgroup;
>> +	struct btrfs_qgroup *next;
>> +
>> +	rbtree_postorder_for_each_entry_safe(qgroup, next,
>> +			&fs_info->qgroup_tree, node) {
>> +		if (qgroup->kobj.state_initialized) {
>> +			kobject_del(&qgroup->kobj);
>> +			kobject_put(&qgroup->kobj);
>> +			wait_for_completion(&qgroup->kobj_unregister);
>> +		}
>> +	}
>> +	kobject_del(fs_info->qgroup_kobj);
>> +	kobject_put(fs_info->qgroup_kobj);
>> +	fs_info->qgroup_kobj = NULL;
>> +}
>> +
>> +/* Called when qgroup get initialized, thus there is no need for extra lock. */
>> +int btrfs_sysfs_add_qgroups(struct btrfs_fs_info *fs_info)
>> +{
>> +	struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj;
>> +	struct btrfs_qgroup *qgroup;
>> +	struct btrfs_qgroup *next;
>> +	int ret = 0;
>> +
>> +	ASSERT(fsid_kobj);
>> +	if (fs_info->qgroup_kobj)
>> +		return 0;
>> +
>> +	fs_info->qgroup_kobj = kobject_create_and_add("qgroups", fsid_kobj);
>> +	if (!fs_info->qgroup_kobj) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +	rbtree_postorder_for_each_entry_safe(qgroup, next,
>> +			&fs_info->qgroup_tree, node) {
>> +		ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
>> +		if (ret < 0)
>> +			goto out;
>> +	}
>> +
>> +out:
>> +	if (ret < 0)
>> +		btrfs_sysfs_del_qgroups(fs_info);
>> +	return ret;
>> +}
>> +
>> +void btrfs_sysfs_del_one_qgroup(struct btrfs_fs_info *fs_info,
>> +				struct btrfs_qgroup *qgroup)
>> +{
>> +	kobject_del(&qgroup->kobj);
>> +	kobject_put(&qgroup->kobj);
>> +	wait_for_completion(&qgroup->kobj_unregister);
>> +}
>>  
>>  /*
>>   * Change per-fs features in /sys/fs/btrfs/UUID/features to match current
>> diff --git a/fs/btrfs/sysfs.h b/fs/btrfs/sysfs.h
>> index 718a26c97833..1e27a9c94c84 100644
>> --- a/fs/btrfs/sysfs.h
>> +++ b/fs/btrfs/sysfs.h
>> @@ -36,4 +36,10 @@ int btrfs_sysfs_add_space_info_type(struct btrfs_fs_info *fs_info,
>>  void btrfs_sysfs_remove_space_info(struct btrfs_space_info *space_info);
>>  void btrfs_sysfs_update_devid(struct btrfs_device *device);
>>  
>> +int btrfs_sysfs_add_one_qgroup(struct btrfs_fs_info *fs_info,
>> +				struct btrfs_qgroup *qgroup);
>> +void btrfs_sysfs_del_qgroups(struct btrfs_fs_info *fs_info);
>> +int btrfs_sysfs_add_qgroups(struct btrfs_fs_info *fs_info);
>> +void btrfs_sysfs_del_one_qgroup(struct btrfs_fs_info *fs_info,
>> +				struct btrfs_qgroup *qgroup);
>>  #endif
>> -- 
>> 2.27.0