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

[Sidong Yang <realwakka@gmail.com>]

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