[PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[Sidong Yang]

There are options '-c' in create subvolume and '-c' and '-x' in
snapshot. And the codes about them is there, but not in the manual or
help. This codes should be removed to avoid confusion.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 cmds/subvolume.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index fbf56566..408aebee 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -108,18 +108,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
 
 	optind = 0;
 	while (1) {
-		int c = getopt(argc, argv, "c:i:");
+		int c = getopt(argc, argv, "i:");
 		if (c < 0)
 			break;
 
 		switch (c) {
-		case 'c':
-			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
-			if (res) {
-				retval = res;
-				goto out;
-			}
-			break;
 		case 'i':
 			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
 			if (res) {
@@ -541,18 +534,11 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
 	memset(&args, 0, sizeof(args));
 	optind = 0;
 	while (1) {
-		int c = getopt(argc, argv, "c:i:r");
+		int c = getopt(argc, argv, "i:r");
 		if (c < 0)
 			break;
 
 		switch (c) {
-		case 'c':
-			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
-			if (res) {
-				retval = res;
-				goto out;
-			}
-			break;
 		case 'i':
 			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
 			if (res) {
@@ -563,13 +549,6 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
 		case 'r':
 			readonly = 1;
 			break;
-		case 'x':
-			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 1);
-			if (res) {
-				retval = res;
-				goto out;
-			}
-			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
-- 
2.25.1

[PATCH 2/2] btrfs-progs: qgroup: remove unused btrfs_qgroup_inherit_add_group()

[Sidong Yang]

The function btrfs_qgroup_inherit_add_group() was used for options in
create subvolume '-c' or snapshot '-c' or '-x' command. These options
are deleted and the function is not needed everywhere now. So we can
delete it.

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
 cmds/qgroup.c | 41 -----------------------------------------
 cmds/qgroup.h |  2 --
 2 files changed, 43 deletions(-)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 139dca97..3f59ba05 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -1477,47 +1477,6 @@ int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *
 	return 0;
 }
 
-int btrfs_qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
-			    int type)
-{
-	int ret;
-	u64 qgroup_src;
-	u64 qgroup_dst;
-	char *p;
-	int pos = 0;
-
-	p = strchr(arg, ':');
-	if (!p) {
-bad:
-		error("invalid copy specification, missing separator :");
-		return -EINVAL;
-	}
-	*p = 0;
-	qgroup_src = parse_qgroupid_or_path(arg);
-	qgroup_dst = parse_qgroupid_or_path(p + 1);
-	*p = ':';
-
-	if (!qgroup_src || !qgroup_dst)
-		goto bad;
-
-	if (*inherit)
-		pos = (*inherit)->num_qgroups +
-		      (*inherit)->num_ref_copies * 2 * type;
-
-	ret = qgroup_inherit_realloc(inherit, 2, pos);
-	if (ret)
-		return ret;
-
-	(*inherit)->qgroups[pos++] = qgroup_src;
-	(*inherit)->qgroups[pos++] = qgroup_dst;
-
-	if (!type)
-		++(*inherit)->num_ref_copies;
-	else
-		++(*inherit)->num_excl_copies;
-
-	return 0;
-}
 static const char * const qgroup_cmd_group_usage[] = {
 	"btrfs qgroup <command> [options] <path>",
 	NULL
diff --git a/cmds/qgroup.h b/cmds/qgroup.h
index 69b8c11f..f30cb8a8 100644
--- a/cmds/qgroup.h
+++ b/cmds/qgroup.h
@@ -38,8 +38,6 @@ struct btrfs_qgroup_stats {
 
 int btrfs_qgroup_inherit_size(struct btrfs_qgroup_inherit *p);
 int btrfs_qgroup_inherit_add_group(struct btrfs_qgroup_inherit **inherit, char *arg);
-int btrfs_qgroup_inherit_add_copy(struct btrfs_qgroup_inherit **inherit, char *arg,
-			    int type);
 int btrfs_qgroup_query(int fd, u64 qgroupid, struct btrfs_qgroup_stats *stats);
 
 #endif
-- 
2.25.1

Re: [PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[Qu Wenruo]

On 2022/3/9 16:14, Sidong Yang wrote:
> There are options '-c' in create subvolume and '-c' and '-x' in
> snapshot. And the codes about them is there, but not in the manual or
> help. This codes should be removed to avoid confusion.

I'd like more explanation on why we don't use it.

In fact the truth is, those -c/-x allows us to directly copy qgroup
numbers from other subvolumes when creating subvolume.

This is definitely going to screw up qgroup numbers.

Nowadays btrfs qgroup will automatically inherit the qgroup numbers when
-i option is used.

So the old -c/-x is no longer needed, and any inexperienced usage of
them will lead to inconsistent qgroup numbers anyway.

Thanks,
Qu

>
> Signed-off-by: Sidong Yang <realwakka@gmail.com>
> ---
>   cmds/subvolume.c | 25 ++-----------------------
>   1 file changed, 2 insertions(+), 23 deletions(-)
>
> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> index fbf56566..408aebee 100644
> --- a/cmds/subvolume.c
> +++ b/cmds/subvolume.c
> @@ -108,18 +108,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
>
>   	optind = 0;
>   	while (1) {
> -		int c = getopt(argc, argv, "c:i:");
> +		int c = getopt(argc, argv, "i:");
>   		if (c < 0)
>   			break;
>
>   		switch (c) {
> -		case 'c':
> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> -			if (res) {
> -				retval = res;
> -				goto out;
> -			}
> -			break;
>   		case 'i':
>   			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
>   			if (res) {
> @@ -541,18 +534,11 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
>   	memset(&args, 0, sizeof(args));
>   	optind = 0;
>   	while (1) {
> -		int c = getopt(argc, argv, "c:i:r");
> +		int c = getopt(argc, argv, "i:r");
>   		if (c < 0)
>   			break;
>
>   		switch (c) {
> -		case 'c':
> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> -			if (res) {
> -				retval = res;
> -				goto out;
> -			}
> -			break;
>   		case 'i':
>   			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
>   			if (res) {
> @@ -563,13 +549,6 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
>   		case 'r':
>   			readonly = 1;
>   			break;
> -		case 'x':
> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 1);
> -			if (res) {
> -				retval = res;
> -				goto out;
> -			}
> -			break;
>   		default:
>   			usage_unknown_option(cmd, argv);
>   		}

Re: [PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[Sidong Yang]

On Wed, Mar 09, 2022 at 04:52:15PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/3/9 16:14, Sidong Yang wrote:
> > There are options '-c' in create subvolume and '-c' and '-x' in
> > snapshot. And the codes about them is there, but not in the manual or
> > help. This codes should be removed to avoid confusion.
> 
> I'd like more explanation on why we don't use it.
> 
> In fact the truth is, those -c/-x allows us to directly copy qgroup
> numbers from other subvolumes when creating subvolume.
> 
> This is definitely going to screw up qgroup numbers.

Actually, I don't understand that you said this option screw up qgroup
numbers. Could you explain more?
I checked that -c/-x options are not working. The commands are like
below.

# btrfs qgroup create 1/0 /mnt
# btrfs qgroup create 1/1 /mnt
# btrfs subvol create -c 1/0:1/1 /mnt/a

But it's not working. It seems that it ignores btrfs_qgroup_inherit
argument. New subvolume doesn't inherit anything.

> 
> Nowadays btrfs qgroup will automatically inherit the qgroup numbers when
> -i option is used.

Totally agree. -i option is enough to use.

Thanks,
Sidong
> 
> So the old -c/-x is no longer needed, and any inexperienced usage of
> them will lead to inconsistent qgroup numbers anyway.
> 
> Thanks,
> Qu
> 
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >   cmds/subvolume.c | 25 ++-----------------------
> >   1 file changed, 2 insertions(+), 23 deletions(-)
> > 
> > diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> > index fbf56566..408aebee 100644
> > --- a/cmds/subvolume.c
> > +++ b/cmds/subvolume.c
> > @@ -108,18 +108,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
> > 
> >   	optind = 0;
> >   	while (1) {
> > -		int c = getopt(argc, argv, "c:i:");
> > +		int c = getopt(argc, argv, "i:");
> >   		if (c < 0)
> >   			break;
> > 
> >   		switch (c) {
> > -		case 'c':
> > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> > -			if (res) {
> > -				retval = res;
> > -				goto out;
> > -			}
> > -			break;
> >   		case 'i':
> >   			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> >   			if (res) {
> > @@ -541,18 +534,11 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
> >   	memset(&args, 0, sizeof(args));
> >   	optind = 0;
> >   	while (1) {
> > -		int c = getopt(argc, argv, "c:i:r");
> > +		int c = getopt(argc, argv, "i:r");
> >   		if (c < 0)
> >   			break;
> > 
> >   		switch (c) {
> > -		case 'c':
> > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> > -			if (res) {
> > -				retval = res;
> > -				goto out;
> > -			}
> > -			break;
> >   		case 'i':
> >   			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> >   			if (res) {
> > @@ -563,13 +549,6 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
> >   		case 'r':
> >   			readonly = 1;
> >   			break;
> > -		case 'x':
> > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 1);
> > -			if (res) {
> > -				retval = res;
> > -				goto out;
> > -			}
> > -			break;
> >   		default:
> >   			usage_unknown_option(cmd, argv);
> >   		}

Re: [PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[Qu Wenruo]

On 2022/3/9 21:17, Sidong Yang wrote:
> On Wed, Mar 09, 2022 at 04:52:15PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2022/3/9 16:14, Sidong Yang wrote:
>>> There are options '-c' in create subvolume and '-c' and '-x' in
>>> snapshot. And the codes about them is there, but not in the manual or
>>> help. This codes should be removed to avoid confusion.
>>
>> I'd like more explanation on why we don't use it.
>>
>> In fact the truth is, those -c/-x allows us to directly copy qgroup
>> numbers from other subvolumes when creating subvolume.
>>
>> This is definitely going to screw up qgroup numbers.
>
> Actually, I don't understand that you said this option screw up qgroup
> numbers. Could you explain more?
> I checked that -c/-x options are not working. The commands are like
> below.
>
> # btrfs qgroup create 1/0 /mnt
> # btrfs qgroup create 1/1 /mnt
> # btrfs subvol create -c 1/0:1/1 /mnt/a
>
> But it's not working. It seems that it ignores btrfs_qgroup_inherit
> argument. New subvolume doesn't inherit anything.

Those -c/-x is for qgroup refer/exclusive copy.

Check btrfs_qgroup_inherit(), there are two for loops, one for
num_ref_copies, the other for num_excl_copies.

In those loops, we just copy the number directly.
(And of course, mark qgroup inconsistent).

Thanks,
Qu

>
>>
>> Nowadays btrfs qgroup will automatically inherit the qgroup numbers when
>> -i option is used.
>
> Totally agree. -i option is enough to use.
>
> Thanks,
> Sidong
>>
>> So the old -c/-x is no longer needed, and any inexperienced usage of
>> them will lead to inconsistent qgroup numbers anyway.
>>
>> Thanks,
>> Qu
>>
>>>
>>> Signed-off-by: Sidong Yang <realwakka@gmail.com>
>>> ---
>>>    cmds/subvolume.c | 25 ++-----------------------
>>>    1 file changed, 2 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/cmds/subvolume.c b/cmds/subvolume.c
>>> index fbf56566..408aebee 100644
>>> --- a/cmds/subvolume.c
>>> +++ b/cmds/subvolume.c
>>> @@ -108,18 +108,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
>>>
>>>    	optind = 0;
>>>    	while (1) {
>>> -		int c = getopt(argc, argv, "c:i:");
>>> +		int c = getopt(argc, argv, "i:");
>>>    		if (c < 0)
>>>    			break;
>>>
>>>    		switch (c) {
>>> -		case 'c':
>>> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
>>> -			if (res) {
>>> -				retval = res;
>>> -				goto out;
>>> -			}
>>> -			break;
>>>    		case 'i':
>>>    			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
>>>    			if (res) {
>>> @@ -541,18 +534,11 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
>>>    	memset(&args, 0, sizeof(args));
>>>    	optind = 0;
>>>    	while (1) {
>>> -		int c = getopt(argc, argv, "c:i:r");
>>> +		int c = getopt(argc, argv, "i:r");
>>>    		if (c < 0)
>>>    			break;
>>>
>>>    		switch (c) {
>>> -		case 'c':
>>> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
>>> -			if (res) {
>>> -				retval = res;
>>> -				goto out;
>>> -			}
>>> -			break;
>>>    		case 'i':
>>>    			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
>>>    			if (res) {
>>> @@ -563,13 +549,6 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
>>>    		case 'r':
>>>    			readonly = 1;
>>>    			break;
>>> -		case 'x':
>>> -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 1);
>>> -			if (res) {
>>> -				retval = res;
>>> -				goto out;
>>> -			}
>>> -			break;
>>>    		default:
>>>    			usage_unknown_option(cmd, argv);
>>>    		}

Re: [PATCH 1/2] btrfs-progs: subvolme: remove unused options for create and snapshot

[Sidong Yang]

On Wed, Mar 09, 2022 at 09:25:45PM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/3/9 21:17, Sidong Yang wrote:
> > On Wed, Mar 09, 2022 at 04:52:15PM +0800, Qu Wenruo wrote:
> > > 
> > > 
> > > On 2022/3/9 16:14, Sidong Yang wrote:
> > > > There are options '-c' in create subvolume and '-c' and '-x' in
> > > > snapshot. And the codes about them is there, but not in the manual or
> > > > help. This codes should be removed to avoid confusion.
> > > 
> > > I'd like more explanation on why we don't use it.
> > > 
> > > In fact the truth is, those -c/-x allows us to directly copy qgroup
> > > numbers from other subvolumes when creating subvolume.
> > > 
> > > This is definitely going to screw up qgroup numbers.
> > 
> > Actually, I don't understand that you said this option screw up qgroup
> > numbers. Could you explain more?
> > I checked that -c/-x options are not working. The commands are like
> > below.
> > 
> > # btrfs qgroup create 1/0 /mnt
> > # btrfs qgroup create 1/1 /mnt
> > # btrfs subvol create -c 1/0:1/1 /mnt/a
> > 
> > But it's not working. It seems that it ignores btrfs_qgroup_inherit
> > argument. New subvolume doesn't inherit anything.
> 
> Those -c/-x is for qgroup refer/exclusive copy.
> 
> Check btrfs_qgroup_inherit(), there are two for loops, one for
> num_ref_copies, the other for num_excl_copies.
> 
> In those loops, we just copy the number directly.
> (And of course, mark qgroup inconsistent).

I see that it just copies rfer/excl directly. Thanks!
I'll add more explaination for next version.

Thanks,
Sidong
> 
> Thanks,
> Qu
> 
> > 
> > > 
> > > Nowadays btrfs qgroup will automatically inherit the qgroup numbers when
> > > -i option is used.
> > 
> > Totally agree. -i option is enough to use.
> > 
> > Thanks,
> > Sidong
> > > 
> > > So the old -c/-x is no longer needed, and any inexperienced usage of
> > > them will lead to inconsistent qgroup numbers anyway.
> > > 
> > > Thanks,
> > > Qu
> > > 
> > > > 
> > > > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > > > ---
> > > >    cmds/subvolume.c | 25 ++-----------------------
> > > >    1 file changed, 2 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> > > > index fbf56566..408aebee 100644
> > > > --- a/cmds/subvolume.c
> > > > +++ b/cmds/subvolume.c
> > > > @@ -108,18 +108,11 @@ static int cmd_subvol_create(const struct cmd_struct *cmd,
> > > > 
> > > >    	optind = 0;
> > > >    	while (1) {
> > > > -		int c = getopt(argc, argv, "c:i:");
> > > > +		int c = getopt(argc, argv, "i:");
> > > >    		if (c < 0)
> > > >    			break;
> > > > 
> > > >    		switch (c) {
> > > > -		case 'c':
> > > > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> > > > -			if (res) {
> > > > -				retval = res;
> > > > -				goto out;
> > > > -			}
> > > > -			break;
> > > >    		case 'i':
> > > >    			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> > > >    			if (res) {
> > > > @@ -541,18 +534,11 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
> > > >    	memset(&args, 0, sizeof(args));
> > > >    	optind = 0;
> > > >    	while (1) {
> > > > -		int c = getopt(argc, argv, "c:i:r");
> > > > +		int c = getopt(argc, argv, "i:r");
> > > >    		if (c < 0)
> > > >    			break;
> > > > 
> > > >    		switch (c) {
> > > > -		case 'c':
> > > > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 0);
> > > > -			if (res) {
> > > > -				retval = res;
> > > > -				goto out;
> > > > -			}
> > > > -			break;
> > > >    		case 'i':
> > > >    			res = btrfs_qgroup_inherit_add_group(&inherit, optarg);
> > > >    			if (res) {
> > > > @@ -563,13 +549,6 @@ static int cmd_subvol_snapshot(const struct cmd_struct *cmd,
> > > >    		case 'r':
> > > >    			readonly = 1;
> > > >    			break;
> > > > -		case 'x':
> > > > -			res = btrfs_qgroup_inherit_add_copy(&inherit, optarg, 1);
> > > > -			if (res) {
> > > > -				retval = res;
> > > > -				goto out;
> > > > -			}
> > > > -			break;
> > > >    		default:
> > > >    			usage_unknown_option(cmd, argv);
> > > >    		}