* [PATCH 1/8] btrfs: Split up btrfs_remove_qgroup, no logic changes
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
@ 2017-05-20 8:38 ` Sargun Dhillon
2017-05-20 8:39 ` [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails Sargun Dhillon
` (6 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:38 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
2017-05-20 8:38 ` [PATCH 1/8] btrfs: Split up btrfs_remove_qgroup, no logic changes Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-22 1:14 ` Qu Wenruo
2017-05-20 8:39 ` [PATCH 3/8] btrfs: Split up btrfs_create_qgroup, no logic changes Sargun Dhillon
` (5 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails
2017-05-20 8:39 ` [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails Sargun Dhillon
@ 2017-05-22 1:14 ` Qu Wenruo
2017-05-22 1:30 ` Sargun Dhillon
0 siblings, 1 reply; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 1:14 UTC (permalink / raw)
To: Sargun Dhillon, linux-btrfs
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,
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails
2017-05-22 1:14 ` Qu Wenruo
@ 2017-05-22 1:30 ` Sargun Dhillon
0 siblings, 0 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-22 1:30 UTC (permalink / raw)
To: Qu Wenruo; +Cc: BTRFS ML
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,
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/8] btrfs: Split up btrfs_create_qgroup, no logic changes
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
2017-05-20 8:38 ` [PATCH 1/8] btrfs: Split up btrfs_remove_qgroup, no logic changes Sargun Dhillon
2017-05-20 8:39 ` [PATCH 2/8] btrfs: Fail on removing qgroup if del_qgroup_item fails Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-20 8:39 ` [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override Sargun Dhillon
` (4 subsequent siblings)
7 siblings, 0 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
` (2 preceding siblings ...)
2017-05-20 8:39 ` [PATCH 3/8] btrfs: Split up btrfs_create_qgroup, no logic changes Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-20 19:32 ` Sargun Dhillon
2017-05-22 1:20 ` Qu Wenruo
2017-05-20 8:39 ` [PATCH 5/8] btrfs: qgroup.h whitespace change Sargun Dhillon
` (3 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-20 8:39 ` [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override Sargun Dhillon
@ 2017-05-20 19:32 ` Sargun Dhillon
2017-05-22 1:20 ` Qu Wenruo
1 sibling, 0 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 19:32 UTC (permalink / raw)
To: BTRFS ML
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
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-20 8:39 ` [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override Sargun Dhillon
2017-05-20 19:32 ` Sargun Dhillon
@ 2017-05-22 1:20 ` Qu Wenruo
2017-05-22 1:58 ` Sargun Dhillon
1 sibling, 1 reply; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 1:20 UTC (permalink / raw)
To: Sargun Dhillon, linux-btrfs
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");
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-22 1:20 ` Qu Wenruo
@ 2017-05-22 1:58 ` Sargun Dhillon
2017-05-22 2:03 ` Qu Wenruo
0 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-22 1:58 UTC (permalink / raw)
To: Qu Wenruo; +Cc: BTRFS ML
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");
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-22 1:58 ` Sargun Dhillon
@ 2017-05-22 2:03 ` Qu Wenruo
2017-05-22 17:31 ` Sargun Dhillon
0 siblings, 1 reply; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 2:03 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: BTRFS ML
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");
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-22 2:03 ` Qu Wenruo
@ 2017-05-22 17:31 ` Sargun Dhillon
2017-05-23 0:48 ` Qu Wenruo
0 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-22 17:31 UTC (permalink / raw)
To: Qu Wenruo; +Cc: BTRFS ML
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");
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override
2017-05-22 17:31 ` Sargun Dhillon
@ 2017-05-23 0:48 ` Qu Wenruo
0 siblings, 0 replies; 24+ messages in thread
From: Qu Wenruo @ 2017-05-23 0:48 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: BTRFS ML
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");
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/8] btrfs: qgroup.h whitespace change
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
` (3 preceding siblings ...)
2017-05-20 8:39 ` [PATCH 4/8] btrfs: autoremove qgroup by default, and add a mount flag to override Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-26 19:08 ` David Sterba
2017-05-20 8:39 ` [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists Sargun Dhillon
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
` (4 preceding siblings ...)
2017-05-20 8:39 ` [PATCH 5/8] btrfs: qgroup.h whitespace change Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-22 1:39 ` Qu Wenruo
2017-05-20 8:39 ` [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol Sargun Dhillon
2017-05-20 8:40 ` [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal Sargun Dhillon
7 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists
2017-05-20 8:39 ` [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists Sargun Dhillon
@ 2017-05-22 1:39 ` Qu Wenruo
2017-05-22 3:04 ` Sargun Dhillon
0 siblings, 1 reply; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 1:39 UTC (permalink / raw)
To: Sargun Dhillon, linux-btrfs
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);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists
2017-05-22 1:39 ` Qu Wenruo
@ 2017-05-22 3:04 ` Sargun Dhillon
2017-05-22 3:40 ` Qu Wenruo
0 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-22 3:04 UTC (permalink / raw)
To: Qu Wenruo; +Cc: BTRFS ML
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);
>>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists
2017-05-22 3:04 ` Sargun Dhillon
@ 2017-05-22 3:40 ` Qu Wenruo
0 siblings, 0 replies; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 3:40 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: BTRFS ML
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
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
` (5 preceding siblings ...)
2017-05-20 8:39 ` [PATCH 6/8] btrfs: Add code to check if a qgroup's subvol exists Sargun Dhillon
@ 2017-05-20 8:39 ` Sargun Dhillon
2017-05-23 4:54 ` kbuild test robot
2017-05-23 6:27 ` kbuild test robot
2017-05-20 8:40 ` [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal Sargun Dhillon
7 siblings, 2 replies; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:39 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol
2017-05-20 8:39 ` [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol Sargun Dhillon
@ 2017-05-23 4:54 ` kbuild test robot
2017-05-23 6:27 ` kbuild test robot
1 sibling, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2017-05-23 4:54 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: kbuild-all, linux-btrfs
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol
2017-05-20 8:39 ` [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol Sargun Dhillon
2017-05-23 4:54 ` kbuild test robot
@ 2017-05-23 6:27 ` kbuild test robot
1 sibling, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2017-05-23 6:27 UTC (permalink / raw)
To: Sargun Dhillon; +Cc: kbuild-all, linux-btrfs
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal
2017-05-20 8:38 [PATCH 0/8] BtrFS: QGroups uapi improvements Sargun Dhillon
` (6 preceding siblings ...)
2017-05-20 8:39 ` [PATCH 7/8] btrfs: Add code to prevent qgroup creation for a non-existent subvol Sargun Dhillon
@ 2017-05-20 8:40 ` Sargun Dhillon
2017-05-22 1:51 ` Qu Wenruo
7 siblings, 1 reply; 24+ messages in thread
From: Sargun Dhillon @ 2017-05-20 8:40 UTC (permalink / raw)
To: linux-btrfs
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal
2017-05-20 8:40 ` [PATCH 8/8] btrfs: Add new ioctl uapis for qgroup creation / removal Sargun Dhillon
@ 2017-05-22 1:51 ` Qu Wenruo
0 siblings, 0 replies; 24+ messages in thread
From: Qu Wenruo @ 2017-05-22 1:51 UTC (permalink / raw)
To: Sargun Dhillon, linux-btrfs
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 */
>
^ permalink raw reply [flat|nested] 24+ messages in thread