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

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

[Qu Wenruo]

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 */
 	struct btrfs_qgroup_limit lim;
 	__u64	qgroups[0];
 };
-- 
2.15.1

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

[Nikolay Borisov]

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

>  	struct btrfs_qgroup_limit lim;
>  	__u64	qgroups[0];
>  };
> 

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

[Qu Wenruo]

[-- 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 --]

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

[Nikolay Borisov]

On 19.12.2017 13:20, Qu Wenruo wrote:
> 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.

Right, I saw the check now, missed it the first time. I think we should
remove it and the code should completely ignore these values. Ideally,
we would have removed them from qgroup_inherit struct as well but this
means breaking the on-disk format. So let's ignore them altogether.

> 
> Thanks,

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

[Qu Wenruo]

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



On 2017年12月19日 19:24, Nikolay Borisov wrote:
> 
> 
> On 19.12.2017 13:20, Qu Wenruo wrote:
>> 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.
> 
> Right, I saw the check now, missed it the first time. I think we should
> remove it and the code should completely ignore these values. Ideally,
> we would have removed them from qgroup_inherit struct as well but this
> means breaking the on-disk format. So let's ignore them altogether.

Ignoring the values completely seems quite reasonable.

I'll update the patch.

Thanks,
Qu
> 
>>
>> Thanks,


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

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

[Omar Sandoval]

On Tue, Dec 19, 2017 at 06:45:11PM +0800, 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>

And reported to Nikolay by me ;)

My only objection is that we shouldn't rename the field names in the
UAPI header. Let's just add a comment that the two counters are ignored.
Besides that,

Reviewed-by: Omar Sandoval <osandov@fb.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.

If we want this functionality, we should add a specific ioctl for it
instead of piggy-backing off of subvolume creation.

Thanks!

> ---
>  fs/btrfs/qgroup.c          | 56 ++++++++++++++--------------------------------
>  include/uapi/linux/btrfs.h |  4 ++--
>  2 files changed, 19 insertions(+), 41 deletions(-)

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

[Nikolay Borisov]

On 19.12.2017 21:01, Omar Sandoval wrote:
> My only objection is that we shouldn't rename the field names in the
> UAPI header. Let's just add a comment that the two counters are ignored.
> Besides that,

Why is that?

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

[Omar Sandoval]

On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote:
> 
> 
> On 19.12.2017 21:01, Omar Sandoval wrote:
> > My only objection is that we shouldn't rename the field names in the
> > UAPI header. Let's just add a comment that the two counters are ignored.
> > Besides that,
> 
> Why is that?

We don't know if anyone is including the UAPI header and referring to
these fields for whatever reason. A quick Google search doesn't turn up
anything, but it has been there forever so I think we should err on the
side of not breaking the API.

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

[Omar Sandoval]

On Tue, Dec 19, 2017 at 11:36:58AM -0800, Omar Sandoval wrote:
> On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote:
> > 
> > 
> > On 19.12.2017 21:01, Omar Sandoval wrote:
> > > My only objection is that we shouldn't rename the field names in the
> > > UAPI header. Let's just add a comment that the two counters are ignored.
> > > Besides that,
> > 
> > Why is that?
> 
> We don't know if anyone is including the UAPI header and referring to
> these fields for whatever reason. A quick Google search doesn't turn up
> anything, but it has been there forever so I think we should err on the
> side of not breaking the API.

At least snapper sets these:
https://github.com/openSUSE/snapper/blob/3c126c92bf2bd25a952800b2efb18754148ac227/snapper/BtrfsUtils.cc#L147

They get it from the libbtrfs header, but we might some day sync it to
the UAPI header. So it's not completely theoretical :)

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

[Nikolay Borisov]

On 19.12.2017 21:36, Omar Sandoval wrote:
> On Tue, Dec 19, 2017 at 09:19:10PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 19.12.2017 21:01, Omar Sandoval wrote:
>>> My only objection is that we shouldn't rename the field names in the
>>> UAPI header. Let's just add a comment that the two counters are ignored.
>>> Besides that,
>>
>> Why is that?
> 
> We don't know if anyone is including the UAPI header and referring to
> these fields for whatever reason. A quick Google search doesn't turn up
> anything, but it has been there forever so I think we should err on the
> side of not breaking the API.

So it makes sense, but I think we can be brave in this case and really
obscure them. Since from where I'm standing it looks someone made a
_really_ bad decision in making this debug aid part of the on-disk
format and now we have to live with that decision for eternity. I think
teh fact this was not documented provides enough safety for allowing us
to rename the members.

What if in the future we'd like to repurpose those values and use them
for something else it would make sense to rename an unused member to
something meaningful. "I know we have the don't break userspace" rule
but in this case I'm more inclined to disregard for the sake of
long-term maintainability. In the end it's not _that_ big of an issue so
I won't be anal about it but I really hate the situation we find
ourselves in - shit is merged and subsequently we can't really fix it ...