[PATCH 0/8] BtrFS: QGroups uapi improvements

[PATCH 0/8] BtrFS: QGroups uapi improvements

[Sargun Dhillon]

This patchset contains some improvements to qgroups. It changes the
semantics around how qgroups are dealt with when subvolumes are
deleted, and it also adds two new ioctls for qgroup deletion
and addition.

The new semantic around qgroup removal is that when the qgroup_nokeep
mount flag is set, it when a subvolume is deleted, the associated
level-0 qgroup will also be removed. This does not trickle up to
high level qgroups.

In addition, it adds two new ioctls for qgroup addition and removal
which have flags to protect against creating qgroups for non-existent
volumes, and in addition flags to prevent the deletion of qgroups
that are associated with volumes.

Sargun Dhillon (8):
  btrfs: Split up btrfs_remove_qgroup, no logic changes
  btrfs: Fail on removing qgroup if del_qgroup_item fails
  btrfs: Split up btrfs_create_qgroup, no logic changes
  btrfs: autoremove qgroup by default, and add a mount flag to override
  btrfs: qgroup.h whitespace change
  btrfs: Add code to check if a qgroup's subvol exists
  btrfs: Add code to prevent qgroup creation for a non-existent subvol
  btrfs: Add new ioctl uapis for qgroup creation / removal

 fs/btrfs/ctree.h           |   1 +
 fs/btrfs/ioctl.c           | 116 ++++++++++++++++++++++++++++++++++++-
 fs/btrfs/qgroup.c          | 140 ++++++++++++++++++++++++++++++++++-----------
 fs/btrfs/qgroup.h          |   6 +-
 fs/btrfs/super.c           |  16 +++++-
 include/uapi/linux/btrfs.h |  23 ++++++++
 6 files changed, 264 insertions(+), 38 deletions(-)

-- 
2.9.3

[PATCH 1/8] btrfs: Split up btrfs_remove_qgroup, no logic changes

[Sargun Dhillon]

This change is purely a style change, so that btrfs_remove_qgroup is
split. There is some whitespace changes, but this shouldn't have any
effect on the logic of the code.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/qgroup.c | 49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index deffbeb..a2add44 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1281,40 +1281,35 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid)
+/* Must be called with qgroup_ioctl_lock held */
+static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
+				 struct btrfs_fs_info *fs_info, u64 qgroupid)
 {
+	struct btrfs_qgroup_list *list;
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
-	struct btrfs_qgroup_list *list;
-	int ret = 0;
+	int ret;
 
-	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (!quota_root)
+		return -EINVAL;
 
 	qgroup = find_qgroup_rb(fs_info, qgroupid);
-	if (!qgroup) {
-		ret = -ENOENT;
-		goto out;
-	} else {
-		/* check if there are no children of this qgroup */
-		if (!list_empty(&qgroup->members)) {
-			ret = -EBUSY;
-			goto out;
-		}
-	}
+	if (!qgroup)
+		return -ENOENT;
+
+	/* check if there are no children of this qgroup */
+	if (!list_empty(&qgroup->members))
+		return -EBUSY;
+
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
 
 	while (!list_empty(&qgroup->groups)) {
 		list = list_first_entry(&qgroup->groups,
 					struct btrfs_qgroup_list, next_group);
 		ret = __del_qgroup_relation(trans, fs_info,
-					   qgroupid,
-					   list->group->qgroupid);
+					    qgroupid,
+					    list->group->qgroupid);
 		if (ret)
 			goto out;
 	}
@@ -1322,8 +1317,20 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	spin_lock(&fs_info->qgroup_lock);
 	del_qgroup_rb(fs_info, qgroupid);
 	spin_unlock(&fs_info->qgroup_lock);
+
 out:
+	return ret;
+}
+
+int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
+			struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+	int ret;
+
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
+	ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
+
 	return ret;
 }
 
-- 
2.9.3

[PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails

[Sargun Dhillon]

Previously, we were calling del_qgroup_item, and ignoring the return code
resulting in a potential to have divergent in-memory state without an
error. Perhaps, it makes sense to handle this error code, and put the
filesystem into a read only, or similar state.

This patch only adds reporting of the error.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/qgroup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a2add44..7e772ba 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1303,6 +1303,8 @@ static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 		return -EBUSY;
 
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
+	if (ret)
+		goto out;
 
 	while (!list_empty(&qgroup->groups)) {
 		list = list_first_entry(&qgroup->groups,
-- 
2.9.3

Re: [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails

[Qu Wenruo]

At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
> Previously, we were calling del_qgroup_item, and ignoring the return code
> resulting in a potential to have divergent in-memory state without an
> error. Perhaps, it makes sense to handle this error code, and put the
> filesystem into a read only, or similar state.
> 
> This patch only adds reporting of the error.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>   fs/btrfs/qgroup.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index a2add44..7e772ba 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1303,6 +1303,8 @@ static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>   		return -EBUSY;
>   
>   	ret = del_qgroup_item(trans, quota_root, qgroupid);
> +	if (ret)
> +		goto out;

I think we should continue to remove in-memory qgroup relationship even 
we didn't find corresponding qgroup item.

IIRC it's better to catch less severe error like -ENOENT and continue.

Although normally previous find_qgroup_rb() should return -ENOENT before 
we continue to del_qgroup_item(), but catching -ENOENT here could make 
the code more robust.

Thanks,
Qu

>   
>   	while (!list_empty(&qgroup->groups)) {
>   		list = list_first_entry(&qgroup->groups,
> 

Re: [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails

[Sargun Dhillon]

On Sun, May 21, 2017 at 6:14 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>
> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>
>> Previously, we were calling del_qgroup_item, and ignoring the return code
>> resulting in a potential to have divergent in-memory state without an
>> error. Perhaps, it makes sense to handle this error code, and put the
>> filesystem into a read only, or similar state.
>>
>> This patch only adds reporting of the error.
>>
>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> ---
>>   fs/btrfs/qgroup.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index a2add44..7e772ba 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1303,6 +1303,8 @@ static int __btrfs_remove_qgroup(struct
>> btrfs_trans_handle *trans,
>>                 return -EBUSY;
>>         ret = del_qgroup_item(trans, quota_root, qgroupid);
>> +       if (ret)
>> +               goto out;
>
>
> I think we should continue to remove in-memory qgroup relationship even we
> didn't find corresponding qgroup item.
>
> IIRC it's better to catch less severe error like -ENOENT and continue.
>
> Although normally previous find_qgroup_rb() should return -ENOENT before we
> continue to del_qgroup_item(), but catching -ENOENT here could make the code
> more robust.
Agreed. Will fix this. I think it also return ENOMEM.
>
> Thanks,
> Qu
>
>
>>         while (!list_empty(&qgroup->groups)) {
>>                 list = list_first_entry(&qgroup->groups,
>>
>
>

[PATCH 3/8] btrfs: Split up btrfs_create_qgroup, no logic changes

[Sargun Dhillon]

This change is purely a style change, so that btrfs_create_qgroup is
split. There is some whitespace changes, but this shouldn't have any
effect on the logic of the code.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/qgroup.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 7e772ba..588248b 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1247,24 +1247,21 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid)
+/* Must be called with qgroup_ioctl_lock held */
+static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
+				 struct btrfs_fs_info *fs_info, u64 qgroupid)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
-	int ret = 0;
+	int ret;
 
-	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	quota_root = fs_info->quota_root;
-	if (!quota_root) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (!quota_root)
+		return -EINVAL;
+
 	qgroup = find_qgroup_rb(fs_info, qgroupid);
-	if (qgroup) {
-		ret = -EEXIST;
-		goto out;
-	}
+	if (qgroup)
+		return -EEXIST;
 
 	ret = add_qgroup_item(trans, quota_root, qgroupid);
 	if (ret)
@@ -1277,7 +1274,18 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 	if (IS_ERR(qgroup))
 		ret = PTR_ERR(qgroup);
 out:
+	return ret;
+}
+
+int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
+			struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+	int ret;
+
+	mutex_lock(&fs_info->qgroup_ioctl_lock);
+	ret = __btrfs_create_qgroup(trans, fs_info, qgroupid);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
+
 	return ret;
 }
 
-- 
2.9.3

[PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

This change follows the change to automatically remove qgroups
if the associated subvolume has also been removed. It changes
the default behaviour to automatically remove qgroups when
a subvolume is deleted, but this can be override with the
qgroup_keep mount flag.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/ctree.h |  1 +
 fs/btrfs/ioctl.c | 14 ++++++++++++++
 fs/btrfs/super.c | 16 +++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 643c70d..4d57eb6 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
 #define BTRFS_MOUNT_FRAGMENT_METADATA	(1 << 25)
 #define BTRFS_MOUNT_FREE_SPACE_TREE	(1 << 26)
 #define BTRFS_MOUNT_NOLOGREPLAY		(1 << 27)
+#define BTRFS_MOUNT_QGROUP_KEEP		(1 << 28)
 
 #define BTRFS_DEFAULT_COMMIT_INTERVAL	(30)
 #define BTRFS_DEFAULT_MAX_INLINE	(2048)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e176375..b10d7bb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		}
 	}
 
+	/*
+	 * Attempt to automatically remove the automatically attached qgroup
+	 * setup in btrfs_qgroup_inherit. As a matter of convention, the id
+	 * is the same as the subvolume id.
+	 *
+	 * This can fail non-fatally for level 0 qgroups, and potentially
+	 * leave the filesystem in an awkward, (but working) state.
+	 */
+	if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
+		ret = btrfs_remove_qgroup(trans, fs_info,
+					  dest->root_key.objectid);
+		if (ret && ret != -ENOENT)
+			pr_info("Could not automatically delete qgroup: %d\n", ret);
+	}
 out_end_trans:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4f1cdd5..232e1b8 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -321,7 +321,7 @@ enum {
 	Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
 	Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
 	Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
-	Opt_nologreplay, Opt_norecovery,
+	Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep, Opt_qgroup_nokeep,
 #ifdef CONFIG_BTRFS_DEBUG
 	Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
 #endif
@@ -381,6 +381,8 @@ static const match_table_t tokens = {
 	{Opt_rescan_uuid_tree, "rescan_uuid_tree"},
 	{Opt_fatal_errors, "fatal_errors=%s"},
 	{Opt_commit_interval, "commit=%d"},
+	{Opt_qgroup_keep, "qgroup_keep"},
+	{Opt_qgroup_nokeep, "qgroup_nokeep"},
 #ifdef CONFIG_BTRFS_DEBUG
 	{Opt_fragment_data, "fragment=data"},
 	{Opt_fragment_metadata, "fragment=metadata"},
@@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 				info->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
 			}
 			break;
+		case Opt_qgroup_keep:
+			btrfs_set_and_info(info, QGROUP_KEEP,
+					   "do not automatically delete qgroups");
+			break;
+		case Opt_qgroup_nokeep:
+			btrfs_clear_and_info(info, QGROUP_KEEP,
+					     "automatically delete qgroups");
+			break;
 #ifdef CONFIG_BTRFS_DEBUG
 		case Opt_fragment_all:
 			btrfs_info(info, "fragmenting all space");
@@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
 		seq_puts(seq, ",fatal_errors=panic");
 	if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
 		seq_printf(seq, ",commit=%d", info->commit_interval);
+	if (btrfs_test_opt(info, QGROUP_KEEP))
+		seq_puts(seq, ",qgroup_keep");
+	else
+		seq_puts(seq, ",qgroup_nokeep");
 #ifdef CONFIG_BTRFS_DEBUG
 	if (btrfs_test_opt(info, FRAGMENT_DATA))
 		seq_puts(seq, ",fragment=data");
-- 
2.9.3

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

Just as a follow-up, folks are working around this in userspace:
https://github.com/moby/moby/pull/29427


On Sat, May 20, 2017 at 1:39 AM, Sargun Dhillon <sargun@sargun.me> wrote:
> This change follows the change to automatically remove qgroups
> if the associated subvolume has also been removed. It changes
> the default behaviour to automatically remove qgroups when
> a subvolume is deleted, but this can be override with the
> qgroup_keep mount flag.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>  fs/btrfs/ctree.h |  1 +
>  fs/btrfs/ioctl.c | 14 ++++++++++++++
>  fs/btrfs/super.c | 16 +++++++++++++++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 643c70d..4d57eb6 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
>  #define BTRFS_MOUNT_FRAGMENT_METADATA  (1 << 25)
>  #define BTRFS_MOUNT_FREE_SPACE_TREE    (1 << 26)
>  #define BTRFS_MOUNT_NOLOGREPLAY                (1 << 27)
> +#define BTRFS_MOUNT_QGROUP_KEEP                (1 << 28)
>
>  #define BTRFS_DEFAULT_COMMIT_INTERVAL  (30)
>  #define BTRFS_DEFAULT_MAX_INLINE       (2048)
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e176375..b10d7bb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>                 }
>         }
>
> +       /*
> +        * Attempt to automatically remove the automatically attached qgroup
> +        * setup in btrfs_qgroup_inherit. As a matter of convention, the id
> +        * is the same as the subvolume id.
> +        *
> +        * This can fail non-fatally for level 0 qgroups, and potentially
> +        * leave the filesystem in an awkward, (but working) state.
> +        */
> +       if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
> +               ret = btrfs_remove_qgroup(trans, fs_info,
> +                                         dest->root_key.objectid);
> +               if (ret && ret != -ENOENT)
> +                       pr_info("Could not automatically delete qgroup: %d\n", ret);
> +       }
>  out_end_trans:
>         trans->block_rsv = NULL;
>         trans->bytes_reserved = 0;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 4f1cdd5..232e1b8 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -321,7 +321,7 @@ enum {
>         Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>         Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>         Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
> -       Opt_nologreplay, Opt_norecovery,
> +       Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep, Opt_qgroup_nokeep,
>  #ifdef CONFIG_BTRFS_DEBUG
>         Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>  #endif
> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>         {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>         {Opt_fatal_errors, "fatal_errors=%s"},
>         {Opt_commit_interval, "commit=%d"},
> +       {Opt_qgroup_keep, "qgroup_keep"},
> +       {Opt_qgroup_nokeep, "qgroup_nokeep"},
>  #ifdef CONFIG_BTRFS_DEBUG
>         {Opt_fragment_data, "fragment=data"},
>         {Opt_fragment_metadata, "fragment=metadata"},
> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>                                 info->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
>                         }
>                         break;
> +               case Opt_qgroup_keep:
> +                       btrfs_set_and_info(info, QGROUP_KEEP,
> +                                          "do not automatically delete qgroups");
> +                       break;
> +               case Opt_qgroup_nokeep:
> +                       btrfs_clear_and_info(info, QGROUP_KEEP,
> +                                            "automatically delete qgroups");
> +                       break;
>  #ifdef CONFIG_BTRFS_DEBUG
>                 case Opt_fragment_all:
>                         btrfs_info(info, "fragmenting all space");
> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
>                 seq_puts(seq, ",fatal_errors=panic");
>         if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>                 seq_printf(seq, ",commit=%d", info->commit_interval);
> +       if (btrfs_test_opt(info, QGROUP_KEEP))
> +               seq_puts(seq, ",qgroup_keep");
> +       else
> +               seq_puts(seq, ",qgroup_nokeep");
>  #ifdef CONFIG_BTRFS_DEBUG
>         if (btrfs_test_opt(info, FRAGMENT_DATA))
>                 seq_puts(seq, ",fragment=data");
> --
> 2.9.3
>

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Qu Wenruo]

At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
> This change follows the change to automatically remove qgroups
> if the associated subvolume has also been removed. It changes
> the default behaviour to automatically remove qgroups when
> a subvolume is deleted, but this can be override with the
> qgroup_keep mount flag.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>   fs/btrfs/ctree.h |  1 +
>   fs/btrfs/ioctl.c | 14 ++++++++++++++
>   fs/btrfs/super.c | 16 +++++++++++++++-
>   3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 643c70d..4d57eb6 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct btrfs_fs_info *info)
>   #define BTRFS_MOUNT_FRAGMENT_METADATA	(1 << 25)
>   #define BTRFS_MOUNT_FREE_SPACE_TREE	(1 << 26)
>   #define BTRFS_MOUNT_NOLOGREPLAY		(1 << 27)
> +#define BTRFS_MOUNT_QGROUP_KEEP		(1 << 28)
>   
>   #define BTRFS_DEFAULT_COMMIT_INTERVAL	(30)
>   #define BTRFS_DEFAULT_MAX_INLINE	(2048)
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index e176375..b10d7bb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>   		}
>   	}
>   
> +	/*
> +	 * Attempt to automatically remove the automatically attached qgroup
> +	 * setup in btrfs_qgroup_inherit. As a matter of convention, the id
> +	 * is the same as the subvolume id.
> +	 *
> +	 * This can fail non-fatally for level 0 qgroups, and potentially
> +	 * leave the filesystem in an awkward, (but working) state.
> +	 */
> +	if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
> +		ret = btrfs_remove_qgroup(trans, fs_info,
> +					  dest->root_key.objectid);
> +		if (ret && ret != -ENOENT)
> +			pr_info("Could not automatically delete qgroup: %d\n", ret);
> +	}
>   out_end_trans:
>   	trans->block_rsv = NULL;
>   	trans->bytes_reserved = 0;
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 4f1cdd5..232e1b8 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -321,7 +321,7 @@ enum {
>   	Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>   	Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>   	Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
> -	Opt_nologreplay, Opt_norecovery,
> +	Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep, Opt_qgroup_nokeep,

I prefer to add new meanings for btrfs_ioctl_qgroup_create_args->create.

It's a u64 value while we only checks if it's zero or not, to determine 
if it's creation or deletion.

We could reuse it to extent the create/delete behavior, other than a new 
mount option, which is a global flag, just to alter qgroup behavior.

Thanks,
Qu

>   #ifdef CONFIG_BTRFS_DEBUG
>   	Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>   #endif
> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>   	{Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>   	{Opt_fatal_errors, "fatal_errors=%s"},
>   	{Opt_commit_interval, "commit=%d"},
> +	{Opt_qgroup_keep, "qgroup_keep"},
> +	{Opt_qgroup_nokeep, "qgroup_nokeep"},
>   #ifdef CONFIG_BTRFS_DEBUG
>   	{Opt_fragment_data, "fragment=data"},
>   	{Opt_fragment_metadata, "fragment=metadata"},
> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
>   				info->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
>   			}
>   			break;
> +		case Opt_qgroup_keep:
> +			btrfs_set_and_info(info, QGROUP_KEEP,
> +					   "do not automatically delete qgroups");
> +			break;
> +		case Opt_qgroup_nokeep:
> +			btrfs_clear_and_info(info, QGROUP_KEEP,
> +					     "automatically delete qgroups");
> +			break;
>   #ifdef CONFIG_BTRFS_DEBUG
>   		case Opt_fragment_all:
>   			btrfs_info(info, "fragmenting all space");
> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq, struct dentry *dentry)
>   		seq_puts(seq, ",fatal_errors=panic");
>   	if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>   		seq_printf(seq, ",commit=%d", info->commit_interval);
> +	if (btrfs_test_opt(info, QGROUP_KEEP))
> +		seq_puts(seq, ",qgroup_keep");
> +	else
> +		seq_puts(seq, ",qgroup_nokeep");
>   #ifdef CONFIG_BTRFS_DEBUG
>   	if (btrfs_test_opt(info, FRAGMENT_DATA))
>   		seq_puts(seq, ",fragment=data");
> 

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

On Sun, May 21, 2017 at 6:20 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>
> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>
>> This change follows the change to automatically remove qgroups
>> if the associated subvolume has also been removed. It changes
>> the default behaviour to automatically remove qgroups when
>> a subvolume is deleted, but this can be override with the
>> qgroup_keep mount flag.
>>
>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> ---
>>   fs/btrfs/ctree.h |  1 +
>>   fs/btrfs/ioctl.c | 14 ++++++++++++++
>>   fs/btrfs/super.c | 16 +++++++++++++++-
>>   3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 643c70d..4d57eb6 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct
>> btrfs_fs_info *info)
>>   #define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25)
>>   #define BTRFS_MOUNT_FREE_SPACE_TREE   (1 << 26)
>>   #define BTRFS_MOUNT_NOLOGREPLAY               (1 << 27)
>> +#define BTRFS_MOUNT_QGROUP_KEEP                (1 << 28)
>>     #define BTRFS_DEFAULT_COMMIT_INTERVAL       (30)
>>   #define BTRFS_DEFAULT_MAX_INLINE      (2048)
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index e176375..b10d7bb 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct
>> file *file,
>>                 }
>>         }
>>   +     /*
>> +        * Attempt to automatically remove the automatically attached
>> qgroup
>> +        * setup in btrfs_qgroup_inherit. As a matter of convention, the
>> id
>> +        * is the same as the subvolume id.
>> +        *
>> +        * This can fail non-fatally for level 0 qgroups, and potentially
>> +        * leave the filesystem in an awkward, (but working) state.
>> +        */
>> +       if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>> +               ret = btrfs_remove_qgroup(trans, fs_info,
>> +                                         dest->root_key.objectid);
>> +               if (ret && ret != -ENOENT)
>> +                       pr_info("Could not automatically delete qgroup:
>> %d\n", ret);
>> +       }
>>   out_end_trans:
>>         trans->block_rsv = NULL;
>>         trans->bytes_reserved = 0;
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 4f1cdd5..232e1b8 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -321,7 +321,7 @@ enum {
>>         Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>>         Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>>         Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
>> -       Opt_nologreplay, Opt_norecovery,
>> +       Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep,
>> Opt_qgroup_nokeep,
>
>
> I prefer to add new meanings for btrfs_ioctl_qgroup_create_args->create.
I have two issues with this:
1) Existing software, like Docker Daemon. There's no way I can tell
Docker daemon to use this new flag to create qgroups. Although, I
could modify it, it doesn't fit into the API very well.
2) How do I do this with qgroups that already exist, or qgroups that
are created on-demand by qgroup_inherit?

I think both of these would be fixed if we copied (a subset) of the
qgroup flags. If you look at my other patches, I think we want to
remove the ability to remove level-0 qgroups (which are inherited), so
we'd also want to add an ioctl to be able to modify (some of those)
flags. Do you think that adding inheritance, and a new ioctl is a
reasonable approach?

>
> It's a u64 value while we only checks if it's zero or not, to determine if
> it's creation or deletion.
>
> We could reuse it to extent the create/delete behavior, other than a new
> mount option, which is a global flag, just to alter qgroup behavior.
>
> Thanks,
> Qu
>
>
>>   #ifdef CONFIG_BTRFS_DEBUG
>>         Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>>   #endif
>> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>>         {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>>         {Opt_fatal_errors, "fatal_errors=%s"},
>>         {Opt_commit_interval, "commit=%d"},
>> +       {Opt_qgroup_keep, "qgroup_keep"},
>> +       {Opt_qgroup_nokeep, "qgroup_nokeep"},
>>   #ifdef CONFIG_BTRFS_DEBUG
>>         {Opt_fragment_data, "fragment=data"},
>>         {Opt_fragment_metadata, "fragment=metadata"},
>> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info,
>> char *options,
>>                                 info->commit_interval =
>> BTRFS_DEFAULT_COMMIT_INTERVAL;
>>                         }
>>                         break;
>> +               case Opt_qgroup_keep:
>> +                       btrfs_set_and_info(info, QGROUP_KEEP,
>> +                                          "do not automatically delete
>> qgroups");
>> +                       break;
>> +               case Opt_qgroup_nokeep:
>> +                       btrfs_clear_and_info(info, QGROUP_KEEP,
>> +                                            "automatically delete
>> qgroups");
>> +                       break;
>>   #ifdef CONFIG_BTRFS_DEBUG
>>                 case Opt_fragment_all:
>>                         btrfs_info(info, "fragmenting all space");
>> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq,
>> struct dentry *dentry)
>>                 seq_puts(seq, ",fatal_errors=panic");
>>         if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>>                 seq_printf(seq, ",commit=%d", info->commit_interval);
>> +       if (btrfs_test_opt(info, QGROUP_KEEP))
>> +               seq_puts(seq, ",qgroup_keep");
>> +       else
>> +               seq_puts(seq, ",qgroup_nokeep");
>>   #ifdef CONFIG_BTRFS_DEBUG
>>         if (btrfs_test_opt(info, FRAGMENT_DATA))
>>                 seq_puts(seq, ",fragment=data");
>>
>
>

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Qu Wenruo]

At 05/22/2017 09:58 AM, Sargun Dhillon wrote:
> On Sun, May 21, 2017 at 6:20 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>
>>
>> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>>
>>> This change follows the change to automatically remove qgroups
>>> if the associated subvolume has also been removed. It changes
>>> the default behaviour to automatically remove qgroups when
>>> a subvolume is deleted, but this can be override with the
>>> qgroup_keep mount flag.
>>>
>>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>>> ---
>>>    fs/btrfs/ctree.h |  1 +
>>>    fs/btrfs/ioctl.c | 14 ++++++++++++++
>>>    fs/btrfs/super.c | 16 +++++++++++++++-
>>>    3 files changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>> index 643c70d..4d57eb6 100644
>>> --- a/fs/btrfs/ctree.h
>>> +++ b/fs/btrfs/ctree.h
>>> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct
>>> btrfs_fs_info *info)
>>>    #define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25)
>>>    #define BTRFS_MOUNT_FREE_SPACE_TREE   (1 << 26)
>>>    #define BTRFS_MOUNT_NOLOGREPLAY               (1 << 27)
>>> +#define BTRFS_MOUNT_QGROUP_KEEP                (1 << 28)
>>>      #define BTRFS_DEFAULT_COMMIT_INTERVAL       (30)
>>>    #define BTRFS_DEFAULT_MAX_INLINE      (2048)
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index e176375..b10d7bb 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -2544,6 +2544,20 @@ static noinline int btrfs_ioctl_snap_destroy(struct
>>> file *file,
>>>                  }
>>>          }
>>>    +     /*
>>> +        * Attempt to automatically remove the automatically attached
>>> qgroup
>>> +        * setup in btrfs_qgroup_inherit. As a matter of convention, the
>>> id
>>> +        * is the same as the subvolume id.
>>> +        *
>>> +        * This can fail non-fatally for level 0 qgroups, and potentially
>>> +        * leave the filesystem in an awkward, (but working) state.
>>> +        */
>>> +       if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>>> +               ret = btrfs_remove_qgroup(trans, fs_info,
>>> +                                         dest->root_key.objectid);
>>> +               if (ret && ret != -ENOENT)
>>> +                       pr_info("Could not automatically delete qgroup:
>>> %d\n", ret);
>>> +       }
>>>    out_end_trans:
>>>          trans->block_rsv = NULL;
>>>          trans->bytes_reserved = 0;
>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>> index 4f1cdd5..232e1b8 100644
>>> --- a/fs/btrfs/super.c
>>> +++ b/fs/btrfs/super.c
>>> @@ -321,7 +321,7 @@ enum {
>>>          Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>>>          Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>>>          Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
>>> -       Opt_nologreplay, Opt_norecovery,
>>> +       Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep,
>>> Opt_qgroup_nokeep,
>>
>>
>> I prefer to add new meanings for btrfs_ioctl_qgroup_create_args->create.
> I have two issues with this:
> 1) Existing software, like Docker Daemon. There's no way I can tell
> Docker daemon to use this new flag to create qgroups. Although, I
> could modify it, it doesn't fit into the API very well.
> 2) How do I do this with qgroups that already exist, or qgroups that
> are created on-demand by qgroup_inherit?
> 
> I think both of these would be fixed if we copied (a subset) of the
> qgroup flags. If you look at my other patches, I think we want to
> remove the ability to remove level-0 qgroups (which are inherited), so
> we'd also want to add an ioctl to be able to modify (some of those)
> flags. Do you think that adding inheritance, and a new ioctl is a
> reasonable approach?

Sorry, I didn't see later patches.

Yes, new API is a better method to solve this problem.

But with new APIs, the need of new mount option is even less.
Outputting a warning message in dmesg is good enough to info user to use 
new API.

Thanks,
Qu

> 
>>
>> It's a u64 value while we only checks if it's zero or not, to determine if
>> it's creation or deletion.
>>
>> We could reuse it to extent the create/delete behavior, other than a new
>> mount option, which is a global flag, just to alter qgroup behavior.
>>
>> Thanks,
>> Qu
>>
>>
>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>          Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>>>    #endif
>>> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>>>          {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>>>          {Opt_fatal_errors, "fatal_errors=%s"},
>>>          {Opt_commit_interval, "commit=%d"},
>>> +       {Opt_qgroup_keep, "qgroup_keep"},
>>> +       {Opt_qgroup_nokeep, "qgroup_nokeep"},
>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>          {Opt_fragment_data, "fragment=data"},
>>>          {Opt_fragment_metadata, "fragment=metadata"},
>>> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info,
>>> char *options,
>>>                                  info->commit_interval =
>>> BTRFS_DEFAULT_COMMIT_INTERVAL;
>>>                          }
>>>                          break;
>>> +               case Opt_qgroup_keep:
>>> +                       btrfs_set_and_info(info, QGROUP_KEEP,
>>> +                                          "do not automatically delete
>>> qgroups");
>>> +                       break;
>>> +               case Opt_qgroup_nokeep:
>>> +                       btrfs_clear_and_info(info, QGROUP_KEEP,
>>> +                                            "automatically delete
>>> qgroups");
>>> +                       break;
>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>                  case Opt_fragment_all:
>>>                          btrfs_info(info, "fragmenting all space");
>>> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file *seq,
>>> struct dentry *dentry)
>>>                  seq_puts(seq, ",fatal_errors=panic");
>>>          if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>>>                  seq_printf(seq, ",commit=%d", info->commit_interval);
>>> +       if (btrfs_test_opt(info, QGROUP_KEEP))
>>> +               seq_puts(seq, ",qgroup_keep");
>>> +       else
>>> +               seq_puts(seq, ",qgroup_nokeep");
>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>          if (btrfs_test_opt(info, FRAGMENT_DATA))
>>>                  seq_puts(seq, ",fragment=data");
>>>
>>
>>
> 
> 

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Sargun Dhillon]

On Sun, May 21, 2017 at 7:03 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>
> At 05/22/2017 09:58 AM, Sargun Dhillon wrote:
>>
>> On Sun, May 21, 2017 at 6:20 PM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>> wrote:
>>>
>>>
>>>
>>> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>>>
>>>>
>>>> This change follows the change to automatically remove qgroups
>>>> if the associated subvolume has also been removed. It changes
>>>> the default behaviour to automatically remove qgroups when
>>>> a subvolume is deleted, but this can be override with the
>>>> qgroup_keep mount flag.
>>>>
>>>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>>>> ---
>>>>    fs/btrfs/ctree.h |  1 +
>>>>    fs/btrfs/ioctl.c | 14 ++++++++++++++
>>>>    fs/btrfs/super.c | 16 +++++++++++++++-
>>>>    3 files changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>>> index 643c70d..4d57eb6 100644
>>>> --- a/fs/btrfs/ctree.h
>>>> +++ b/fs/btrfs/ctree.h
>>>> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const
>>>> struct
>>>> btrfs_fs_info *info)
>>>>    #define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25)
>>>>    #define BTRFS_MOUNT_FREE_SPACE_TREE   (1 << 26)
>>>>    #define BTRFS_MOUNT_NOLOGREPLAY               (1 << 27)
>>>> +#define BTRFS_MOUNT_QGROUP_KEEP                (1 << 28)
>>>>      #define BTRFS_DEFAULT_COMMIT_INTERVAL       (30)
>>>>    #define BTRFS_DEFAULT_MAX_INLINE      (2048)
>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>> index e176375..b10d7bb 100644
>>>> --- a/fs/btrfs/ioctl.c
>>>> +++ b/fs/btrfs/ioctl.c
>>>> @@ -2544,6 +2544,20 @@ static noinline int
>>>> btrfs_ioctl_snap_destroy(struct
>>>> file *file,
>>>>                  }
>>>>          }
>>>>    +     /*
>>>> +        * Attempt to automatically remove the automatically attached
>>>> qgroup
>>>> +        * setup in btrfs_qgroup_inherit. As a matter of convention, the
>>>> id
>>>> +        * is the same as the subvolume id.
>>>> +        *
>>>> +        * This can fail non-fatally for level 0 qgroups, and
>>>> potentially
>>>> +        * leave the filesystem in an awkward, (but working) state.
>>>> +        */
>>>> +       if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>>>> +               ret = btrfs_remove_qgroup(trans, fs_info,
>>>> +                                         dest->root_key.objectid);
>>>> +               if (ret && ret != -ENOENT)
>>>> +                       pr_info("Could not automatically delete qgroup:
>>>> %d\n", ret);
>>>> +       }
>>>>    out_end_trans:
>>>>          trans->block_rsv = NULL;
>>>>          trans->bytes_reserved = 0;
>>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>>> index 4f1cdd5..232e1b8 100644
>>>> --- a/fs/btrfs/super.c
>>>> +++ b/fs/btrfs/super.c
>>>> @@ -321,7 +321,7 @@ enum {
>>>>          Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>>>>          Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>>>>          Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
>>>> -       Opt_nologreplay, Opt_norecovery,
>>>> +       Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep,
>>>> Opt_qgroup_nokeep,
>>>
>>>
>>>
>>> I prefer to add new meanings for btrfs_ioctl_qgroup_create_args->create.
So, just to bring this full circle before I do a v2, we want to make
it so on creation time, or after creation time you can make a qgroup
autoremove. And we want this to be a flag that gets inherited if
SUBVOL_QGROUP_INHERIT is set?

>>
>> I have two issues with this:
>> 1) Existing software, like Docker Daemon. There's no way I can tell
>> Docker daemon to use this new flag to create qgroups. Although, I
>> could modify it, it doesn't fit into the API very well.
>> 2) How do I do this with qgroups that already exist, or qgroups that
>> are created on-demand by qgroup_inherit?
>>
>> I think both of these would be fixed if we copied (a subset) of the
>> qgroup flags. If you look at my other patches, I think we want to
>> remove the ability to remove level-0 qgroups (which are inherited), so
>> we'd also want to add an ioctl to be able to modify (some of those)
>> flags. Do you think that adding inheritance, and a new ioctl is a
>> reasonable approach?
>
>
> Sorry, I didn't see later patches.
>
> Yes, new API is a better method to solve this problem.
>
> But with new APIs, the need of new mount option is even less.
> Outputting a warning message in dmesg is good enough to info user to use new
> API.
>
> Thanks,
> Qu
>
>
>>
>>>
>>> It's a u64 value while we only checks if it's zero or not, to determine
>>> if
>>> it's creation or deletion.
>>>
>>> We could reuse it to extent the create/delete behavior, other than a new
>>> mount option, which is a global flag, just to alter qgroup behavior.
>>>
>>> Thanks,
>>> Qu
>>>
>>>
>>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>>          Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>>>>    #endif
>>>> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>>>>          {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>>>>          {Opt_fatal_errors, "fatal_errors=%s"},
>>>>          {Opt_commit_interval, "commit=%d"},
>>>> +       {Opt_qgroup_keep, "qgroup_keep"},
>>>> +       {Opt_qgroup_nokeep, "qgroup_nokeep"},
>>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>>          {Opt_fragment_data, "fragment=data"},
>>>>          {Opt_fragment_metadata, "fragment=metadata"},
>>>> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info,
>>>> char *options,
>>>>                                  info->commit_interval =
>>>> BTRFS_DEFAULT_COMMIT_INTERVAL;
>>>>                          }
>>>>                          break;
>>>> +               case Opt_qgroup_keep:
>>>> +                       btrfs_set_and_info(info, QGROUP_KEEP,
>>>> +                                          "do not automatically delete
>>>> qgroups");
>>>> +                       break;
>>>> +               case Opt_qgroup_nokeep:
>>>> +                       btrfs_clear_and_info(info, QGROUP_KEEP,
>>>> +                                            "automatically delete
>>>> qgroups");
>>>> +                       break;
>>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>>                  case Opt_fragment_all:
>>>>                          btrfs_info(info, "fragmenting all space");
>>>> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file
>>>> *seq,
>>>> struct dentry *dentry)
>>>>                  seq_puts(seq, ",fatal_errors=panic");
>>>>          if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>>>>                  seq_printf(seq, ",commit=%d", info->commit_interval);
>>>> +       if (btrfs_test_opt(info, QGROUP_KEEP))
>>>> +               seq_puts(seq, ",qgroup_keep");
>>>> +       else
>>>> +               seq_puts(seq, ",qgroup_nokeep");
>>>>    #ifdef CONFIG_BTRFS_DEBUG
>>>>          if (btrfs_test_opt(info, FRAGMENT_DATA))
>>>>                  seq_puts(seq, ",fragment=data");
>>>>
>>>
>>>
>>
>>
>
>

Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override

[Qu Wenruo]

At 05/23/2017 01:31 AM, Sargun Dhillon wrote:
> On Sun, May 21, 2017 at 7:03 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>
>>
>> At 05/22/2017 09:58 AM, Sargun Dhillon wrote:
>>>
>>> On Sun, May 21, 2017 at 6:20 PM, Qu Wenruo <quwenruo@cn.fujitsu.com>
>>> wrote:
>>>>
>>>>
>>>>
>>>> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>>>>
>>>>>
>>>>> This change follows the change to automatically remove qgroups
>>>>> if the associated subvolume has also been removed. It changes
>>>>> the default behaviour to automatically remove qgroups when
>>>>> a subvolume is deleted, but this can be override with the
>>>>> qgroup_keep mount flag.
>>>>>
>>>>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>>>>> ---
>>>>>     fs/btrfs/ctree.h |  1 +
>>>>>     fs/btrfs/ioctl.c | 14 ++++++++++++++
>>>>>     fs/btrfs/super.c | 16 +++++++++++++++-
>>>>>     3 files changed, 30 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>>>>> index 643c70d..4d57eb6 100644
>>>>> --- a/fs/btrfs/ctree.h
>>>>> +++ b/fs/btrfs/ctree.h
>>>>> @@ -1348,6 +1348,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const
>>>>> struct
>>>>> btrfs_fs_info *info)
>>>>>     #define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25)
>>>>>     #define BTRFS_MOUNT_FREE_SPACE_TREE   (1 << 26)
>>>>>     #define BTRFS_MOUNT_NOLOGREPLAY               (1 << 27)
>>>>> +#define BTRFS_MOUNT_QGROUP_KEEP                (1 << 28)
>>>>>       #define BTRFS_DEFAULT_COMMIT_INTERVAL       (30)
>>>>>     #define BTRFS_DEFAULT_MAX_INLINE      (2048)
>>>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>>>> index e176375..b10d7bb 100644
>>>>> --- a/fs/btrfs/ioctl.c
>>>>> +++ b/fs/btrfs/ioctl.c
>>>>> @@ -2544,6 +2544,20 @@ static noinline int
>>>>> btrfs_ioctl_snap_destroy(struct
>>>>> file *file,
>>>>>                   }
>>>>>           }
>>>>>     +     /*
>>>>> +        * Attempt to automatically remove the automatically attached
>>>>> qgroup
>>>>> +        * setup in btrfs_qgroup_inherit. As a matter of convention, the
>>>>> id
>>>>> +        * is the same as the subvolume id.
>>>>> +        *
>>>>> +        * This can fail non-fatally for level 0 qgroups, and
>>>>> potentially
>>>>> +        * leave the filesystem in an awkward, (but working) state.
>>>>> +        */
>>>>> +       if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>>>>> +               ret = btrfs_remove_qgroup(trans, fs_info,
>>>>> +                                         dest->root_key.objectid);
>>>>> +               if (ret && ret != -ENOENT)
>>>>> +                       pr_info("Could not automatically delete qgroup:
>>>>> %d\n", ret);
>>>>> +       }
>>>>>     out_end_trans:
>>>>>           trans->block_rsv = NULL;
>>>>>           trans->bytes_reserved = 0;
>>>>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>>>>> index 4f1cdd5..232e1b8 100644
>>>>> --- a/fs/btrfs/super.c
>>>>> +++ b/fs/btrfs/super.c
>>>>> @@ -321,7 +321,7 @@ enum {
>>>>>           Opt_commit_interval, Opt_barrier, Opt_nodefrag, Opt_nodiscard,
>>>>>           Opt_noenospc_debug, Opt_noflushoncommit, Opt_acl, Opt_datacow,
>>>>>           Opt_datasum, Opt_treelog, Opt_noinode_cache, Opt_usebackuproot,
>>>>> -       Opt_nologreplay, Opt_norecovery,
>>>>> +       Opt_nologreplay, Opt_norecovery, Opt_qgroup_keep,
>>>>> Opt_qgroup_nokeep,
>>>>
>>>>
>>>>
>>>> I prefer to add new meanings for btrfs_ioctl_qgroup_create_args->create.
> So, just to bring this full circle before I do a v2, we want to make
> it so on creation time, or after creation time you can make a qgroup
> autoremove. And we want this to be a flag that gets inherited if
> SUBVOL_QGROUP_INHERIT is set?

Errr, another line I forgot to remove.

As long as you're going to introduce new API, there is no need to modify 
old structure.

So my idea is:
1) Introduce new API
2) Add btrfs_info/btrfs_debug for old API
    And make it default to disallow creating level 0 qgroups, and auto
    remove level 0 qgroups when creating new subvolume.
3) That's all

Thanks,
Qu

> 
>>>
>>> I have two issues with this:
>>> 1) Existing software, like Docker Daemon. There's no way I can tell
>>> Docker daemon to use this new flag to create qgroups. Although, I
>>> could modify it, it doesn't fit into the API very well.
>>> 2) How do I do this with qgroups that already exist, or qgroups that
>>> are created on-demand by qgroup_inherit?
>>>
>>> I think both of these would be fixed if we copied (a subset) of the
>>> qgroup flags. If you look at my other patches, I think we want to
>>> remove the ability to remove level-0 qgroups (which are inherited), so
>>> we'd also want to add an ioctl to be able to modify (some of those)
>>> flags. Do you think that adding inheritance, and a new ioctl is a
>>> reasonable approach?
>>
>>
>> Sorry, I didn't see later patches.
>>
>> Yes, new API is a better method to solve this problem.
>>
>> But with new APIs, the need of new mount option is even less.
>> Outputting a warning message in dmesg is good enough to info user to use new
>> API.
>>
>> Thanks,
>> Qu
>>
>>
>>>
>>>>
>>>> It's a u64 value while we only checks if it's zero or not, to determine
>>>> if
>>>> it's creation or deletion.
>>>>
>>>> We could reuse it to extent the create/delete behavior, other than a new
>>>> mount option, which is a global flag, just to alter qgroup behavior.
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>>
>>>>>     #ifdef CONFIG_BTRFS_DEBUG
>>>>>           Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
>>>>>     #endif
>>>>> @@ -381,6 +381,8 @@ static const match_table_t tokens = {
>>>>>           {Opt_rescan_uuid_tree, "rescan_uuid_tree"},
>>>>>           {Opt_fatal_errors, "fatal_errors=%s"},
>>>>>           {Opt_commit_interval, "commit=%d"},
>>>>> +       {Opt_qgroup_keep, "qgroup_keep"},
>>>>> +       {Opt_qgroup_nokeep, "qgroup_nokeep"},
>>>>>     #ifdef CONFIG_BTRFS_DEBUG
>>>>>           {Opt_fragment_data, "fragment=data"},
>>>>>           {Opt_fragment_metadata, "fragment=metadata"},
>>>>> @@ -808,6 +810,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info,
>>>>> char *options,
>>>>>                                   info->commit_interval =
>>>>> BTRFS_DEFAULT_COMMIT_INTERVAL;
>>>>>                           }
>>>>>                           break;
>>>>> +               case Opt_qgroup_keep:
>>>>> +                       btrfs_set_and_info(info, QGROUP_KEEP,
>>>>> +                                          "do not automatically delete
>>>>> qgroups");
>>>>> +                       break;
>>>>> +               case Opt_qgroup_nokeep:
>>>>> +                       btrfs_clear_and_info(info, QGROUP_KEEP,
>>>>> +                                            "automatically delete
>>>>> qgroups");
>>>>> +                       break;
>>>>>     #ifdef CONFIG_BTRFS_DEBUG
>>>>>                   case Opt_fragment_all:
>>>>>                           btrfs_info(info, "fragmenting all space");
>>>>> @@ -1299,6 +1309,10 @@ static int btrfs_show_options(struct seq_file
>>>>> *seq,
>>>>> struct dentry *dentry)
>>>>>                   seq_puts(seq, ",fatal_errors=panic");
>>>>>           if (info->commit_interval != BTRFS_DEFAULT_COMMIT_INTERVAL)
>>>>>                   seq_printf(seq, ",commit=%d", info->commit_interval);
>>>>> +       if (btrfs_test_opt(info, QGROUP_KEEP))
>>>>> +               seq_puts(seq, ",qgroup_keep");
>>>>> +       else
>>>>> +               seq_puts(seq, ",qgroup_nokeep");
>>>>>     #ifdef CONFIG_BTRFS_DEBUG
>>>>>           if (btrfs_test_opt(info, FRAGMENT_DATA))
>>>>>                   seq_puts(seq, ",fragment=data");
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 

[PATCH 5/8] btrfs: qgroup.h whitespace change

[Sargun Dhillon]

This patch is just changing whitespace alignment in qgroup.h

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/qgroup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fe04d3f..fb6c7da 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -127,7 +127,7 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-			      struct btrfs_fs_info *fs_info, u64 qgroupid);
+			struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 qgroupid,
 		       struct btrfs_qgroup_limit *limit);
-- 
2.9.3

Re: [PATCH 5/8] btrfs: qgroup.h whitespace change

[David Sterba]

On Sat, May 20, 2017 at 08:39:37AM +0000, Sargun Dhillon wrote:
> This patch is just changing whitespace alignment in qgroup.h

>  int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
> -			      struct btrfs_fs_info *fs_info, u64 qgroupid);
> +			struct btrfs_fs_info *fs_info, u64 qgroupid);

Please don't send such changes.

https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start

https://btrfs.wiki.kernel.org/index.php/Project_ideas#Cleanup_projects

"Please note that pure whitespace and style reformatting changes are not
really necessary at this phase of development. They get fixed along
regular changes. Possibly once upon in a while a patch that fixes many
if not all whitespace errors could work, but otherwise it's considered a
noise. "

[PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists

[Sargun Dhillon]

This patch is to prepare for following patches in this patchset. The
purpose is to make it so that we can prevent accidental removal of
qgroups that are actively in use.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/ioctl.c  |  4 ++--
 fs/btrfs/qgroup.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/qgroup.h |  3 ++-
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index b10d7bb..2b1a8c1 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2554,7 +2554,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	 */
 	if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
 		ret = btrfs_remove_qgroup(trans, fs_info,
-					  dest->root_key.objectid);
+					  dest->root_key.objectid, 0);
 		if (ret && ret != -ENOENT)
 			pr_info("Could not automatically delete qgroup: %d\n", ret);
 	}
@@ -4974,7 +4974,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 	if (sa->create) {
 		ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
 	} else {
-		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid);
+		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
 	}
 
 	err = btrfs_end_transaction(trans);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 588248b..a0699fd 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1247,6 +1247,45 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
+/*
+ * Meant to only operate on level-0 qroupid.
+ *
+ * It returns 1 if a matching subvolume is found; 0 if none is found.
+ * < 0 if there is an error.
+ */
+static int btrfs_subvolume_exists(struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+	struct btrfs_path *path;
+	struct btrfs_key key;
+	int err, ret = 0;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	key.objectid = qgroupid;
+	key.type = BTRFS_ROOT_BACKREF_KEY;
+	key.offset = 0;
+
+	err = btrfs_search_slot_for_read(fs_info->tree_root, &key, path, 1, 0);
+	if (err == 1)
+		goto out;
+
+	if (err) {
+		ret = err;
+		goto out;
+	}
+
+	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+	if (key.objectid != qgroupid || key.type != BTRFS_ROOT_BACKREF_KEY)
+		goto out;
+
+	ret = 1;
+out:
+	btrfs_free_path(path);
+	return ret;
+}
+
 /* Must be called with qgroup_ioctl_lock held */
 static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 				 struct btrfs_fs_info *fs_info, u64 qgroupid)
@@ -1333,10 +1372,19 @@ static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid)
+			struct btrfs_fs_info *fs_info, u64 qgroupid,
+			int check_in_use)
 {
 	int ret;
 
+	if (check_in_use && btrfs_qgroup_level(qgroupid) == 0) {
+		ret = btrfs_subvolume_exists(fs_info, qgroupid);
+		if (ret < 0)
+			return ret;
+		if (ret)
+			return -EBUSY;
+	}
+
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fb6c7da..fc08bdb 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -127,7 +127,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info, u64 qgroupid);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid);
+			struct btrfs_fs_info *fs_info, u64 qgroupid,
+			int check_in_use);
 int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 qgroupid,
 		       struct btrfs_qgroup_limit *limit);
-- 
2.9.3

Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists

[Qu Wenruo]

At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
> This patch is to prepare for following patches in this patchset. The
> purpose is to make it so that we can prevent accidental removal of
> qgroups that are actively in use.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>   fs/btrfs/ioctl.c  |  4 ++--
>   fs/btrfs/qgroup.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   fs/btrfs/qgroup.h |  3 ++-
>   3 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b10d7bb..2b1a8c1 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2554,7 +2554,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>   	 */
>   	if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>   		ret = btrfs_remove_qgroup(trans, fs_info,
> -					  dest->root_key.objectid);
> +					  dest->root_key.objectid, 0);
>   		if (ret && ret != -ENOENT)
>   			pr_info("Could not automatically delete qgroup: %d\n", ret);
>   	}
> @@ -4974,7 +4974,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>   	if (sa->create) {
>   		ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
>   	} else {
> -		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid);
> +		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
>   	}
>   
>   	err = btrfs_end_transaction(trans);
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 588248b..a0699fd 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1247,6 +1247,45 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
>   	return ret;
>   }
>   
> +/*
> + * Meant to only operate on level-0 qroupid.
> + *
> + * It returns 1 if a matching subvolume is found; 0 if none is found.
> + * < 0 if there is an error.
> + */
> +static int btrfs_subvolume_exists(struct btrfs_fs_info *fs_info, u64 qgroupid)
> +{
> +	struct btrfs_path *path;
> +	struct btrfs_key key;
> +	int err, ret = 0;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	key.objectid = qgroupid;
> +	key.type = BTRFS_ROOT_BACKREF_KEY;

Fs root (subvolume id equals to 5) has no ROOT_BACKREF key.
Such search would make fs tree always treated as not existed.

Thanks,
Qu
> +	key.offset = 0;
> +
> +	err = btrfs_search_slot_for_read(fs_info->tree_root, &key, path, 1, 0);
> +	if (err == 1)
> +		goto out;
> +
> +	if (err) {
> +		ret = err;
> +		goto out;
> +	}
> +
> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +	if (key.objectid != qgroupid || key.type != BTRFS_ROOT_BACKREF_KEY)
> +		goto out;
> +
> +	ret = 1;
> +out:
> +	btrfs_free_path(path);
> +	return ret;
> +}
> +
>   /* Must be called with qgroup_ioctl_lock held */
>   static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>   				 struct btrfs_fs_info *fs_info, u64 qgroupid)
> @@ -1333,10 +1372,19 @@ static int __btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>   }
>   
>   int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info, u64 qgroupid)
> +			struct btrfs_fs_info *fs_info, u64 qgroupid,
> +			int check_in_use)
>   {
>   	int ret;
>   
> +	if (check_in_use && btrfs_qgroup_level(qgroupid) == 0) {
> +		ret = btrfs_subvolume_exists(fs_info, qgroupid);
> +		if (ret < 0)
> +			return ret;
> +		if (ret)
> +			return -EBUSY;
> +	}
> +
>   	mutex_lock(&fs_info->qgroup_ioctl_lock);
>   	ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
>   	mutex_unlock(&fs_info->qgroup_ioctl_lock);
> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
> index fb6c7da..fc08bdb 100644
> --- a/fs/btrfs/qgroup.h
> +++ b/fs/btrfs/qgroup.h
> @@ -127,7 +127,8 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
>   int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>   			struct btrfs_fs_info *fs_info, u64 qgroupid);
>   int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
> -			struct btrfs_fs_info *fs_info, u64 qgroupid);
> +			struct btrfs_fs_info *fs_info, u64 qgroupid,
> +			int check_in_use);
>   int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
>   		       struct btrfs_fs_info *fs_info, u64 qgroupid,
>   		       struct btrfs_qgroup_limit *limit);
> 

Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists

[Sargun Dhillon]

On Sun, May 21, 2017 at 6:39 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>
>
> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>
>> This patch is to prepare for following patches in this patchset. The
>> purpose is to make it so that we can prevent accidental removal of
>> qgroups that are actively in use.
>>
>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> ---
>>   fs/btrfs/ioctl.c  |  4 ++--
>>   fs/btrfs/qgroup.c | 50
>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   fs/btrfs/qgroup.h |  3 ++-
>>   3 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index b10d7bb..2b1a8c1 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2554,7 +2554,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct
>> file *file,
>>          */
>>         if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>>                 ret = btrfs_remove_qgroup(trans, fs_info,
>> -                                         dest->root_key.objectid);
>> +                                         dest->root_key.objectid, 0);
>>                 if (ret && ret != -ENOENT)
>>                         pr_info("Could not automatically delete qgroup:
>> %d\n", ret);
>>         }
>> @@ -4974,7 +4974,7 @@ static long btrfs_ioctl_qgroup_create(struct file
>> *file, void __user *arg)
>>         if (sa->create) {
>>                 ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
>>         } else {
>> -               ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid);
>> +               ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid,
>> 0);
>>         }
>>         err = btrfs_end_transaction(trans);
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 588248b..a0699fd 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1247,6 +1247,45 @@ int btrfs_del_qgroup_relation(struct
>> btrfs_trans_handle *trans,
>>         return ret;
>>   }
>>   +/*
>> + * Meant to only operate on level-0 qroupid.
>> + *
>> + * It returns 1 if a matching subvolume is found; 0 if none is found.
>> + * < 0 if there is an error.
>> + */
>> +static int btrfs_subvolume_exists(struct btrfs_fs_info *fs_info, u64
>> qgroupid)
>> +{
>> +       struct btrfs_path *path;
>> +       struct btrfs_key key;
>> +       int err, ret = 0;
>> +
>> +       path = btrfs_alloc_path();
>> +       if (!path)
>> +               return -ENOMEM;
>> +
>> +       key.objectid = qgroupid;
>> +       key.type = BTRFS_ROOT_BACKREF_KEY;
>
>
> Fs root (subvolume id equals to 5) has no ROOT_BACKREF key.
> Such search would make fs tree always treated as not existed.
Is it okay to just explicitly exempt 5, or is there a better way to do
this search?

>
> Thanks,
> Qu
>
>> +       key.offset = 0;
>> +
>> +       err = btrfs_search_slot_for_read(fs_info->tree_root, &key, path,
>> 1, 0);
>> +       if (err == 1)
>> +               goto out;
>> +
>> +       if (err) {
>> +               ret = err;
>> +               goto out;
>> +       }
>> +
>> +       btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +       if (key.objectid != qgroupid || key.type !=
>> BTRFS_ROOT_BACKREF_KEY)
>> +               goto out;
>> +
>> +       ret = 1;
>> +out:
>> +       btrfs_free_path(path);
>> +       return ret;
>> +}
>> +
>>   /* Must be called with qgroup_ioctl_lock held */
>>   static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>>                                  struct btrfs_fs_info *fs_info, u64
>> qgroupid)
>> @@ -1333,10 +1372,19 @@ static int __btrfs_remove_qgroup(struct
>> btrfs_trans_handle *trans,
>>   }
>>     int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>> -                       struct btrfs_fs_info *fs_info, u64 qgroupid)
>> +                       struct btrfs_fs_info *fs_info, u64 qgroupid,
>> +                       int check_in_use)
>>   {
>>         int ret;
>>   +     if (check_in_use && btrfs_qgroup_level(qgroupid) == 0) {
>> +               ret = btrfs_subvolume_exists(fs_info, qgroupid);
>> +               if (ret < 0)
>> +                       return ret;
>> +               if (ret)
>> +                       return -EBUSY;
>> +       }
>> +
>>         mutex_lock(&fs_info->qgroup_ioctl_lock);
>>         ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
>>         mutex_unlock(&fs_info->qgroup_ioctl_lock);
>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>> index fb6c7da..fc08bdb 100644
>> --- a/fs/btrfs/qgroup.h
>> +++ b/fs/btrfs/qgroup.h
>> @@ -127,7 +127,8 @@ int btrfs_del_qgroup_relation(struct
>> btrfs_trans_handle *trans,
>>   int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>>                         struct btrfs_fs_info *fs_info, u64 qgroupid);
>>   int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>> -                       struct btrfs_fs_info *fs_info, u64 qgroupid);
>> +                       struct btrfs_fs_info *fs_info, u64 qgroupid,
>> +                       int check_in_use);
>>   int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
>>                        struct btrfs_fs_info *fs_info, u64 qgroupid,
>>                        struct btrfs_qgroup_limit *limit);
>>
>
>

Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists

[Qu Wenruo]

At 05/22/2017 11:04 AM, Sargun Dhillon wrote:
> On Sun, May 21, 2017 at 6:39 PM, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>>
>>
>> At 05/20/2017 04:39 PM, Sargun Dhillon wrote:
>>>
>>> This patch is to prepare for following patches in this patchset. The
>>> purpose is to make it so that we can prevent accidental removal of
>>> qgroups that are actively in use.
>>>
>>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>>> ---
>>>    fs/btrfs/ioctl.c  |  4 ++--
>>>    fs/btrfs/qgroup.c | 50
>>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>    fs/btrfs/qgroup.h |  3 ++-
>>>    3 files changed, 53 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>>> index b10d7bb..2b1a8c1 100644
>>> --- a/fs/btrfs/ioctl.c
>>> +++ b/fs/btrfs/ioctl.c
>>> @@ -2554,7 +2554,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct
>>> file *file,
>>>           */
>>>          if (!btrfs_test_opt(fs_info, QGROUP_KEEP)) {
>>>                  ret = btrfs_remove_qgroup(trans, fs_info,
>>> -                                         dest->root_key.objectid);
>>> +                                         dest->root_key.objectid, 0);
>>>                  if (ret && ret != -ENOENT)
>>>                          pr_info("Could not automatically delete qgroup:
>>> %d\n", ret);
>>>          }
>>> @@ -4974,7 +4974,7 @@ static long btrfs_ioctl_qgroup_create(struct file
>>> *file, void __user *arg)
>>>          if (sa->create) {
>>>                  ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
>>>          } else {
>>> -               ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid);
>>> +               ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid,
>>> 0);
>>>          }
>>>          err = btrfs_end_transaction(trans);
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 588248b..a0699fd 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1247,6 +1247,45 @@ int btrfs_del_qgroup_relation(struct
>>> btrfs_trans_handle *trans,
>>>          return ret;
>>>    }
>>>    +/*
>>> + * Meant to only operate on level-0 qroupid.
>>> + *
>>> + * It returns 1 if a matching subvolume is found; 0 if none is found.
>>> + * < 0 if there is an error.
>>> + */
>>> +static int btrfs_subvolume_exists(struct btrfs_fs_info *fs_info, u64
>>> qgroupid)
>>> +{
>>> +       struct btrfs_path *path;
>>> +       struct btrfs_key key;
>>> +       int err, ret = 0;
>>> +
>>> +       path = btrfs_alloc_path();
>>> +       if (!path)
>>> +               return -ENOMEM;
>>> +
>>> +       key.objectid = qgroupid;
>>> +       key.type = BTRFS_ROOT_BACKREF_KEY;
>>
>>
>> Fs root (subvolume id equals to 5) has no ROOT_BACKREF key.
>> Such search would make fs tree always treated as not existed.
> Is it okay to just explicitly exempt 5, or is there a better way to do
> this search?

5(BTRFS_FS_TREE_OBJECTID) is an exception.

Other part seems OK.

Thanks,
Qu
> 
>>
>> Thanks,
>> Qu
>>
>>> +       key.offset = 0;
>>> +
>>> +       err = btrfs_search_slot_for_read(fs_info->tree_root, &key, path,
>>> 1, 0);
>>> +       if (err == 1)
>>> +               goto out;
>>> +
>>> +       if (err) {
>>> +               ret = err;
>>> +               goto out;
>>> +       }
>>> +
>>> +       btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +       if (key.objectid != qgroupid || key.type !=
>>> BTRFS_ROOT_BACKREF_KEY)
>>> +               goto out;
>>> +
>>> +       ret = 1;
>>> +out:
>>> +       btrfs_free_path(path);
>>> +       return ret;
>>> +}
>>> +
>>>    /* Must be called with qgroup_ioctl_lock held */
>>>    static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>>>                                   struct btrfs_fs_info *fs_info, u64
>>> qgroupid)
>>> @@ -1333,10 +1372,19 @@ static int __btrfs_remove_qgroup(struct
>>> btrfs_trans_handle *trans,
>>>    }
>>>      int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>>> -                       struct btrfs_fs_info *fs_info, u64 qgroupid)
>>> +                       struct btrfs_fs_info *fs_info, u64 qgroupid,
>>> +                       int check_in_use)
>>>    {
>>>          int ret;
>>>    +     if (check_in_use && btrfs_qgroup_level(qgroupid) == 0) {
>>> +               ret = btrfs_subvolume_exists(fs_info, qgroupid);
>>> +               if (ret < 0)
>>> +                       return ret;
>>> +               if (ret)
>>> +                       return -EBUSY;
>>> +       }
>>> +
>>>          mutex_lock(&fs_info->qgroup_ioctl_lock);
>>>          ret = __btrfs_remove_qgroup(trans, fs_info, qgroupid);
>>>          mutex_unlock(&fs_info->qgroup_ioctl_lock);
>>> diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
>>> index fb6c7da..fc08bdb 100644
>>> --- a/fs/btrfs/qgroup.h
>>> +++ b/fs/btrfs/qgroup.h
>>> @@ -127,7 +127,8 @@ int btrfs_del_qgroup_relation(struct
>>> btrfs_trans_handle *trans,
>>>    int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
>>>                          struct btrfs_fs_info *fs_info, u64 qgroupid);
>>>    int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
>>> -                       struct btrfs_fs_info *fs_info, u64 qgroupid);
>>> +                       struct btrfs_fs_info *fs_info, u64 qgroupid,
>>> +                       int check_in_use);
>>>    int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
>>>                         struct btrfs_fs_info *fs_info, u64 qgroupid,
>>>                         struct btrfs_qgroup_limit *limit);
>>>
>>
>>
> --
> 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
> 
> 

[PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol

[Sargun Dhillon]

This patch is to prepare for following patches in this patchset. The code
allows you to check if a subvol exists, and to only allow the creation
of qgroups on subvols that already exist. It doesn't make sense to allow
the creation of level 0 qgroups otherwise.

The behaviour is to inherit (create) a qgroup when you create a new
subvol with quota on. If there is an existing qgroup with this same
ID, it will be reset.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/ioctl.c  |  2 +-
 fs/btrfs/qgroup.c | 11 ++++++++++-
 fs/btrfs/qgroup.h |  3 ++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2b1a8c1..5e20643 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4972,7 +4972,7 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 
 	/* FIXME: check if the IDs really exist */
 	if (sa->create) {
-		ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid);
+		ret = btrfs_create_qgroup(trans, fs_info, sa->qgroupid, 0);
 	} else {
 		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
 	}
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index a0699fd..28e2c7f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1317,10 +1317,19 @@ static int __btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 }
 
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid)
+			struct btrfs_fs_info *fs_info, u64 qgroupid,
+			int check_subvol_exists)
 {
 	int ret;
 
+	if (check_subvol_exists && btrfs_qgroup_level(qgroupid) == 0) {
+		ret = btrfs_subvolume_exists(fs_info, qgroupid);
+		if (ret < 0)
+			return ret;
+		if (!ret)
+			return -ENOENT;
+	}
+
 	mutex_lock(&fs_info->qgroup_ioctl_lock);
 	ret = __btrfs_create_qgroup(trans, fs_info, qgroupid);
 	mutex_unlock(&fs_info->qgroup_ioctl_lock);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fc08bdb..1afbe40 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -125,7 +125,8 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans,
 int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans,
 			      struct btrfs_fs_info *fs_info, u64 src, u64 dst);
 int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
-			struct btrfs_fs_info *fs_info, u64 qgroupid);
+			struct btrfs_fs_info *fs_info, u64 qgroupid,
+			int check_subvol_exists);
 int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 			struct btrfs_fs_info *fs_info, u64 qgroupid,
 			int check_in_use);
-- 
2.9.3

Re: [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

Hi Sargun,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.12-rc2 next-20170522]
[cannot apply to btrfs/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sargun-Dhillon/BtrFS-QGroups-uapi-improvements/20170523-111746
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   fs/btrfs/tests/qgroup-tests.c: In function 'test_no_shared_qgroup':
>> fs/btrfs/tests/qgroup-tests.c:232:8: error: too few arguments to function 'btrfs_create_qgroup'
     ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FS_TREE_OBJECTID);
           ^
   In file included from fs/btrfs/tests/qgroup-tests.c:24:0:
   fs/btrfs/tests/../qgroup.h:127:5: note: declared here
    int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
        ^
   fs/btrfs/tests/qgroup-tests.c: In function 'test_multiple_refs':
   fs/btrfs/tests/qgroup-tests.c:334:8: error: too few arguments to function 'btrfs_create_qgroup'
     ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FIRST_FREE_OBJECTID);
           ^
   In file included from fs/btrfs/tests/qgroup-tests.c:24:0:
   fs/btrfs/tests/../qgroup.h:127:5: note: declared here
    int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
        ^

vim +/btrfs_create_qgroup +232 fs/btrfs/tests/qgroup-tests.c

442244c9 Qu Wenruo     2015-04-16  226  	struct ulist *new_roots = NULL;
faa2dbf0 Josef Bacik   2014-05-07  227  	int ret;
faa2dbf0 Josef Bacik   2014-05-07  228  
7c55ee0c Omar Sandoval 2015-09-29  229  	btrfs_init_dummy_trans(&trans);
faa2dbf0 Josef Bacik   2014-05-07  230  
faa2dbf0 Josef Bacik   2014-05-07  231  	test_msg("Qgroup basic add\n");
ef9f2db3 Feifei Xu     2016-06-01 @232  	ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FS_TREE_OBJECTID);
faa2dbf0 Josef Bacik   2014-05-07  233  	if (ret) {
faa2dbf0 Josef Bacik   2014-05-07  234  		test_msg("Couldn't create a qgroup %d\n", ret);
faa2dbf0 Josef Bacik   2014-05-07  235  		return ret;

:::::: The code at line 232 was first introduced by commit
:::::: ef9f2db365c31433e52b0c5863793273bb632666 Btrfs: self-tests: Use macros instead of constants and add missing newline

:::::: TO: Feifei Xu <xufeifei@linux.vnet.ibm.com>
:::::: CC: David Sterba <dsterba@suse.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50141 bytes --]

Re: [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol

[kbuild test robot]

Hi Sargun,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.12-rc2 next-20170522]
[cannot apply to btrfs/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sargun-Dhillon/BtrFS-QGroups-uapi-improvements/20170523-111746
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   include/linux/compiler.h:264:8: sparse: attribute 'no_sanitize_address': unknown attribute
>> fs/btrfs/tests/qgroup-tests.c:232:34: sparse: not enough arguments for function btrfs_create_qgroup
   fs/btrfs/tests/qgroup-tests.c:334:34: sparse: not enough arguments for function btrfs_create_qgroup
   fs/btrfs/tests/qgroup-tests.c: In function 'test_no_shared_qgroup':
   fs/btrfs/tests/qgroup-tests.c:232:8: error: too few arguments to function 'btrfs_create_qgroup'
     ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FS_TREE_OBJECTID);
           ^~~~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/tests/qgroup-tests.c:24:0:
   fs/btrfs/tests/../qgroup.h:127:5: note: declared here
    int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
        ^~~~~~~~~~~~~~~~~~~
   fs/btrfs/tests/qgroup-tests.c: In function 'test_multiple_refs':
   fs/btrfs/tests/qgroup-tests.c:334:8: error: too few arguments to function 'btrfs_create_qgroup'
     ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FIRST_FREE_OBJECTID);
           ^~~~~~~~~~~~~~~~~~~
   In file included from fs/btrfs/tests/qgroup-tests.c:24:0:
   fs/btrfs/tests/../qgroup.h:127:5: note: declared here
    int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
        ^~~~~~~~~~~~~~~~~~~

vim +232 fs/btrfs/tests/qgroup-tests.c

faa2dbf0 Josef Bacik        2014-05-07  216  	btrfs_free_path(path);
faa2dbf0 Josef Bacik        2014-05-07  217  	return ret;
faa2dbf0 Josef Bacik        2014-05-07  218  }
faa2dbf0 Josef Bacik        2014-05-07  219  
b9ef22de Feifei Xu          2016-06-01  220  static int test_no_shared_qgroup(struct btrfs_root *root,
b9ef22de Feifei Xu          2016-06-01  221  		u32 sectorsize, u32 nodesize)
faa2dbf0 Josef Bacik        2014-05-07  222  {
faa2dbf0 Josef Bacik        2014-05-07  223  	struct btrfs_trans_handle trans;
faa2dbf0 Josef Bacik        2014-05-07  224  	struct btrfs_fs_info *fs_info = root->fs_info;
442244c9 Qu Wenruo          2015-04-16  225  	struct ulist *old_roots = NULL;
442244c9 Qu Wenruo          2015-04-16  226  	struct ulist *new_roots = NULL;
faa2dbf0 Josef Bacik        2014-05-07  227  	int ret;
faa2dbf0 Josef Bacik        2014-05-07  228  
7c55ee0c Omar Sandoval      2015-09-29  229  	btrfs_init_dummy_trans(&trans);
faa2dbf0 Josef Bacik        2014-05-07  230  
faa2dbf0 Josef Bacik        2014-05-07  231  	test_msg("Qgroup basic add\n");
ef9f2db3 Feifei Xu          2016-06-01 @232  	ret = btrfs_create_qgroup(NULL, fs_info, BTRFS_FS_TREE_OBJECTID);
faa2dbf0 Josef Bacik        2014-05-07  233  	if (ret) {
faa2dbf0 Josef Bacik        2014-05-07  234  		test_msg("Couldn't create a qgroup %d\n", ret);
faa2dbf0 Josef Bacik        2014-05-07  235  		return ret;
faa2dbf0 Josef Bacik        2014-05-07  236  	}
faa2dbf0 Josef Bacik        2014-05-07  237  
442244c9 Qu Wenruo          2015-04-16  238  	/*
01327610 Nicholas D Steeves 2016-05-19  239  	 * Since the test trans doesn't have the complicated delayed refs,
442244c9 Qu Wenruo          2015-04-16  240  	 * we can only call btrfs_qgroup_account_extent() directly to test

:::::: The code at line 232 was first introduced by commit
:::::: ef9f2db365c31433e52b0c5863793273bb632666 Btrfs: self-tests: Use macros instead of constants and add missing newline

:::::: TO: Feifei Xu <xufeifei@linux.vnet.ibm.com>
:::::: CC: David Sterba <dsterba@suse.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal

[Sargun Dhillon]

This patch ties together the work in the previous patches, to introduce
semantics around the qgroup creation / removal API that are a bit more
intuitive that the current one. It also creates two new args structures
which have reserved space for future expansion, as opposed to single
datastructure for both creation and removal.

The associated semantics are as follows:
1) You cannot create a level 0 qgroup for a subvol which does not exist
   unless you pass in an override flag.
2) You cannot delete a level 0 qgroup which refers to a subvolume, which
   currently exists unless you pass in an override flag.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/ioctl.c           | 98 ++++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/btrfs.h | 23 +++++++++++
 2 files changed, 121 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5e20643..7bf34b7 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4936,6 +4936,100 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
 	return ret;
 }
 
+static long btrfs_ioctl_qgroup_create_v2(struct file *file, void __user *arg)
+{
+	struct btrfs_ioctl_qgroup_create_args_v2 create_args;
+	struct inode *inode = file_inode(file);
+	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info;
+	struct btrfs_root *root;
+	int check_subvol_exists;
+	int ret, err;
+
+	fs_info = btrfs_sb(inode->i_sb);
+	root = BTRFS_I(inode)->root;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = copy_from_user(&create_args, arg, sizeof(create_args));
+	if (ret)
+		return ret;
+
+	if (!create_args.qgroupid)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	trans = btrfs_join_transaction(root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
+
+	check_subvol_exists = !(create_args.flags &
+				BTRFS_QGROUP_CREATE_IGNORE_UNUSED);
+	ret = btrfs_create_qgroup(trans, fs_info, create_args.qgroupid,
+				  check_subvol_exists);
+
+	err = btrfs_end_transaction(trans);
+	if (err && !ret)
+		ret = err;
+
+out:
+	mnt_drop_write_file(file);
+	return ret;
+}
+
+static long btrfs_ioctl_qgroup_remove_v2(struct file *file, void __user *arg)
+{
+	struct btrfs_ioctl_qgroup_remove_args_v2 remove_args;
+	struct inode *inode = file_inode(file);
+	struct btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info;
+	struct btrfs_root *root;
+	int check_in_use;
+	int ret, err;
+
+	fs_info = btrfs_sb(inode->i_sb);
+	root = BTRFS_I(inode)->root;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	ret = copy_from_user(&remove_args, arg, sizeof(remove_args));
+	if (ret)
+		return ret;
+
+	if (!remove_args.qgroupid)
+		return -EINVAL;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	trans = btrfs_join_transaction(root);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out;
+	}
+
+	check_in_use = !(remove_args.flags &
+			 BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE);
+	ret = btrfs_remove_qgroup(trans, fs_info, remove_args.qgroupid,
+				  check_in_use);
+
+	err = btrfs_end_transaction(trans);
+	if (err && !ret)
+		ret = err;
+
+out:
+	mnt_drop_write_file(file);
+	return ret;
+}
+
 static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 {
 	struct inode *inode = file_inode(file);
@@ -5626,6 +5720,10 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_qgroup_assign(file, argp);
 	case BTRFS_IOC_QGROUP_CREATE:
 		return btrfs_ioctl_qgroup_create(file, argp);
+	case BTRFS_IOC_QGROUP_CREATE_V2:
+		return btrfs_ioctl_qgroup_create_v2(file, argp);
+	case BTRFS_IOC_QGROUP_REMOVE_V2:
+		return btrfs_ioctl_qgroup_remove_v2(file, argp);
 	case BTRFS_IOC_QGROUP_LIMIT:
 		return btrfs_ioctl_qgroup_limit(file, argp);
 	case BTRFS_IOC_QUOTA_RESCAN:
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index a456e53..0a6652d 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -653,6 +653,25 @@ struct btrfs_ioctl_qgroup_create_args {
 	__u64 create;
 	__u64 qgroupid;
 };
+
+/* Allow the user to delete qgroups even if there isn't a subvol with the id */
+#define	BTRFS_QGROUP_CREATE_IGNORE_UNUSED		(1ULL << 0)
+
+struct btrfs_ioctl_qgroup_create_args_v2 {
+	__u64 qgroupid;
+	__u64 flags;
+	__u64 reserved[16];
+};
+
+/* Allow the user to delete qgroups even if they are referenced by a subvol */
+#define	BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE		(1ULL << 0)
+
+struct btrfs_ioctl_qgroup_remove_args_v2 {
+	__u64 qgroupid;
+	__u64 flags;
+	__u64 reserved[16];
+};
+
 struct btrfs_ioctl_timespec {
 	__u64 sec;
 	__u32 nsec;
@@ -818,5 +837,9 @@ enum btrfs_err_code {
 				   struct btrfs_ioctl_feature_flags[3])
 #define BTRFS_IOC_RM_DEV_V2 _IOW(BTRFS_IOCTL_MAGIC, 58, \
 				   struct btrfs_ioctl_vol_args_v2)
+#define BTRFS_IOC_QGROUP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 59, \
+			       struct btrfs_ioctl_qgroup_create_args_v2)
+#define BTRFS_IOC_QGROUP_REMOVE_V2 _IOW(BTRFS_IOCTL_MAGIC, 60, \
+			       struct btrfs_ioctl_qgroup_remove_args_v2)
 
 #endif /* _UAPI_LINUX_BTRFS_H */
-- 
2.9.3

Re: [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal

[Qu Wenruo]

At 05/20/2017 04:40 PM, Sargun Dhillon wrote:
> This patch ties together the work in the previous patches, to introduce
> semantics around the qgroup creation / removal API that are a bit more
> intuitive that the current one. It also creates two new args structures
> which have reserved space for future expansion, as opposed to single
> datastructure for both creation and removal.
> 
> The associated semantics are as follows:
> 1) You cannot create a level 0 qgroup for a subvol which does not exist
>     unless you pass in an override flag.
> 2) You cannot delete a level 0 qgroup which refers to a subvolume, which
>     currently exists unless you pass in an override flag.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>   fs/btrfs/ioctl.c           | 98 ++++++++++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/btrfs.h | 23 +++++++++++
>   2 files changed, 121 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 5e20643..7bf34b7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4936,6 +4936,100 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg)
>   	return ret;
>   }
>   
> +static long btrfs_ioctl_qgroup_create_v2(struct file *file, void __user *arg)
> +{
> +	struct btrfs_ioctl_qgroup_create_args_v2 create_args;
> +	struct inode *inode = file_inode(file);
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_fs_info *fs_info;
> +	struct btrfs_root *root;
> +	int check_subvol_exists;
> +	int ret, err;
> +
> +	fs_info = btrfs_sb(inode->i_sb);
> +	root = BTRFS_I(inode)->root;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ret = copy_from_user(&create_args, arg, sizeof(create_args));
> +	if (ret)
> +		return ret;
> +
> +	if (!create_args.qgroupid)
> +		return -EINVAL;
> +
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		return ret;
> +
> +	trans = btrfs_join_transaction(root);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
> +	check_subvol_exists = !(create_args.flags &
> +				BTRFS_QGROUP_CREATE_IGNORE_UNUSED);
> +	ret = btrfs_create_qgroup(trans, fs_info, create_args.qgroupid,
> +				  check_subvol_exists);
> +
> +	err = btrfs_end_transaction(trans);
> +	if (err && !ret)
> +		ret = err;
> +
> +out:
> +	mnt_drop_write_file(file);
> +	return ret;
> +}
> +
> +static long btrfs_ioctl_qgroup_remove_v2(struct file *file, void __user *arg)
> +{
> +	struct btrfs_ioctl_qgroup_remove_args_v2 remove_args;
> +	struct inode *inode = file_inode(file);
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_fs_info *fs_info;
> +	struct btrfs_root *root;
> +	int check_in_use;
> +	int ret, err;
> +
> +	fs_info = btrfs_sb(inode->i_sb);
> +	root = BTRFS_I(inode)->root;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	ret = copy_from_user(&remove_args, arg, sizeof(remove_args));
> +	if (ret)
> +		return ret;
> +
> +	if (!remove_args.qgroupid)
> +		return -EINVAL;
> +
> +	ret = mnt_want_write_file(file);
> +	if (ret)
> +		return ret;
> +
> +	trans = btrfs_join_transaction(root);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto out;
> +	}
> +
> +	check_in_use = !(remove_args.flags &
> +			 BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE);
> +	ret = btrfs_remove_qgroup(trans, fs_info, remove_args.qgroupid,
> +				  check_in_use);
> +
> +	err = btrfs_end_transaction(trans);
> +	if (err && !ret)
> +		ret = err;
> +
> +out:
> +	mnt_drop_write_file(file);
> +	return ret;
> +}
> +
>   static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>   {
>   	struct inode *inode = file_inode(file);
> @@ -5626,6 +5720,10 @@ long btrfs_ioctl(struct file *file, unsigned int
>   		return btrfs_ioctl_qgroup_assign(file, argp);
>   	case BTRFS_IOC_QGROUP_CREATE:
>   		return btrfs_ioctl_qgroup_create(file, argp);
> +	case BTRFS_IOC_QGROUP_CREATE_V2:
> +		return btrfs_ioctl_qgroup_create_v2(file, argp);
> +	case BTRFS_IOC_QGROUP_REMOVE_V2:
> +		return btrfs_ioctl_qgroup_remove_v2(file, argp);
>   	case BTRFS_IOC_QGROUP_LIMIT:
>   		return btrfs_ioctl_qgroup_limit(file, argp);
>   	case BTRFS_IOC_QUOTA_RESCAN:
> diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
> index a456e53..0a6652d 100644
> --- a/include/uapi/linux/btrfs.h
> +++ b/include/uapi/linux/btrfs.h
> @@ -653,6 +653,25 @@ struct btrfs_ioctl_qgroup_create_args {
>   	__u64 create;
>   	__u64 qgroupid;
>   };
> +
> +/* Allow the user to delete qgroups even if there isn't a subvol with the id */
> +#define	BTRFS_QGROUP_CREATE_IGNORE_UNUSED		(1ULL << 0)
> +
> +struct btrfs_ioctl_qgroup_create_args_v2 {
> +	__u64 qgroupid;
> +	__u64 flags;
> +	__u64 reserved[16];
> +};

New APIs are much better than original one.
Splitting creation and deletion, so it won't cause any confusion, and 
with reserved members for us to extend it later.

The only concern is the flags. It's quite easy to confuse the 
BTRFS_QGROUP_CREATE_IGNORE_UNUSED and 
BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE, as they are the same value.

What about sharing the flags between create and remove args?



While with that IGNORE_UNUSED flags, why do we still need the mount 
option method?

IIRC, just introducing new APIs is good enough.

> +
> +/* Allow the user to delete qgroups even if they are referenced by a subvol */
> +#define	BTRFS_QGROUP_REMOVE_NO_CHECK_IN_USE		(1ULL << 0)
> +
> +struct btrfs_ioctl_qgroup_remove_args_v2 {
> +	__u64 qgroupid;
> +	__u64 flags;
> +	__u64 reserved[16];
> +};
> +
>   struct btrfs_ioctl_timespec {
>   	__u64 sec;
>   	__u32 nsec;
> @@ -818,5 +837,9 @@ enum btrfs_err_code {
>   				   struct btrfs_ioctl_feature_flags[3])
>   #define BTRFS_IOC_RM_DEV_V2 _IOW(BTRFS_IOCTL_MAGIC, 58, \
>   				   struct btrfs_ioctl_vol_args_v2)
> +#define BTRFS_IOC_QGROUP_CREATE_V2 _IOW(BTRFS_IOCTL_MAGIC, 59, \
> +			       struct btrfs_ioctl_qgroup_create_args_v2)
> +#define BTRFS_IOC_QGROUP_REMOVE_V2 _IOW(BTRFS_IOCTL_MAGIC, 60, \
> +			       struct btrfs_ioctl_qgroup_remove_args_v2)

IIRC we need acknowledgement from other developers to determine the 
ioctl number.

Thanks,
Qu

>   
>   #endif /* _UAPI_LINUX_BTRFS_H */
>