[PATCH v2 0/4] Qgroup uapi improvements

[PATCH v2 0/4] Qgroup uapi improvements

[Sargun Dhillon]

This patchset has several improvements around the qgroups user-facing
API. It introduces two new ioctls for creating, and removing qgroups.
These ioctls have a new args structure that allows passing flags, and
some reserved fields for future expansion. The ioctls prevent some
operations around level-0 qgroups as well.

The create operation prevents the creation of level-0 qgroups for
subvolumes that do not exist. The delete operations prevents the
deletion of level-0 qgroups that reference active volumes.

In adddition, it adds a mount option "qgroup_auto_cleanup".
When this mount option is specified, qgroups will automatically
be cleaned up at volume deletion time. The reason this is a mount
option over a default behaviour is to avoid breaking old scripts
that rely on the behaviour of the existing APIs. Users can opt
into the new behaviour, as opposed it being an automated
trigger on newly created qgroups. Later on, we can introduce
a flag to subvol / qgroup create that marks the qgroup as
automatically created, and delete the qgroup as automatically
as well.

Changes since v1:
  * Remove creation of level-0 qgroups without subvol
  * Add deprecation message for old API

Sargun Dhillon (4):
  btrfs: Fail on removing qgroup if del_qgroup_item fails
  btrfs: Add new ioctl uapis for qgroup creation / removal
  btrfs: Warn the user when the legacy btrfs_qgroup_create API is used
  btrfs: Add qgroup_auto_cleanup mount flag to automatically cleanup
    qgroups

 fs/btrfs/ctree.h           |   1 +
 fs/btrfs/ioctl.c           | 117 ++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/qgroup.c          |  79 ++++++++++++++++++++++++++++--
 fs/btrfs/qgroup.h          |   6 ++-
 fs/btrfs/super.c           |  15 +++++-
 include/uapi/linux/btrfs.h |  22 +++++++++
 6 files changed, 230 insertions(+), 10 deletions(-)

-- 
2.9.3

[PATCH v2 1/4] 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 if the error is fatal,
(any error other than qgroup not found).

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

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index deffbeb..d39ffcc 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1309,6 +1309,9 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	}
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
 
+	if (ret && ret != -ENOENT)
+		goto out;
+
 	while (!list_empty(&qgroup->groups)) {
 		list = list_first_entry(&qgroup->groups,
 					struct btrfs_qgroup_list, next_group);
-- 
2.9.3

[PATCH v2 2/4] btrfs: Add new ioctl uapis for qgroup creation / removal

[Sargun Dhillon]

This patch introduces two new ioctls to create, and remove qgroups. These
offer a somewhat more intuitive set of operations with the opportunity
to add flags that gate to unintentional manipulation of qgroups.

The create qgroup ioctl has a a new semantic which level-0 qgroups cannot
be created unless a matching subvolume ID is created.

The delete qgroup ioctl has the new semantic that it will not let you
delete a currently utilized (referenced by a subvolume) level-0
qgroup unless you pass the BTRFS_QGROUP_NO_SUBVOL_CHECK flag.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 fs/btrfs/ioctl.c           | 100 ++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/qgroup.c          |  76 +++++++++++++++++++++++++++++++---
 fs/btrfs/qgroup.h          |   6 ++-
 include/uapi/linux/btrfs.h |  22 ++++++++++
 4 files changed, 195 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e176375..78c8321 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4922,6 +4922,98 @@ 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 btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info;
+	struct btrfs_root *root;
+	struct inode *inode;
+	int ret;
+	int err;
+
+	inode = file_inode(file);
+	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;
+	}
+
+	ret = btrfs_create_qgroup(trans, fs_info, create_args.qgroupid, 1);
+	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 btrfs_trans_handle *trans;
+	struct btrfs_fs_info *fs_info;
+	struct btrfs_root *root;
+	struct inode *inode;
+	int check;
+	int ret;
+	int err;
+
+	inode = file_inode(file);
+	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;
+	check = !(remove_args.flags & BTRFS_QGROUP_NO_SUBVOL_CHECK);
+
+	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;
+	}
+
+	ret = btrfs_remove_qgroup(trans, fs_info, remove_args.qgroupid, check);
+	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);
@@ -4958,9 +5050,9 @@ 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);
+		ret = btrfs_remove_qgroup(trans, fs_info, sa->qgroupid, 0);
 	}
 
 	err = btrfs_end_transaction(trans);
@@ -5632,6 +5724,10 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_get_features(file, argp);
 	case BTRFS_IOC_SET_FEATURES:
 		return btrfs_ioctl_set_features(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);
 	}
 
 	return -ENOTTY;
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index d39ffcc..ba8a6ee 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1247,8 +1247,51 @@ 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;
+
+	if (qgroupid == BTRFS_FS_TREE_OBJECTID)
+		return 1;
+
+	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;
+}
+
 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)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
@@ -1260,12 +1303,23 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans,
 		ret = -EINVAL;
 		goto out;
 	}
+
 	qgroup = find_qgroup_rb(fs_info, qgroupid);
 	if (qgroup) {
 		ret = -EEXIST;
 		goto out;
 	}
 
+	if (check_subvol_exists && btrfs_qgroup_level(qgroupid) == 0) {
+		ret = btrfs_subvolume_exists(fs_info, qgroupid);
+		if (ret < 0)
+			goto out;
+		if (!ret) {
+			ret = -ENOENT;
+			goto out;
+		}
+	}
+
 	ret = add_qgroup_item(trans, quota_root, qgroupid);
 	if (ret)
 		goto out;
@@ -1282,7 +1336,8 @@ int btrfs_create_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_subvol_exists)
 {
 	struct btrfs_root *quota_root;
 	struct btrfs_qgroup *qgroup;
@@ -1300,13 +1355,24 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans,
 	if (!qgroup) {
 		ret = -ENOENT;
 		goto out;
-	} else {
-		/* check if there are no children of this qgroup */
-		if (!list_empty(&qgroup->members)) {
+	}
+
+	if (check_subvol_exists && btrfs_qgroup_level(qgroupid) == 0) {
+		ret = btrfs_subvolume_exists(fs_info, qgroupid);
+		if (ret < 0)
+			goto out;
+		if (ret) {
 			ret = -EBUSY;
 			goto out;
 		}
 	}
+
+	/* check if there are no children of this qgroup */
+	if (!list_empty(&qgroup->members)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	ret = del_qgroup_item(trans, quota_root, qgroupid);
 
 	if (ret && ret != -ENOENT)
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index fe04d3f..8702d74 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -125,9 +125,11 @@ 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);
+			struct btrfs_fs_info *fs_info, u64 qgroupid,
+			int check_subvol_exists);
 int btrfs_limit_qgroup(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 qgroupid,
 		       struct btrfs_qgroup_limit *limit);
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index a456e53..c073854 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -653,6 +653,24 @@ struct btrfs_ioctl_qgroup_create_args {
 	__u64 create;
 	__u64 qgroupid;
 };
+
+struct btrfs_ioctl_qgroup_create_args_v2 {
+	__u64 qgroupid;
+	__u64 flags;
+	__u64 reserved[16];
+};
+
+/*
+ * Allows for the deletion of qgroups even if they are associated with active
+ * volumes.
+ */
+#define BTRFS_QGROUP_NO_SUBVOL_CHECK	(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 +836,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

[PATCH v2 3/4] btrfs: Warn the user when the legacy btrfs_qgroup_create API is used

[Sargun Dhillon]

This patch adds a warning to let the user know when the legacy
qgroup creation / removal API is in use. Eventually, we can
deprecate this API.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 78c8321..fba409f 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -5027,6 +5027,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	pr_info_once("btrfs: Usage of deprecated btrfs_qgroup_create ioctl\n");
+
 	ret = mnt_want_write_file(file);
 	if (ret)
 		return ret;
-- 
2.9.3

[PATCH v2 4/4] btrfs: Add qgroup_auto_cleanup mount flag to automatically cleanup qgroups

[Sargun Dhillon]

This patch introduces a new mount option - qgroup_auto_cleanup.
The purpose of this mount option is to cause btrfs to automatically
delete qgroups on subvolume deletion. This only cleans up the
associated level-0 qgroup, and not qgroups that are above it.

Since this behaviour is API-breaking, as people may already have scripts
that do this cleanup on subvolume destruction, this feature, at least
for now, must be opt-in.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 643c70d..0300f6f 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_AUTO_CLEANUP	(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 fba409f..04e4022 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2543,6 +2543,21 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 			goto out_end_trans;
 		}
 	}
+	/*
+	 * 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, therefore we do
+	 * not abort the transaction if this fails, nor return an error.
+	 */
+	if (btrfs_test_opt(fs_info, QGROUP_AUTO_CLEANUP)) {
+		ret = btrfs_remove_qgroup(trans, fs_info,
+					  dest->root_key.objectid, 0);
+		if (ret && ret != -ENOENT)
+			btrfs_warn(fs_info,
+				   "Failed to cleanup qgroup. err: %d", ret);
+	}
 
 out_end_trans:
 	trans->block_rsv = NULL;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4f1cdd5..557f53f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -321,7 +321,8 @@ 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_auto_cleanup,
+	Opt_no_qgroup_auto_cleanup,
 #ifdef CONFIG_BTRFS_DEBUG
 	Opt_fragment_data, Opt_fragment_metadata, Opt_fragment_all,
 #endif
@@ -381,6 +382,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_auto_cleanup, "qgroup_auto_cleanup"},
+	{Opt_no_qgroup_auto_cleanup, "no_qgroup_auto_cleanup"},
 #ifdef CONFIG_BTRFS_DEBUG
 	{Opt_fragment_data, "fragment=data"},
 	{Opt_fragment_metadata, "fragment=metadata"},
@@ -808,6 +811,14 @@ int btrfs_parse_options(struct btrfs_fs_info *info, char *options,
 				info->commit_interval = BTRFS_DEFAULT_COMMIT_INTERVAL;
 			}
 			break;
+		case Opt_qgroup_auto_cleanup:
+			 btrfs_set_and_info(info, QGROUP_AUTO_CLEANUP,
+					    "enabling qgroup auto cleanup");
+			break;
+		case Opt_no_qgroup_auto_cleanup:
+			btrfs_clear_and_info(info, QGROUP_AUTO_CLEANUP,
+					     "disabling qgroup auto cleanup");
+			break;
 #ifdef CONFIG_BTRFS_DEBUG
 		case Opt_fragment_all:
 			btrfs_info(info, "fragmenting all space");
@@ -1299,6 +1310,8 @@ 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_AUTO_CLEANUP))
+		seq_puts(seq, ",qgroup_auto_cleanup");
 #ifdef CONFIG_BTRFS_DEBUG
 	if (btrfs_test_opt(info, FRAGMENT_DATA))
 		seq_puts(seq, ",fragment=data");
-- 
2.9.3

Re: [PATCH v2 0/4] Qgroup uapi improvements

[Sargun Dhillon]

On Fri, May 26, 2017 at 1:44 PM, Sargun Dhillon <sargun@sargun.me> wrote:
> This patchset has several improvements around the qgroups user-facing
> API. It introduces two new ioctls for creating, and removing qgroups.
> These ioctls have a new args structure that allows passing flags, and
> some reserved fields for future expansion. The ioctls prevent some
> operations around level-0 qgroups as well.
>
> The create operation prevents the creation of level-0 qgroups for
> subvolumes that do not exist. The delete operations prevents the
> deletion of level-0 qgroups that reference active volumes.
>
> In adddition, it adds a mount option "qgroup_auto_cleanup".
> When this mount option is specified, qgroups will automatically
> be cleaned up at volume deletion time. The reason this is a mount
> option over a default behaviour is to avoid breaking old scripts
> that rely on the behaviour of the existing APIs. Users can opt
> into the new behaviour, as opposed it being an automated
> trigger on newly created qgroups. Later on, we can introduce
> a flag to subvol / qgroup create that marks the qgroup as
> automatically created, and delete the qgroup as automatically
> as well.
>
> Changes since v1:
>   * Remove creation of level-0 qgroups without subvol
>   * Add deprecation message for old API
>
> Sargun Dhillon (4):
>   btrfs: Fail on removing qgroup if del_qgroup_item fails
>   btrfs: Add new ioctl uapis for qgroup creation / removal
>   btrfs: Warn the user when the legacy btrfs_qgroup_create API is used
>   btrfs: Add qgroup_auto_cleanup mount flag to automatically cleanup
>     qgroups
>
>  fs/btrfs/ctree.h           |   1 +
>  fs/btrfs/ioctl.c           | 117 ++++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/qgroup.c          |  79 ++++++++++++++++++++++++++++--
>  fs/btrfs/qgroup.h          |   6 ++-
>  fs/btrfs/super.c           |  15 +++++-
>  include/uapi/linux/btrfs.h |  22 +++++++++
>  6 files changed, 230 insertions(+), 10 deletions(-)
>
> --
> 2.9.3
>

Hey,
I wanted to see what people thought of this. This was the only way I
saw forward that wouldn't break existing scripts, and would allow
users to opt-in to this new behaviour. If y'all think that different
semantics make sense, let me know, and I'll rev this patchset.

Re: [PATCH v2 4/4] btrfs: Add qgroup_auto_cleanup mount flag to automatically cleanup qgroups

[David Sterba]

On Fri, May 26, 2017 at 08:45:08PM +0000, Sargun Dhillon wrote:
> This patch introduces a new mount option - qgroup_auto_cleanup.
> The purpose of this mount option is to cause btrfs to automatically
> delete qgroups on subvolume deletion. This only cleans up the
> associated level-0 qgroup, and not qgroups that are above it.
> 
> Since this behaviour is API-breaking, as people may already have scripts
> that do this cleanup on subvolume destruction, this feature, at least
> for now, must be opt-in.

I'd rather make the auto-cleaning default and I don't remember why this
wasn't actually done back then.

I don't like mount options for fine grained tuning of a filesystem, IMO
it's almost always the bad interface for that. But people propose
various mout options because it's easy to implement and test (known
interface, existing tools). Ok for testing and first draft but as it's
an interface that's supposed to be forever, it's good to take time to
think it through.

>From the API-breaking point you wrote above, sounds like switching
auto-cleaning on by default is not a good option.

The mount option is somehow persistent, ie. stored in a config file and
applied on mount. I was thinking about a sysfs tunable, this would
require a post-mount action, less comfort. Another option is to
add a new property for the filesystem. That would need some more
preparatory work though but I'd prefer this approach.

Re: [PATCH v2 3/4] btrfs: Warn the user when the legacy btrfs_qgroup_create API is used

[David Sterba]

On Fri, May 26, 2017 at 08:44:50PM +0000, Sargun Dhillon wrote:
> This patch adds a warning to let the user know when the legacy
> qgroup creation / removal API is in use. Eventually, we can
> deprecate this API.
> 
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>  fs/btrfs/ioctl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 78c8321..fba409f 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5027,6 +5027,8 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
>  
> +	pr_info_once("btrfs: Usage of deprecated btrfs_qgroup_create ioctl\n");

Would be better to use the btrfs_info helper otherwise the user does not
know on which filesystem it happened.

Deprecation of v1 makes sense but the ioctl must stay anyway, similar to
other v1/v2 ioctls that we have. In long-term and long-living distros,
the applications probably use the deprecated interfaces so the syslog
message causes noise. I'd first document the deprecation and add a real
warning in a few releases maybe.

Re: [PATCH v2 2/4] btrfs: Add new ioctl uapis for qgroup creation / removal

[David Sterba]

On Fri, May 26, 2017 at 08:44:41PM +0000, Sargun Dhillon wrote:
> This patch introduces two new ioctls to create, and remove qgroups. These
> offer a somewhat more intuitive set of operations with the opportunity
> to add flags that gate to unintentional manipulation of qgroups.
> 
> The create qgroup ioctl has a a new semantic which level-0 qgroups cannot
> be created unless a matching subvolume ID is created.
> 
> The delete qgroup ioctl has the new semantic that it will not let you
> delete a currently utilized (referenced by a subvolume) level-0
> qgroup unless you pass the BTRFS_QGROUP_NO_SUBVOL_CHECK flag.

I'm fine with the qgroup ioctl revisions, if only to address the lack of
the flags or reserved fields. The updated semantics also justify a new
ioctl number.

On the implementation side, options are to do one ioctl per action or do
the "swiss army knife" style (multiple actions under on ioctl number).

Adding a special data structure for each ioctl is not necessary, as you
can see, btrfs_ioctl_qgroup_create_args_v2 and
btrfs_ioctl_qgroup_remove_args_v2 are the same. Although we could extend
them in different ways in the future, this could also happen if it were
just one structure and flags are set to denote meaning of whatever new
struct members would be added.

I'd go for something similar as the subvolume creation/deletion ioctls,
to have separate ioctl numbers assigned but use the same structure.

Similar to the mount options, new ioctls need some time to settle, but
merging this patchset in 4.13 seems still doable.