Re: [RFC PATCH] btrfs: qgroup: Deprecate the ability to manually inherit rfer/excl numbers

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4981 bytes --]



On 2017年12月19日 19:12, Nikolay Borisov wrote:
> 
> 
> On 19.12.2017 12:45, Qu Wenruo wrote:
>> btrfs_qgroup_inherit structure has two members, num_ref_copies and
>> num_excl_copies, to info btrfs kernel modules to inherit (copy)
>> rfer/excl numbers at snapshot/subvolume creation time.
>>
>> Since qgroup number is already hard to maintain for multi-level qgroup
>> scenario, allowing user to manually manipulate qgroup inherit is quite
>> easy to screw up qgroup numbers.
>>
>> Although btrfs-progs supports such inheritance specification, the
>> options are hidden from user and not documented.
>> So there is no need to allow user to manually specify inheritance in
>> kernel.
>>
>> Reported-by: Nikolay Borisov <nborisov@suse.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> The only concern is, currently we don't have good tool to handle
>> inheritance of multi-level qgroups.
>> The only method to get qgroup numbers correct is to run a quota rescan.
>>
>> So there may be some case where experienced (well, mostly a developer)
>> user can use the hidden btrfs-progs options or manually craft an ioctl
>> to handle multi-level qgroups without costly rescan.
>> ---
>>  fs/btrfs/qgroup.c          | 56 ++++++++++++++--------------------------------
>>  include/uapi/linux/btrfs.h |  4 ++--
>>  2 files changed, 19 insertions(+), 41 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 168fd03ca3ac..d8a2413272f9 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -2158,9 +2158,24 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>  	}
>>  
>>  	if (inherit) {
>> +		/*
>> +		 * num_excl/rfer_copies indicate how many qgroup pairs needs
>> +		 * to be manually inherited (copy rfer or excl from src
>> +		 * qgroup to dst)
>> +		 *
>> +		 * Allowing user to manipulate inheritance can easily cause
>> +		 * problem in multi-level qgroup scenario.
>> +		 * And the ioctl interface is hidden in btrfs-progs for a long
>> +		 * time, deprecate them should not be a big problem.
>> +		 */
>> +		if (inherit->__num_excl_copies || inherit->__num_ref_copies) {
>> +			ret = -ENOTTY;
>> +			btrfs_warn(fs_info,
>> +			"manually inherit excl/rfer is no longer supported");
>> +			goto out;
>> +		}
>>  		i_qgroups = (u64 *)(inherit + 1);
>> -		nums = inherit->num_qgroups + 2 * inherit->num_ref_copies +
>> -		       2 * inherit->num_excl_copies;
>> +		nums = inherit->num_qgroups;
>>  		for (i = 0; i < nums; ++i) {
>>  			srcgroup = find_qgroup_rb(fs_info, *i_qgroups);
>>  
>> @@ -2286,43 +2301,6 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans,
>>  		++i_qgroups;
>>  	}
>>  
>> -	for (i = 0; i <  inherit->num_ref_copies; ++i, i_qgroups += 2) {
>> -		struct btrfs_qgroup *src;
>> -		struct btrfs_qgroup *dst;
>> -
>> -		if (!i_qgroups[0] || !i_qgroups[1])
>> -			continue;
>> -
>> -		src = find_qgroup_rb(fs_info, i_qgroups[0]);
>> -		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
>> -
>> -		if (!src || !dst) {
>> -			ret = -EINVAL;
>> -			goto unlock;
>> -		}
>> -
>> -		dst->rfer = src->rfer - level_size;
>> -		dst->rfer_cmpr = src->rfer_cmpr - level_size;
>> -	}
>> -	for (i = 0; i <  inherit->num_excl_copies; ++i, i_qgroups += 2) {
>> -		struct btrfs_qgroup *src;
>> -		struct btrfs_qgroup *dst;
>> -
>> -		if (!i_qgroups[0] || !i_qgroups[1])
>> -			continue;
>> -
>> -		src = find_qgroup_rb(fs_info, i_qgroups[0]);
>> -		dst = find_qgroup_rb(fs_info, i_qgroups[1]);
>> -
>> -		if (!src || !dst) {
>> -			ret = -EINVAL;
>> -			goto unlock;
>> -		}
>> -
>> -		dst->excl = src->excl + level_size;
>> -		dst->excl_cmpr = src->excl_cmpr + level_size;
>> -	}
>> -
>>  unlock:
>>  	spin_unlock(&fs_info->qgroup_lock);
>>  out:
>> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
>> index ce615b75e855..099e088414d6 100644
>> --- a/include/uapi/linux/btrfs.h
>> +++ b/include/uapi/linux/btrfs.h
>> @@ -80,8 +80,8 @@ struct btrfs_qgroup_limit {
>>  struct btrfs_qgroup_inherit {
>>  	__u64	flags;
>>  	__u64	num_qgroups;
>> -	__u64	num_ref_copies;
>> -	__u64	num_excl_copies;
>> +	__u64	__num_ref_copies;	/* DEPRECATED */
>> +	__u64	__num_excl_copies;	/* DEPRECATED */
> 
> I'd prefer we name them something even more generic i.e. :
> pad1, pad2 or unused1, unused2 to really deter any efforts to use them.
> I guess this could shouldn't have been merged in the first place ...

Naming like pad1/2 will make the check in btrfs_qgroup_inherit() look
quite weird.

Although I don't have any better idea, so I'm mostly fine with such rename.

Thanks,
Qu

> 
>>  	struct btrfs_qgroup_limit lim;
>>  	__u64	qgroups[0];
>>  };
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]