* [PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume.
@ 2015-02-10 10:24 Dongsheng Yang
2015-02-10 10:24 ` [PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's Dongsheng Yang
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Dongsheng Yang @ 2015-02-10 10:24 UTC (permalink / raw)
To: linux-btrfs; +Cc: Dongsheng Yang
Hi all,
This is a part3 for qgroup, it is based on
Btrfs: qgroup: part-2: bug fixes.
Dongsheng Yang (3):
btrfs: qgroup: return EINVAL if level of parent is not higher than
child's.
btrfs: qgroup: allow to remove qgroup which has parent but no child.
btrfs: qgroup: fix a wrong parameter of no_quota.
fs/btrfs/extent-tree.c | 8 ++++++--
fs/btrfs/qgroup.c | 35 ++++++++++++++++++++++++++++++-----
2 files changed, 36 insertions(+), 7 deletions(-)
--
1.8.4.2
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's. 2015-02-10 10:24 [PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume Dongsheng Yang @ 2015-02-10 10:24 ` Dongsheng Yang 2015-02-10 10:24 ` [PATCH 2/3] btrfs: qgroup: allow to remove qgroup which has parent but no child Dongsheng Yang 2015-02-10 10:24 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 2 siblings, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-02-10 10:24 UTC (permalink / raw) To: linux-btrfs; +Cc: Dongsheng Yang When we create a subvol inheriting a qgroup, we need to check the level of them. Otherwise, there is a chance a qgroup can inherit another qgroup at the same level. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- fs/btrfs/qgroup.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 5a1463d..d519055 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -2244,6 +2244,11 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, ret = -EINVAL; goto out; } + + if ((srcgroup->qgroupid >> 48) <= (objectid >> 48)) { + ret = -EINVAL; + goto out; + } ++i_qgroups; } } -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] btrfs: qgroup: allow to remove qgroup which has parent but no child. 2015-02-10 10:24 [PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume Dongsheng Yang 2015-02-10 10:24 ` [PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's Dongsheng Yang @ 2015-02-10 10:24 ` Dongsheng Yang 2015-02-10 10:24 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 2 siblings, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-02-10 10:24 UTC (permalink / raw) To: linux-btrfs; +Cc: Dongsheng Yang When a qgroup has parents but no child, it should be removable in Theory I think. But currently, we can not remove it when it has either parent or child. Example: # btrfs quota enable /mnt # btrfs qgroup create 1/0 /mnt # btrfs qgroup create 2/0 /mnt # btrfs qgroup assign 1/0 2/0 /mnt # btrfs qgroup show -pcre /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16384 16384 0 0 --- --- 1/0 0 0 0 0 2/0 --- 2/0 0 0 0 0 --- 1/0 At this time, there is no subvol or qgroup depending on it. Just a qgroup 2/0 is its parent, but 2/0 can work well without 1/0. So I think 1/0 should be removalbe. But: # btrfs qgroup destroy 1/0 /mnt ERROR: unable to destroy quota group: Device or resource busy This patch remove the check of qgroup->parent in removing it, then we can remove a qgroup when it has a parent. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- fs/btrfs/qgroup.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index d519055..28b0aa5 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1062,7 +1062,7 @@ out: return ret; } -int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, +int __del_qgroup_relation(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, u64 src, u64 dst) { struct btrfs_root *quota_root; @@ -1072,7 +1072,6 @@ int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, int ret = 0; int err; - mutex_lock(&fs_info->qgroup_ioctl_lock); quota_root = fs_info->quota_root; if (!quota_root) { ret = -EINVAL; @@ -1103,7 +1102,18 @@ exist: del_relation_rb(fs_info, src, dst); spin_unlock(&fs_info->qgroup_lock); out: + return ret; +} + +int btrfs_del_qgroup_relation(struct btrfs_trans_handle *trans, + struct btrfs_fs_info *fs_info, u64 src, u64 dst) +{ + int ret = 0; + + mutex_lock(&fs_info->qgroup_ioctl_lock); + ret = __del_qgroup_relation(trans, fs_info, src, dst); mutex_unlock(&fs_info->qgroup_ioctl_lock); + return ret; } @@ -1146,6 +1156,7 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, { struct btrfs_root *quota_root; struct btrfs_qgroup *qgroup; + struct btrfs_qgroup_list *list; int ret = 0; mutex_lock(&fs_info->qgroup_ioctl_lock); @@ -1160,15 +1171,24 @@ int btrfs_remove_qgroup(struct btrfs_trans_handle *trans, ret = -ENOENT; goto out; } else { - /* check if there are no relations to this qgroup */ - if (!list_empty(&qgroup->groups) || - !list_empty(&qgroup->members)) { + /* 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); + 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); + if (ret) + goto out; + } + spin_lock(&fs_info->qgroup_lock); del_qgroup_rb(quota_root->fs_info, qgroupid); spin_unlock(&fs_info->qgroup_lock); -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-10 10:24 [PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume Dongsheng Yang 2015-02-10 10:24 ` [PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's Dongsheng Yang 2015-02-10 10:24 ` [PATCH 2/3] btrfs: qgroup: allow to remove qgroup which has parent but no child Dongsheng Yang @ 2015-02-10 10:24 ` Dongsheng Yang 2015-02-10 11:24 ` Filipe David Manana 2015-02-13 9:38 ` Dongsheng Yang 2 siblings, 2 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-02-10 10:24 UTC (permalink / raw) To: linux-btrfs; +Cc: Dongsheng Yang In function of __btrfs_mod_ref(), we are passing the no_quota=1 to process_func() anyway. It will make the numbers in qgroup be wrong when deleting a subvolume. The data size in a deleted subvolume will never be decressed from all related qgroups. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- fs/btrfs/extent-tree.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 652be74..f59809c 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, int i; int level; int ret = 0; + int no_quota = 0; int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *, u64, u64, u64, u64, u64, u64, int); @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, else parent = 0; + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) + no_quota = 1; + for (i = 0; i < nritems; i++) { if (level == 0) { btrfs_item_key_to_cpu(buf, &key, i); @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, key.offset -= btrfs_file_extent_offset(buf, fi); ret = process_func(trans, root, bytenr, num_bytes, parent, ref_root, key.objectid, - key.offset, 1); + key.offset, no_quota); if (ret) goto fail; } else { @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, num_bytes = root->nodesize; ret = process_func(trans, root, bytenr, num_bytes, parent, ref_root, level - 1, 0, - 1); + no_quota); if (ret) goto fail; } -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-10 10:24 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang @ 2015-02-10 11:24 ` Filipe David Manana 2015-02-11 2:51 ` Dongsheng Yang 2015-02-13 9:38 ` Dongsheng Yang 1 sibling, 1 reply; 19+ messages in thread From: Filipe David Manana @ 2015-02-10 11:24 UTC (permalink / raw) To: Dongsheng Yang; +Cc: linux-btrfs@vger.kernel.org On Tue, Feb 10, 2015 at 10:24 AM, Dongsheng Yang <yangds.fnst@cn.fujitsu.com> wrote: > In function of __btrfs_mod_ref(), we are passing the no_quota=1 > to process_func() anyway. It will make the numbers in qgroup be > wrong when deleting a subvolume. The data size in a deleted subvolume > will never be decressed from all related qgroups. How about getting a test case for xfstests for this? thanks > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > fs/btrfs/extent-tree.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 652be74..f59809c 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > int i; > int level; > int ret = 0; > + int no_quota = 0; > int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *, > u64, u64, u64, u64, u64, u64, int); > > @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > else > parent = 0; > > + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) > + no_quota = 1; > + > for (i = 0; i < nritems; i++) { > if (level == 0) { > btrfs_item_key_to_cpu(buf, &key, i); > @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > key.offset -= btrfs_file_extent_offset(buf, fi); > ret = process_func(trans, root, bytenr, num_bytes, > parent, ref_root, key.objectid, > - key.offset, 1); > + key.offset, no_quota); > if (ret) > goto fail; > } else { > @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > num_bytes = root->nodesize; > ret = process_func(trans, root, bytenr, num_bytes, > parent, ref_root, level - 1, 0, > - 1); > + no_quota); > if (ret) > goto fail; > } > -- > 1.8.4.2 > > -- > 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 -- Filipe David Manana, "Reasonable men adapt themselves to the world. Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men." ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-10 11:24 ` Filipe David Manana @ 2015-02-11 2:51 ` Dongsheng Yang 0 siblings, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-02-11 2:51 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs@vger.kernel.org On 02/10/2015 07:24 PM, Filipe David Manana wrote: > On Tue, Feb 10, 2015 at 10:24 AM, Dongsheng Yang > <yangds.fnst@cn.fujitsu.com> wrote: >> In function of __btrfs_mod_ref(), we are passing the no_quota=1 >> to process_func() anyway. It will make the numbers in qgroup be >> wrong when deleting a subvolume. The data size in a deleted subvolume >> will never be decressed from all related qgroups. > How about getting a test case for xfstests for this? Good idea!! Will do it. Thanx > > thanks > >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> fs/btrfs/extent-tree.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >> index 652be74..f59809c 100644 >> --- a/fs/btrfs/extent-tree.c >> +++ b/fs/btrfs/extent-tree.c >> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, >> int i; >> int level; >> int ret = 0; >> + int no_quota = 0; >> int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *, >> u64, u64, u64, u64, u64, u64, int); >> >> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, >> else >> parent = 0; >> >> + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) >> + no_quota = 1; >> + >> for (i = 0; i < nritems; i++) { >> if (level == 0) { >> btrfs_item_key_to_cpu(buf, &key, i); >> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, >> key.offset -= btrfs_file_extent_offset(buf, fi); >> ret = process_func(trans, root, bytenr, num_bytes, >> parent, ref_root, key.objectid, >> - key.offset, 1); >> + key.offset, no_quota); >> if (ret) >> goto fail; >> } else { >> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, >> num_bytes = root->nodesize; >> ret = process_func(trans, root, bytenr, num_bytes, >> parent, ref_root, level - 1, 0, >> - 1); >> + no_quota); >> if (ret) >> goto fail; >> } >> -- >> 1.8.4.2 >> >> -- >> 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] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-10 10:24 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 2015-02-10 11:24 ` Filipe David Manana @ 2015-02-13 9:38 ` Dongsheng Yang 2015-02-26 6:05 ` Dongsheng Yang 1 sibling, 1 reply; 19+ messages in thread From: Dongsheng Yang @ 2015-02-13 9:38 UTC (permalink / raw) To: Josef Bacik, mfasheh; +Cc: linux-btrfs Hi Josef and Mark, I saw the commit message of 1152651a: [btrfs: qgroup: account shared subtrees during snapshot delete] that SUBTREE operation was introduced to account the quota number in subvolume deleting. But, I found it does not work well currently： Create subvolume '/mnt/sub' + btrfs qgroup show -prce /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- 0/257 16.00KiB 16.00KiB 0.00B 0.00B --- --- + dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100 100+0 records in 100+0 records out 102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s + sync + btrfs qgroup show -prce --raw /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16384 16384 0 0 --- --- 0/257 118784 118784 0 0 --- --- + btrfs sub snap /mnt/sub /mnt/snap Create a snapshot of '/mnt/sub' in '/mnt/snap' + sync + btrfs qgroup show -prce --raw /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16384 16384 0 0 --- --- 0/257 118784 16384 0 0 --- --- 0/258 118784 16384 0 0 --- --- + btrfs sub delete /mnt/sub Delete subvolume (no-commit): '/mnt/sub' + sync + btrfs qgroup show -prce --raw /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16384 16384 0 0 --- --- 0/257 118784 16384 0 0 --- --- 0/258 118784 16384 0 0 --- --- --------------------------------------------------------------------------- <------- WAIT for cleaner [root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- 0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was not cleaned from qgroup. 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl in the snapshot qgroup is increased to 116KB by SUBTREE operation My question is: (1), SUBTREE is not working well. (2), why we need a rule-breaker in qgroup operations SUBTREE to do this work? We can do it via the delayed-ref routine like the other qgroup operations. with my patch here applied and 1152651a reverted, I got the result: qgroupid rfer excl max_rfer max_excl parent child -------- ---- ---- -------- -------- ------ ----- 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- 0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree block both are cleaned 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers in snapshot qgroup is correct. (1), quota is working well now in subvolume deleting. (2), all operations are inserted by delayed-ref. So, the conclusion seems like that: (1), commit 1152651a was introducing a SUBTREE operation to account the number in subvolume deleted. (2). SUBTREE is not working well. (3). we can solve the same problem in a better way without introducing a new operation of SUBTREE. To be honest, I am suspecting myself *very much*. I trust you guys and believe there must be something I am missing. Then I send this mail to ask your help. Thanx in advance. Yang On 02/10/2015 06:24 PM, Dongsheng Yang wrote: > In function of __btrfs_mod_ref(), we are passing the no_quota=1 > to process_func() anyway. It will make the numbers in qgroup be > wrong when deleting a subvolume. The data size in a deleted subvolume > will never be decressed from all related qgroups. > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > fs/btrfs/extent-tree.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 652be74..f59809c 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > int i; > int level; > int ret = 0; > + int no_quota = 0; > int (*process_func)(struct btrfs_trans_handle *, struct btrfs_root *, > u64, u64, u64, u64, u64, u64, int); > > @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > else > parent = 0; > > + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) > + no_quota = 1; > + > for (i = 0; i < nritems; i++) { > if (level == 0) { > btrfs_item_key_to_cpu(buf, &key, i); > @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > key.offset -= btrfs_file_extent_offset(buf, fi); > ret = process_func(trans, root, bytenr, num_bytes, > parent, ref_root, key.objectid, > - key.offset, 1); > + key.offset, no_quota); > if (ret) > goto fail; > } else { > @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, > num_bytes = root->nodesize; > ret = process_func(trans, root, bytenr, num_bytes, > parent, ref_root, level - 1, 0, > - 1); > + no_quota); > if (ret) > goto fail; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-13 9:38 ` Dongsheng Yang @ 2015-02-26 6:05 ` Dongsheng Yang 2015-03-03 11:13 ` [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol Dongsheng Yang 2015-03-03 11:18 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 0 siblings, 2 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-02-26 6:05 UTC (permalink / raw) To: Josef Bacik, mfasheh; +Cc: linux-btrfs Wait a minute, this patch seems not working well in accounting quota number when deleting data shared by different subvolumes. I will investigate more about it and send a V2 out. Thanx Yang On 02/13/2015 05:38 PM, Dongsheng Yang wrote:hen > Hi Josef and Mark, > > I saw the commit message of 1152651a: > > [btrfs: qgroup: account shared subtrees during snapshot delete] > > that SUBTREE operation was introduced to account the quota number > in subvolume deleting. > > But, I found it does not work well currently： > > Create subvolume '/mnt/sub' > + btrfs qgroup show -prce /mnt > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- > 0/257 16.00KiB 16.00KiB 0.00B 0.00B --- --- > + dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100 > 100+0 records in > 100+0 records out > 102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s > + sync > + btrfs qgroup show -prce --raw /mnt > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16384 16384 0 0 --- --- > 0/257 118784 118784 0 0 --- --- > + btrfs sub snap /mnt/sub /mnt/snap > Create a snapshot of '/mnt/sub' in '/mnt/snap' > + sync > + btrfs qgroup show -prce --raw /mnt > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16384 16384 0 0 --- --- > 0/257 118784 16384 0 0 --- --- > 0/258 118784 16384 0 0 --- --- > + btrfs sub delete /mnt/sub > Delete subvolume (no-commit): '/mnt/sub' > + sync > + btrfs qgroup show -prce --raw /mnt > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16384 16384 0 0 --- --- > 0/257 118784 16384 0 0 --- --- > 0/258 118784 16384 0 0 --- --- > > --------------------------------------------------------------------------- > <------- WAIT for cleaner > [root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- > 0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was > not cleaned from qgroup. > 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl in > the snapshot qgroup is increased to 116KB by SUBTREE operation > > My question is: > (1), SUBTREE is not working well. > (2), why we need a rule-breaker in qgroup operations SUBTREE to do > this work? > We can do it via the delayed-ref routine like the other qgroup > operations. > > with my patch here applied and 1152651a reverted, I got the result: > qgroupid rfer excl max_rfer max_excl parent child > -------- ---- ---- -------- -------- ------ ----- > 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- > 0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree block > both are cleaned > 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers in > snapshot qgroup is correct. > > (1), quota is working well now in subvolume deleting. > (2), all operations are inserted by delayed-ref. > > So, the conclusion seems like that: > (1), commit 1152651a was introducing a SUBTREE operation to account > the number in subvolume deleted. > (2). SUBTREE is not working well. > (3). we can solve the same problem in a better way without introducing > a new operation of SUBTREE. > > To be honest, I am suspecting myself *very much*. I trust you guys and > believe there must be something I am missing. > > Then I send this mail to ask your help. > > Thanx in advance. > Yang > > > On 02/10/2015 06:24 PM, Dongsheng Yang wrote: >> In function of __btrfs_mod_ref(), we are passing the no_quota=1 >> to process_func() anyway. It will make the numbers in qgroup be >> wrong when deleting a subvolume. The data size in a deleted subvolume >> will never be decressed from all related qgroups. >> >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> fs/btrfs/extent-tree.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >> index 652be74..f59809c 100644 >> --- a/fs/btrfs/extent-tree.c >> +++ b/fs/btrfs/extent-tree.c >> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct >> btrfs_trans_handle *trans, >> int i; >> int level; >> int ret = 0; >> + int no_quota = 0; >> int (*process_func)(struct btrfs_trans_handle *, struct >> btrfs_root *, >> u64, u64, u64, u64, u64, u64, int); >> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct >> btrfs_trans_handle *trans, >> else >> parent = 0; >> + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) >> + no_quota = 1; >> + >> for (i = 0; i < nritems; i++) { >> if (level == 0) { >> btrfs_item_key_to_cpu(buf, &key, i); >> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct >> btrfs_trans_handle *trans, >> key.offset -= btrfs_file_extent_offset(buf, fi); >> ret = process_func(trans, root, bytenr, num_bytes, >> parent, ref_root, key.objectid, >> - key.offset, 1); >> + key.offset, no_quota); >> if (ret) >> goto fail; >> } else { >> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct >> btrfs_trans_handle *trans, >> num_bytes = root->nodesize; >> ret = process_func(trans, root, bytenr, num_bytes, >> parent, ref_root, level - 1, 0, >> - 1); >> + no_quota); >> if (ret) >> goto fail; >> } > > -- > 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] 19+ messages in thread
* [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-02-26 6:05 ` Dongsheng Yang @ 2015-03-03 11:13 ` Dongsheng Yang 2015-03-06 5:06 ` Eryu Guan 2015-03-03 11:18 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 1 sibling, 1 reply; 19+ messages in thread From: Dongsheng Yang @ 2015-03-03 11:13 UTC (permalink / raw) To: fstests; +Cc: linux-btrfs, Dongsheng Yang This regression is introduced by two commits: e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- tests/btrfs/084 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/084.out | 3 ++ tests/btrfs/group | 1 + 3 files changed, 88 insertions(+) create mode 100755 tests/btrfs/084 create mode 100644 tests/btrfs/084.out diff --git a/tests/btrfs/084 b/tests/btrfs/084 new file mode 100755 index 0000000..7b7c7ac --- /dev/null +++ b/tests/btrfs/084 @@ -0,0 +1,84 @@ +#! /bin/bash +# FS QA Test No. 084 +# +# This is a case for the regression introduced by +# +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) +# +#----------------------------------------------------------------------- +# Copyright (c) 2015 Fujitsu. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_need_to_be_root +_supported_fs btrfs +_supported_os Linux +_require_scratch + +_scratch_mount + +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null + +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap + +_run_btrfs_util_prog quota enable $SCRATCH_MNT +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT + +units=`_btrfs_qgroup_units` +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` + +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap + +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT + +timeout=100 +# There is a background thread doing the clean work +for ((i=0; i<$timeout; i++)) +do + result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` + if (($orig_result != $result)) + then + break + fi + sleep 1 +done + +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}' + +# success, all done +status=0 +exit diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out new file mode 100644 index 0000000..7bf6dbf --- /dev/null +++ b/tests/btrfs/084.out @@ -0,0 +1,3 @@ +QA output created by 084 +24576 24576 +0 0 diff --git a/tests/btrfs/group b/tests/btrfs/group index fe82a9c..1be7392 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -86,3 +86,4 @@ 081 auto quick clone 082 auto quick remount 083 auto quick send +084 qgroup -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-03-03 11:13 ` [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol Dongsheng Yang @ 2015-03-06 5:06 ` Eryu Guan 2015-03-16 5:06 ` Dongsheng Yang 0 siblings, 1 reply; 19+ messages in thread From: Eryu Guan @ 2015-03-06 5:06 UTC (permalink / raw) To: Dongsheng Yang; +Cc: fstests, linux-btrfs On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote: > This regression is introduced by two commits: > > e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) > 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) I think you should add more description about the issue you're going to test. I see only failures like [root@dhcp-66-86-11 xfstests]# diff -u tests/btrfs/084.out /root/xfstests/results//btrfs/084.out.bad --- tests/btrfs/084.out 2015-03-06 12:47:02.319000000 +0800 +++ /root/xfstests/results//btrfs/084.out.bad 2015-03-06 12:48:03.707000000 +0800 @@ -1,3 +1,3 @@ QA output created by 084 24576 24576 -0 0 +8192 0 but from the test I don't know why "8192 0" is expected result not "0 0" > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > tests/btrfs/084 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/084.out | 3 ++ > tests/btrfs/group | 1 + > 3 files changed, 88 insertions(+) > create mode 100755 tests/btrfs/084 > create mode 100644 tests/btrfs/084.out > > diff --git a/tests/btrfs/084 b/tests/btrfs/084 > new file mode 100755 > index 0000000..7b7c7ac > --- /dev/null > +++ b/tests/btrfs/084 > @@ -0,0 +1,84 @@ > +#! /bin/bash > +# FS QA Test No. 084 > +# > +# This is a case for the regression introduced by > +# > +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) > +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2015 Fujitsu. All Rights Reserved. > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it would be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write the Free Software Foundation, > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > +#----------------------------------------------------------------------- > +# > + > +seq=`basename $0` > +seqres=$RESULT_DIR/$seq > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +_cleanup() > +{ > + cd / > + rm -f $tmp.* Better to use a tab not 4 spaces, maybe "new" should be updated too (in another patch) > +} > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# real QA test starts here > +_need_to_be_root > +_supported_fs btrfs > +_supported_os Linux > +_require_scratch > + > +_scratch_mount _scratch_mkfs first before you mount it. And you need to remove $seqres.full before test, since you called _run_btrfs_util_prog and it will dump commands and outputs to $seqres.full, the file will keep growing over runs if you don't remove it first. rm -f $seqres.full > + > +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null > + > +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap > + > +_run_btrfs_util_prog quota enable $SCRATCH_MNT > +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT > + > +units=`_btrfs_qgroup_units` > +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` > + > +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap > + > +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT > + > +timeout=100 > +# There is a background thread doing the clean work > +for ((i=0; i<$timeout; i++)) > +do > + result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` > + if (($orig_result != $result)) > + then > + break > + fi > + sleep 1 I'm not sure if we need 100 iterations and sleeping 1 sec after each iteration, that means we need 100s for a PASS run. > +done Please follow this code style, as Dave pointed before for ; do # code done if ; then # code fi > + > +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}' > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out > new file mode 100644 > index 0000000..7bf6dbf > --- /dev/null > +++ b/tests/btrfs/084.out > @@ -0,0 +1,3 @@ > +QA output created by 084 > +24576 24576 > +0 0 > diff --git a/tests/btrfs/group b/tests/btrfs/group > index fe82a9c..1be7392 100644 > --- a/tests/btrfs/group > +++ b/tests/btrfs/group > @@ -86,3 +86,4 @@ > 081 auto quick clone > 082 auto quick remount > 083 auto quick send > +084 qgroup Missed auto group? Thanks, Eryu ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-03-06 5:06 ` Eryu Guan @ 2015-03-16 5:06 ` Dongsheng Yang 2015-03-16 5:33 ` Eryu Guan 2015-03-16 5:58 ` [PATCH v2] " Dongsheng Yang 0 siblings, 2 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-03-16 5:06 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, linux-btrfs Hi Guan, sorry for the late. On 03/06/2015 01:06 PM, Eryu Guan wrote: > On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote: >> This regression is introduced by two commits: >> >> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) > I think you should add more description about the issue you're going to > test. I see only failures like > > [root@dhcp-66-86-11 xfstests]# diff -u tests/btrfs/084.out /root/xfstests/results//btrfs/084.out.bad > --- tests/btrfs/084.out 2015-03-06 12:47:02.319000000 +0800 > +++ /root/xfstests/results//btrfs/084.out.bad 2015-03-06 > 12:48:03.707000000 +0800 > @@ -1,3 +1,3 @@ > QA output created by 084 > 24576 24576 > -0 0 > +8192 0 > > but from the test I don't know why "8192 0" is expected result not "0 0" Sure, I will add more description in V2 to explain why we are expecting "0 0" here. > >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> tests/btrfs/084 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/btrfs/084.out | 3 ++ >> tests/btrfs/group | 1 + >> 3 files changed, 88 insertions(+) >> create mode 100755 tests/btrfs/084 >> create mode 100644 tests/btrfs/084.out >> >> diff --git a/tests/btrfs/084 b/tests/btrfs/084 >> new file mode 100755 >> index 0000000..7b7c7ac >> --- /dev/null >> +++ b/tests/btrfs/084 >> @@ -0,0 +1,84 @@ >> +#! /bin/bash >> +# FS QA Test No. 084 >> +# >> +# This is a case for the regression introduced by >> +# >> +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >> +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) >> +# >> +#----------------------------------------------------------------------- >> +# Copyright (c) 2015 Fujitsu. All Rights Reserved. >> +# >> +# This program is free software; you can redistribute it and/or >> +# modify it under the terms of the GNU General Public License as >> +# published by the Free Software Foundation. >> +# >> +# This program is distributed in the hope that it would be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program; if not, write the Free Software Foundation, >> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >> +#----------------------------------------------------------------------- >> +# >> + >> +seq=`basename $0` >> +seqres=$RESULT_DIR/$seq >> +echo "QA output created by $seq" >> + >> +here=`pwd` >> +tmp=/tmp/$$ >> +status=1 # failure is the default! >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +_cleanup() >> +{ >> + cd / >> + rm -f $tmp.* > Better to use a tab not 4 spaces, maybe "new" should be updated too (in > another patch) Thanx, will send another patch for "./new". > >> +} >> + >> +# get standard environment, filters and checks >> +. ./common/rc >> +. ./common/filter >> + >> +# real QA test starts here >> +_need_to_be_root >> +_supported_fs btrfs >> +_supported_os Linux >> +_require_scratch >> + >> +_scratch_mount > _scratch_mkfs first before you mount it. > > And you need to remove $seqres.full before test, since you called > _run_btrfs_util_prog and it will dump commands and outputs to > $seqres.full, the file will keep growing over runs if you don't remove > it first. > > rm -f $seqres.full Great! thanx for your review. > >> + >> +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null >> + >> +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap >> + >> +_run_btrfs_util_prog quota enable $SCRATCH_MNT >> +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT >> + >> +units=`_btrfs_qgroup_units` >> +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` >> + >> +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap >> + >> +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT >> + >> +timeout=100 >> +# There is a background thread doing the clean work >> +for ((i=0; i<$timeout; i++)) >> +do >> + result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` >> + if (($orig_result != $result)) >> + then >> + break >> + fi >> + sleep 1 > I'm not sure if we need 100 iterations and sleeping 1 sec after each > iteration, that means we need 100s for a PASS run. The 100sec is the limit for the loop, it means if the clean work is done in 100sec, we will break immediately. We don't have to wait for 100s in each test. > >> +done > Please follow this code style, as Dave pointed before > > for ; do > # code > done > > if ; then > # code > fi Great, >> + >> +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}' >> + >> +# success, all done >> +status=0 >> +exit >> diff --git a/tests/btrfs/084.out b/tests/btrfs/084.out >> new file mode 100644 >> index 0000000..7bf6dbf >> --- /dev/null >> +++ b/tests/btrfs/084.out >> @@ -0,0 +1,3 @@ >> +QA output created by 084 >> +24576 24576 >> +0 0 >> diff --git a/tests/btrfs/group b/tests/btrfs/group >> index fe82a9c..1be7392 100644 >> --- a/tests/btrfs/group >> +++ b/tests/btrfs/group >> @@ -86,3 +86,4 @@ >> 081 auto quick clone >> 082 auto quick remount >> 083 auto quick send >> +084 qgroup > Missed auto group? Yea, as this is my first test case for fstest, there are lots of silly problems in it. Thanx a lot for your review and help, will update it soon. Thanx Yang > > Thanks, > Eryu > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-03-16 5:06 ` Dongsheng Yang @ 2015-03-16 5:33 ` Eryu Guan 2015-03-16 5:47 ` Dongsheng Yang 2015-03-16 5:58 ` [PATCH v2] " Dongsheng Yang 1 sibling, 1 reply; 19+ messages in thread From: Eryu Guan @ 2015-03-16 5:33 UTC (permalink / raw) To: Dongsheng Yang; +Cc: fstests, linux-btrfs On Mon, Mar 16, 2015 at 01:06:52PM +0800, Dongsheng Yang wrote: > Hi Guan, sorry for the late. > > On 03/06/2015 01:06 PM, Eryu Guan wrote: > >On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote: > >>This regression is introduced by two commits: > >> > >>e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) > >>1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) [snip] > >>+_cleanup() > >>+{ > >>+ cd / > >>+ rm -f $tmp.* > >Better to use a tab not 4 spaces, maybe "new" should be updated too (in > >another patch) > > Thanx, will send another patch for "./new". Just FYI, I've already sent out the fix, please see http://www.spinics.net/lists/fstests/msg01073.html Thanks, Eryu Guan ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-03-16 5:33 ` Eryu Guan @ 2015-03-16 5:47 ` Dongsheng Yang 0 siblings, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-03-16 5:47 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, linux-btrfs On 03/16/2015 01:33 PM, Eryu Guan wrote: > On Mon, Mar 16, 2015 at 01:06:52PM +0800, Dongsheng Yang wrote: >> Hi Guan, sorry for the late. >> >> On 03/06/2015 01:06 PM, Eryu Guan wrote: >>> On Tue, Mar 03, 2015 at 07:13:30PM +0800, Dongsheng Yang wrote: >>>> This regression is introduced by two commits: >>>> >>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) > [snip] > >>>> +_cleanup() >>>> +{ >>>> + cd / >>>> + rm -f $tmp.* >>> Better to use a tab not 4 spaces, maybe "new" should be updated too (in >>> another patch) >> Thanx, will send another patch for "./new". > Just FYI, I've already sent out the fix, please see > > http://www.spinics.net/lists/fstests/msg01073.html Great!! Thanx for your information. But I found some other 4 spaces in new, I am not sure whether we also need to replace them with tab or not as shown below. Thanx commit 931340c0c5599ae2c6714df16c796ea24240a5a7 Author: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> Date: Mon Mar 16 01:15:53 2015 -0400 new: replace 4 spaces with a tab. Sugguested-by: Eryu Guan <eguan@redhat.com> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> diff --git a/new b/new index 86f9075..f94daed 100755 --- a/new +++ b/new @@ -29,15 +29,15 @@ trap "rm -f /tmp/$$.; exit" 0 1 2 3 15 _cleanup() { - : + : } SRC_GROUPS=`find tests -not -path tests -type d -printf "%f "` usage() { - echo "Usage $0 test_dir" - echo "Available dirs are: $SRC_GROUPS" - exit + echo "Usage $0 test_dir" + echo "Available dirs are: $SRC_GROUPS" + exit } [ $# -eq 0 ] && usage @@ -46,8 +46,8 @@ shift if [ ! -f $tdir/group ] then - echo "Creating the $tdir/group index ..." - cat <<'End-of-File' >$tdir/group + echo "Creating the $tdir/group index ..." + cat <<'End-of-File' >$tdir/group # QA groups control # # define groups and default group owners @@ -65,15 +65,15 @@ fi if [ ! -w $tdir/group ] then - chmod u+w $tdir/group - echo "Warning: making the index file \"$tdir/group\" writeable" + chmod u+w $tdir/group + echo "Warning: making the index file \"$tdir/group\" writeable" fi if make then - : + : else - echo "Warning: make failed -- some tests may be missing" + echo "Warning: make failed -- some tests may be missing" fi i=0 @@ -83,16 +83,16 @@ eof=1 for found in `cat $tdir/group | $AWK_PROG '{ print $1 }'` do - line=$((line+1)) - if [ -z "$found" ] || [ "$found" == "#" ];then + line=$((line+1)) + if [ -z "$found" ] || [ "$found" == "#" ];then continue - fi - i=$((i+1)) - id=`printf "%03d" $i` - if [ "$id" != "$found" ];then + fi + i=$((i+1)) + id=`printf "%03d" $i` + if [ "$id" != "$found" ];then eof=0 break - fi + fi done if [ $eof -eq 1 ]; then line=$((line+1)) @@ -104,9 +104,9 @@ echo "Next test is $id" if [ -f $tdir/$id ] then - echo "Error: test $id already exists!" - _cleanup - exit 1 + echo "Error: test $id already exists!" + _cleanup + exit 1 fi echo -n "Creating skeletal script for you to edit ..." @@ -148,8 +148,8 @@ trap "_cleanup; exit \\\$status" 0 1 2 3 15 _cleanup() { - cd / - rm -f \$tmp.* + cd / + rm -f \$tmp.* } # get standard environment, filters and checks @@ -184,39 +184,39 @@ ${EDITOR-vi} $tdir/$id if [ $# -eq 0 ] then - while true - do + while true + do echo -n "Add to group(s) [other] (? for list): " read ans [ -z "$ans" ] && ans=other if [ "X$ans" = "X?" ] then - for d in $SRC_GROUPS; do + for d in $SRC_GROUPS; do l=$(sed -n < tests/$d/group \ - -e 's/#.*//' \ - -e 's/$/ /' \ - -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p') + -e 's/#.*//' \ + -e 's/$/ /' \ + -e 's;\(^[0-9][0-9][0-9]\)\(.*$\);\2;p') grpl="$grpl $l" - done - lst=`for word in $grpl; do echo $word; done | sort| uniq ` - echo $lst + done + lst=`for word in $grpl; do echo $word; done | sort| uniq ` + echo $lst else - break + break fi - done + done else - # expert mode, groups are on the command line - # - for g in $* - do + # expert mode, groups are on the command line + # + for g in $* + do if grep "^$g[ ]" $tdir/group >/dev/null then - : + : else - echo "Warning: group \"$g\" not defined in $tdir/group" + echo "Warning: group \"$g\" not defined in $tdir/group" fi - done - ans="$*" + done + ans="$*" fi > > Thanks, > Eryu Guan > . > ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2] fstest: btrfs: add a test for quota number when deleting a subvol. 2015-03-16 5:06 ` Dongsheng Yang 2015-03-16 5:33 ` Eryu Guan @ 2015-03-16 5:58 ` Dongsheng Yang 1 sibling, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-03-16 5:58 UTC (permalink / raw) To: eguan, fstests; +Cc: linux-btrfs, Dongsheng Yang This regression is introduced by two commits: e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- changlog: v1: 1. Fix lots of coding style problems pointed by Eryu. 2. Add more description for the regression. tests/btrfs/083 | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/083.out | 3 ++ tests/btrfs/group | 1 + 3 files changed, 122 insertions(+) create mode 100755 tests/btrfs/083 create mode 100644 tests/btrfs/083.out diff --git a/tests/btrfs/083 b/tests/btrfs/083 new file mode 100755 index 0000000..a5e85e3 --- /dev/null +++ b/tests/btrfs/083 @@ -0,0 +1,118 @@ +#! /bin/bash +# FS QA Test No. 083 +# +# This is a case for the regression introduced by +# +# e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) +# 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) +# +# The problem is shown as below: +# $ btrfs sub create /mnt/sub +# Create subvolume '/mnt/sub' +# $ fallocate -l 1M /mnt/sub/data +# $ btrfs quota enable /mnt +# $ sync +# $ btrfs qgroup show --raw /mnt +# qgroupid rfer excl +# -------- ---- ---- +# 0/5 16384 16384 +# 0/257 1064960 1064960 +# $ btrfs sub dele /mnt/sub +# Delete subvolume (no-commit): '/mnt/sub' +# +# ... ... <------------Wait for the cleaner_kthread, about 30s. +# $ btrfs qgroup show --raw /mnt +# qgroupid rfer excl +# -------- ---- ---- +# 0/5 16384 16384 +# 0/257 1048576 1048576 <-----------sub 0/257 is deleted, but the quota numbers are not 0. +# +# +# What we are expecting is: +# $ btrfs qgroup show --raw /mnt +# $ qgroupid rfer excl +# -------- ---- ---- +# 0/5 16384 16384 +# 0/257 0 0 +# +# Currently, just revert these two commits mentioned above, we +# can get the expected result. But there are some more things +# we need to consider. +# +#----------------------------------------------------------------------- +# Copyright (c) 2015 Fujitsu. All Rights Reserved. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- +# + +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_need_to_be_root +_supported_fs btrfs +_supported_os Linux +_require_scratch + +rm -f $seqres.full + +_scratch_mkfs >>$seqres.full 2>&1 +_scratch_mount + +$XFS_IO_PROG -f -d -c "pwrite 0 8K" $SCRATCH_MNT/foo 2>&1 > /dev/null + +_run_btrfs_util_prog subvolume snapshot $SCRATCH_MNT $SCRATCH_MNT/snap + +_run_btrfs_util_prog quota enable $SCRATCH_MNT +_run_btrfs_util_prog quota rescan -w $SCRATCH_MNT + +units=`_btrfs_qgroup_units` +orig_result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` + +_run_btrfs_util_prog subvolume delete $SCRATCH_MNT/snap + +_run_btrfs_util_prog filesystem sync $SCRATCH_MNT + +timeout=100 +# There is a background thread doing the clean work +for ((i=0; i<$timeout; i++)); do + result=`$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG 'NR==NF {print $3}'` + if (($orig_result != $result)); then + break + fi + sleep 1 +done + +$BTRFS_UTIL_PROG qgroup show $units $SCRATCH_MNT | $AWK_PROG '/[0-9]/ {print $2" "$3}' + +# success, all done +status=0 +exit diff --git a/tests/btrfs/083.out b/tests/btrfs/083.out new file mode 100644 index 0000000..e73797e --- /dev/null +++ b/tests/btrfs/083.out @@ -0,0 +1,3 @@ +QA output created by 083 +24576 24576 +0 0 diff --git a/tests/btrfs/group b/tests/btrfs/group index fd2fa76..e98a154 100644 --- a/tests/btrfs/group +++ b/tests/btrfs/group @@ -85,3 +85,4 @@ 080 auto snapshot 081 auto quick clone 082 auto quick remount +083 auto qgroup -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-02-26 6:05 ` Dongsheng Yang 2015-03-03 11:13 ` [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol Dongsheng Yang @ 2015-03-03 11:18 ` Dongsheng Yang 2015-03-03 14:00 ` Josef Bacik 1 sibling, 1 reply; 19+ messages in thread From: Dongsheng Yang @ 2015-03-03 11:18 UTC (permalink / raw) To: Josef Bacik, mfasheh; +Cc: linux-btrfs [-- Attachment #1: Type: text/plain, Size: 6789 bytes --] On 02/26/2015 02:05 PM, Dongsheng Yang wrote: > Wait a minute, this patch seems not working well in accounting quota > number when > deleting data shared by different subvolumes. > > I will investigate more about it and send a V2 out. I have sent a fstest [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol. for this problem I was trying to solve in this patch. Please consider reverting the two commits introduced the problem: e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) as what I attached. Thanx > > Thanx > Yang > > On 02/13/2015 05:38 PM, Dongsheng Yang wrote:hen >> Hi Josef and Mark, >> >> I saw the commit message of 1152651a: >> >> [btrfs: qgroup: account shared subtrees during snapshot delete] >> >> that SUBTREE operation was introduced to account the quota number >> in subvolume deleting. >> >> But, I found it does not work well currently： >> >> Create subvolume '/mnt/sub' >> + btrfs qgroup show -prce /mnt >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- >> 0/257 16.00KiB 16.00KiB 0.00B 0.00B --- --- >> + dd if=/dev/zero of=/mnt/sub/data bs=1024 count=100 >> 100+0 records in >> 100+0 records out >> 102400 bytes (102 kB) copied, 0.000876526 s, 117 MB/s >> + sync >> + btrfs qgroup show -prce --raw /mnt >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16384 16384 0 0 --- --- >> 0/257 118784 118784 0 0 --- --- >> + btrfs sub snap /mnt/sub /mnt/snap >> Create a snapshot of '/mnt/sub' in '/mnt/snap' >> + sync >> + btrfs qgroup show -prce --raw /mnt >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16384 16384 0 0 --- --- >> 0/257 118784 16384 0 0 --- --- >> 0/258 118784 16384 0 0 --- --- >> + btrfs sub delete /mnt/sub >> Delete subvolume (no-commit): '/mnt/sub' >> + sync >> + btrfs qgroup show -prce --raw /mnt >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16384 16384 0 0 --- --- >> 0/257 118784 16384 0 0 --- --- >> 0/258 118784 16384 0 0 --- --- >> >> --------------------------------------------------------------------------- >> <------- WAIT for cleaner >> [root@atest-guest linux_btrfs]# btrfs qgroup show -prce /mnt >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- >> 0/257 100.00KiB 0.00B 0.00B 0.00B --- --- <------- the data size was >> not cleaned from qgroup. >> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------- but the excl >> in the snapshot qgroup is increased to 116KB by SUBTREE operation >> >> My question is: >> (1), SUBTREE is not working well. >> (2), why we need a rule-breaker in qgroup operations SUBTREE to do >> this work? >> We can do it via the delayed-ref routine like the other qgroup >> operations. >> >> with my patch here applied and 1152651a reverted, I got the result: >> qgroupid rfer excl max_rfer max_excl parent child >> -------- ---- ---- -------- -------- ------ ----- >> 0/5 16.00KiB 16.00KiB 0.00B 0.00B --- --- >> 0/257 0.00B 0.00B 0.00B 0.00B --- --- <----------- data and tree >> block both are cleaned >> 0/258 116.00KiB 116.00KiB 0.00B 0.00B --- --- <------ quota numbers >> in snapshot qgroup is correct. >> >> (1), quota is working well now in subvolume deleting. >> (2), all operations are inserted by delayed-ref. >> >> So, the conclusion seems like that: >> (1), commit 1152651a was introducing a SUBTREE operation to account >> the number in subvolume deleted. >> (2). SUBTREE is not working well. >> (3). we can solve the same problem in a better way without >> introducing a new operation of SUBTREE. >> >> To be honest, I am suspecting myself *very much*. I trust you guys >> and believe there must be something I am missing. >> >> Then I send this mail to ask your help. >> >> Thanx in advance. >> Yang >> >> >> On 02/10/2015 06:24 PM, Dongsheng Yang wrote: >>> In function of __btrfs_mod_ref(), we are passing the no_quota=1 >>> to process_func() anyway. It will make the numbers in qgroup be >>> wrong when deleting a subvolume. The data size in a deleted subvolume >>> will never be decressed from all related qgroups. >>> >>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >>> --- >>> fs/btrfs/extent-tree.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c >>> index 652be74..f59809c 100644 >>> --- a/fs/btrfs/extent-tree.c >>> +++ b/fs/btrfs/extent-tree.c >>> @@ -3056,6 +3056,7 @@ static int __btrfs_mod_ref(struct >>> btrfs_trans_handle *trans, >>> int i; >>> int level; >>> int ret = 0; >>> + int no_quota = 0; >>> int (*process_func)(struct btrfs_trans_handle *, struct >>> btrfs_root *, >>> u64, u64, u64, u64, u64, u64, int); >>> @@ -3080,6 +3081,9 @@ static int __btrfs_mod_ref(struct >>> btrfs_trans_handle *trans, >>> else >>> parent = 0; >>> + if (!root->fs_info->quota_enabled || !is_fstree(ref_root)) >>> + no_quota = 1; >>> + >>> for (i = 0; i < nritems; i++) { >>> if (level == 0) { >>> btrfs_item_key_to_cpu(buf, &key, i); >>> @@ -3098,7 +3102,7 @@ static int __btrfs_mod_ref(struct >>> btrfs_trans_handle *trans, >>> key.offset -= btrfs_file_extent_offset(buf, fi); >>> ret = process_func(trans, root, bytenr, num_bytes, >>> parent, ref_root, key.objectid, >>> - key.offset, 1); >>> + key.offset, no_quota); >>> if (ret) >>> goto fail; >>> } else { >>> @@ -3106,7 +3110,7 @@ static int __btrfs_mod_ref(struct >>> btrfs_trans_handle *trans, >>> num_bytes = root->nodesize; >>> ret = process_func(trans, root, bytenr, num_bytes, >>> parent, ref_root, level - 1, 0, >>> - 1); >>> + no_quota); >>> if (ret) >>> goto fail; >>> } >> >> -- >> 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 >> . >> > > -- > 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 > . > [-- Attachment #2: 0001-Revert-btrfs-qgroup-account-shared-subtrees-during-s.patch --] [-- Type: text/x-patch, Size: 15583 bytes --] >From c71244a6017d6917e0281c5390817fcf901b8ca1 Mon Sep 17 00:00:00 2001 From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> Date: Tue, 3 Mar 2015 04:31:43 -0500 Subject: [PATCH 1/2] Revert "btrfs: qgroup: account shared subtrees during snapshot delete" This reverts commit 1152651a081720ef6a8c76bb7da676e8c900ac30. Conflicts: fs/btrfs/extent-tree.c fs/btrfs/qgroup.c --- fs/btrfs/extent-tree.c | 260 ------------------------------------------------- fs/btrfs/qgroup.c | 176 --------------------------------- fs/btrfs/qgroup.h | 1 - 3 files changed, 437 deletions(-) diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 4d9f6c5..ae305fa 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -7499,219 +7499,6 @@ reada: wc->reada_slot = slot; } -static int account_leaf_items(struct btrfs_trans_handle *trans, - struct btrfs_root *root, - struct extent_buffer *eb) -{ - int nr = btrfs_header_nritems(eb); - int i, extent_type, ret; - struct btrfs_key key; - struct btrfs_file_extent_item *fi; - u64 bytenr, num_bytes; - - for (i = 0; i < nr; i++) { - btrfs_item_key_to_cpu(eb, &key, i); - - if (key.type != BTRFS_EXTENT_DATA_KEY) - continue; - - fi = btrfs_item_ptr(eb, i, struct btrfs_file_extent_item); - /* filter out non qgroup-accountable extents */ - extent_type = btrfs_file_extent_type(eb, fi); - - if (extent_type == BTRFS_FILE_EXTENT_INLINE) - continue; - - bytenr = btrfs_file_extent_disk_bytenr(eb, fi); - if (!bytenr) - continue; - - num_bytes = btrfs_file_extent_disk_num_bytes(eb, fi); - - ret = btrfs_qgroup_record_ref(trans, root->fs_info, - root->objectid, - bytenr, num_bytes, - BTRFS_QGROUP_OPER_SUB_SUBTREE, - BTRFS_QGROUP_TYPE_METADATA, 0); - if (ret) - return ret; - } - return 0; -} - -/* - * Walk up the tree from the bottom, freeing leaves and any interior - * nodes which have had all slots visited. If a node (leaf or - * interior) is freed, the node above it will have it's slot - * incremented. The root node will never be freed. - * - * At the end of this function, we should have a path which has all - * slots incremented to the next position for a search. If we need to - * read a new node it will be NULL and the node above it will have the - * correct slot selected for a later read. - * - * If we increment the root nodes slot counter past the number of - * elements, 1 is returned to signal completion of the search. - */ -static int adjust_slots_upwards(struct btrfs_root *root, - struct btrfs_path *path, int root_level) -{ - int level = 0; - int nr, slot; - struct extent_buffer *eb; - - if (root_level == 0) - return 1; - - while (level <= root_level) { - eb = path->nodes[level]; - nr = btrfs_header_nritems(eb); - path->slots[level]++; - slot = path->slots[level]; - if (slot >= nr || level == 0) { - /* - * Don't free the root - we will detect this - * condition after our loop and return a - * positive value for caller to stop walking the tree. - */ - if (level != root_level) { - btrfs_tree_unlock_rw(eb, path->locks[level]); - path->locks[level] = 0; - - free_extent_buffer(eb); - path->nodes[level] = NULL; - path->slots[level] = 0; - } - } else { - /* - * We have a valid slot to walk back down - * from. Stop here so caller can process these - * new nodes. - */ - break; - } - - level++; - } - - eb = path->nodes[root_level]; - if (path->slots[root_level] >= btrfs_header_nritems(eb)) - return 1; - - return 0; -} - -/* - * root_eb is the subtree root and is locked before this function is called. - */ -static int account_shared_subtree(struct btrfs_trans_handle *trans, - struct btrfs_root *root, - struct extent_buffer *root_eb, - u64 root_gen, - int root_level) -{ - int ret = 0; - int level; - struct extent_buffer *eb = root_eb; - struct btrfs_path *path = NULL; - - BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL); - BUG_ON(root_eb == NULL); - - if (!root->fs_info->quota_enabled) - return 0; - - if (!extent_buffer_uptodate(root_eb)) { - ret = btrfs_read_buffer(root_eb, root_gen); - if (ret) - goto out; - } - - if (root_level == 0) { - ret = account_leaf_items(trans, root, root_eb); - goto out; - } - - path = btrfs_alloc_path(); - if (!path) - return -ENOMEM; - - /* - * Walk down the tree. Missing extent blocks are filled in as - * we go. Metadata is accounted every time we read a new - * extent block. - * - * When we reach a leaf, we account for file extent items in it, - * walk back up the tree (adjusting slot pointers as we go) - * and restart the search process. - */ - extent_buffer_get(root_eb); /* For path */ - path->nodes[root_level] = root_eb; - path->slots[root_level] = 0; - path->locks[root_level] = 0; /* so release_path doesn't try to unlock */ -walk_down: - level = root_level; - while (level >= 0) { - if (path->nodes[level] == NULL) { - int parent_slot; - u64 child_gen; - u64 child_bytenr; - - /* We need to get child blockptr/gen from - * parent before we can read it. */ - eb = path->nodes[level + 1]; - parent_slot = path->slots[level + 1]; - child_bytenr = btrfs_node_blockptr(eb, parent_slot); - child_gen = btrfs_node_ptr_generation(eb, parent_slot); - - eb = read_tree_block(root, child_bytenr, child_gen); - if (!eb || !extent_buffer_uptodate(eb)) { - ret = -EIO; - goto out; - } - - path->nodes[level] = eb; - path->slots[level] = 0; - - btrfs_tree_read_lock(eb); - btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK); - path->locks[level] = BTRFS_READ_LOCK_BLOCKING; - - ret = btrfs_qgroup_record_ref(trans, root->fs_info, - root->objectid, - child_bytenr, - root->nodesize, - BTRFS_QGROUP_OPER_SUB_SUBTREE, - BTRFS_QGROUP_TYPE_METADATA, 0); - if (ret) - goto out; - - } - - if (level == 0) { - ret = account_leaf_items(trans, root, path->nodes[level]); - if (ret) - goto out; - - /* Nonzero return here means we completed our search */ - ret = adjust_slots_upwards(root, path, root_level); - if (ret) - break; - - /* Restart search with new slots */ - goto walk_down; - } - - level--; - } - - ret = 0; -out: - btrfs_free_path(path); - - return ret; -} - /* * helper to process tree block while walking down the tree. * @@ -7815,7 +7602,6 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans, int level = wc->level; int reada = 0; int ret = 0; - bool need_account = false; generation = btrfs_node_ptr_generation(path->nodes[level], path->slots[level]); @@ -7861,7 +7647,6 @@ static noinline int do_walk_down(struct btrfs_trans_handle *trans, if (wc->stage == DROP_REFERENCE) { if (wc->refs[level - 1] > 1) { - need_account = true; if (level == 1 && (wc->flags[0] & BTRFS_BLOCK_FLAG_FULL_BACKREF)) goto skip; @@ -7925,16 +7710,6 @@ skip: parent = 0; } - if (need_account) { - ret = account_shared_subtree(trans, root, next, - generation, level - 1); - if (ret) { - printk_ratelimited(KERN_ERR "BTRFS: %s Error " - "%d accounting shared subtree. Quota " - "is out of sync, rescan required.\n", - root->fs_info->sb->s_id, ret); - } - } ret = btrfs_free_extent(trans, root, bytenr, blocksize, parent, root->root_key.objectid, level - 1, 0, 0); BUG_ON(ret); /* -ENOMEM */ @@ -8019,13 +7794,6 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, else ret = btrfs_dec_ref(trans, root, eb, 0); BUG_ON(ret); /* -ENOMEM */ - ret = account_leaf_items(trans, root, eb); - if (ret) { - printk_ratelimited(KERN_ERR "BTRFS: %s Error " - "%d accounting leaf items. Quota " - "is out of sync, rescan required.\n", - root->fs_info->sb->s_id, ret); - } } /* make block locked assertion in clean_tree_block happy */ if (!path->locks[level] && @@ -8151,8 +7919,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, int level; bool root_dropped = false; - btrfs_debug(root->fs_info, "Drop subvolume %llu", root->objectid); - path = btrfs_alloc_path(); if (!path) { err = -ENOMEM; @@ -8278,24 +8044,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, goto out_end_trans; } - /* - * Qgroup update accounting is run from - * delayed ref handling. This usually works - * out because delayed refs are normally the - * only way qgroup updates are added. However, - * we may have added updates during our tree - * walk so run qgroups here to make sure we - * don't lose any updates. - */ - ret = btrfs_delayed_qgroup_accounting(trans, - root->fs_info); - if (ret) - printk_ratelimited(KERN_ERR "BTRFS: Failure %d " - "running qgroup updates " - "during snapshot delete. " - "Quota is out of sync, " - "rescan required.\n", ret); - btrfs_end_transaction_throttle(trans, tree_root); if (!for_reloc && btrfs_need_cleaner_sleep(root)) { pr_debug("BTRFS: drop snapshot early exit\n"); @@ -8349,14 +8097,6 @@ int btrfs_drop_snapshot(struct btrfs_root *root, } root_dropped = true; out_end_trans: - ret = btrfs_delayed_qgroup_accounting(trans, tree_root->fs_info); - if (ret) - printk_ratelimited(KERN_ERR "BTRFS: Failure %d " - "running qgroup updates " - "during snapshot delete. " - "Quota is out of sync, " - "rescan required.\n", ret); - btrfs_end_transaction_throttle(trans, tree_root); out_free: kfree(wc); diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index 103a908..2acf059 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1262,50 +1262,6 @@ out: mutex_unlock(&fs_info->qgroup_ioctl_lock); return ret; } - -static int comp_oper_exist(struct btrfs_qgroup_operation *oper1, - struct btrfs_qgroup_operation *oper2) -{ - /* - * Ignore seq and type here, we're looking for any operation - * at all related to this extent on that root. - */ - if (oper1->bytenr < oper2->bytenr) - return -1; - if (oper1->bytenr > oper2->bytenr) - return 1; - if (oper1->ref_root < oper2->ref_root) - return -1; - if (oper1->ref_root > oper2->ref_root) - return 1; - return 0; -} - -static int qgroup_oper_exists(struct btrfs_fs_info *fs_info, - struct btrfs_qgroup_operation *oper) -{ - struct rb_node *n; - struct btrfs_qgroup_operation *cur; - int cmp; - - spin_lock(&fs_info->qgroup_op_lock); - n = fs_info->qgroup_op_tree.rb_node; - while (n) { - cur = rb_entry(n, struct btrfs_qgroup_operation, n); - cmp = comp_oper_exist(cur, oper); - if (cmp < 0) { - n = n->rb_right; - } else if (cmp) { - n = n->rb_left; - } else { - spin_unlock(&fs_info->qgroup_op_lock); - return -EEXIST; - } - } - spin_unlock(&fs_info->qgroup_op_lock); - return 0; -} - static int comp_oper(struct btrfs_qgroup_operation *oper1, struct btrfs_qgroup_operation *oper2) { @@ -1397,25 +1353,6 @@ int btrfs_qgroup_record_ref(struct btrfs_trans_handle *trans, oper->seq = atomic_inc_return(&fs_info->qgroup_op_seq); INIT_LIST_HEAD(&oper->elem.list); oper->elem.seq = 0; - - trace_btrfs_qgroup_record_ref(oper); - - if (type == BTRFS_QGROUP_OPER_SUB_SUBTREE) { - /* - * If any operation for this bytenr/ref_root combo - * exists, then we know it's not exclusively owned and - * shouldn't be queued up. - * - * This also catches the case where we have a cloned - * extent that gets queued up multiple times during - * drop snapshot. - */ - if (qgroup_oper_exists(fs_info, oper)) { - kfree(oper); - return 0; - } - } - ret = insert_qgroup_oper(fs_info, oper); if (ret) { /* Shouldn't happen so have an assert for developers */ @@ -2025,116 +1962,6 @@ out: } /* - * Process a reference to a shared subtree. This type of operation is - * queued during snapshot removal when we encounter extents which are - * shared between more than one root. - */ -static int qgroup_subtree_accounting(struct btrfs_trans_handle *trans, - struct btrfs_fs_info *fs_info, - struct btrfs_qgroup_operation *oper) -{ - struct ulist *roots = NULL; - struct ulist_node *unode; - struct ulist_iterator uiter; - struct btrfs_qgroup_list *glist; - struct ulist *parents; - int ret = 0; - int err; - struct btrfs_qgroup *qg; - u64 root_obj = 0; - struct seq_list elem = {}; - - parents = ulist_alloc(GFP_NOFS); - if (!parents) - return -ENOMEM; - - btrfs_get_tree_mod_seq(fs_info, &elem); - ret = btrfs_find_all_roots(trans, fs_info, oper->bytenr, - elem.seq, &roots); - btrfs_put_tree_mod_seq(fs_info, &elem); - if (ret < 0) - goto out; - - if (roots->nnodes != 1) - goto out; - - ULIST_ITER_INIT(&uiter); - unode = ulist_next(roots, &uiter); /* Only want 1 so no need to loop */ - /* - * If we find our ref root then that means all refs - * this extent has to the root have not yet been - * deleted. In that case, we do nothing and let the - * last ref for this bytenr drive our update. - * - * This can happen for example if an extent is - * referenced multiple times in a snapshot (clone, - * etc). If we are in the middle of snapshot removal, - * queued updates for such an extent will find the - * root if we have not yet finished removing the - * snapshot. - */ - if (unode->val == oper->ref_root) - goto out; - - root_obj = unode->val; - BUG_ON(!root_obj); - - spin_lock(&fs_info->qgroup_lock); - qg = find_qgroup_rb(fs_info, root_obj); - if (!qg) - goto out_unlock; - - if ((qg->type & oper->quota_type)) { - qg->excl += oper->num_bytes; - qg->excl_cmpr += oper->num_bytes; - qgroup_dirty(fs_info, qg); - } - - /* - * Adjust counts for parent groups. First we find all - * parents, then in the 2nd loop we do the adjustment - * while adding parents of the parents to our ulist. - */ - list_for_each_entry(glist, &qg->groups, next_group) { - err = ulist_add(parents, glist->group->qgroupid, - ptr_to_u64(glist->group), GFP_ATOMIC); - if (err < 0) { - ret = err; - goto out_unlock; - } - } - - ULIST_ITER_INIT(&uiter); - while ((unode = ulist_next(parents, &uiter))) { - qg = u64_to_ptr(unode->aux); - - if ((qg->type & oper->quota_type)) { - qg->excl += oper->num_bytes; - qg->excl_cmpr += oper->num_bytes; - qgroup_dirty(fs_info, qg); - } - - /* Add any parents of the parents */ - list_for_each_entry(glist, &qg->groups, next_group) { - err = ulist_add(parents, glist->group->qgroupid, - ptr_to_u64(glist->group), GFP_ATOMIC); - if (err < 0) { - ret = err; - goto out_unlock; - } - } - } - -out_unlock: - spin_unlock(&fs_info->qgroup_lock); - -out: - ulist_free(roots); - ulist_free(parents); - return ret; -} - -/* * btrfs_qgroup_account_ref is called for every ref that is added to or deleted * from the fs. First, all roots referencing the extent are searched, and * then the space is accounted accordingly to the different roots. The @@ -2173,9 +2000,6 @@ static int btrfs_qgroup_account(struct btrfs_trans_handle *trans, case BTRFS_QGROUP_OPER_SUB_SHARED: ret = qgroup_shared_accounting(trans, fs_info, oper); break; - case BTRFS_QGROUP_OPER_SUB_SUBTREE: - ret = qgroup_subtree_accounting(trans, fs_info, oper); - break; default: ASSERT(0); } diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h index 52cfdc2..9722888 100644 --- a/fs/btrfs/qgroup.h +++ b/fs/btrfs/qgroup.h @@ -60,7 +60,6 @@ enum btrfs_qgroup_operation_type { BTRFS_QGROUP_OPER_ADD_SHARED, BTRFS_QGROUP_OPER_SUB_EXCL, BTRFS_QGROUP_OPER_SUB_SHARED, - BTRFS_QGROUP_OPER_SUB_SUBTREE, }; struct btrfs_qgroup_operation { -- 1.8.4.2 [-- Attachment #3: 0002-Revert-Btrfs-__btrfs_mod_ref-should-always-use-no_qu.patch --] [-- Type: text/x-patch, Size: 6600 bytes --] >From 21b2e636b1beb005d39188608030793c41c1e4fc Mon Sep 17 00:00:00 2001 From: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> Date: Tue, 3 Mar 2015 06:59:19 -0500 Subject: [PATCH 2/2] Revert "Btrfs: __btrfs_mod_ref should always use no_quota" This reverts commit e339a6b097c515a31ce230d498c44ff2e89f1cf4. --- fs/btrfs/ctree.c | 20 ++++++++++---------- fs/btrfs/ctree.h | 4 ++-- fs/btrfs/extent-tree.c | 24 +++++++++++++----------- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 14a72ed..a06cd4a 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -269,9 +269,9 @@ int btrfs_copy_root(struct btrfs_trans_handle *trans, WARN_ON(btrfs_header_generation(buf) > trans->transid); if (new_root_objectid == BTRFS_TREE_RELOC_OBJECTID) - ret = btrfs_inc_ref(trans, root, cow, 1); + ret = btrfs_inc_ref(trans, root, cow, 1, 1); else - ret = btrfs_inc_ref(trans, root, cow, 0); + ret = btrfs_inc_ref(trans, root, cow, 0, 1); if (ret) return ret; @@ -1024,14 +1024,14 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, if ((owner == root->root_key.objectid || root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) && !(flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)) { - ret = btrfs_inc_ref(trans, root, buf, 1); + ret = btrfs_inc_ref(trans, root, buf, 1, 1); BUG_ON(ret); /* -ENOMEM */ if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) { - ret = btrfs_dec_ref(trans, root, buf, 0); + ret = btrfs_dec_ref(trans, root, buf, 0, 1); BUG_ON(ret); /* -ENOMEM */ - ret = btrfs_inc_ref(trans, root, cow, 1); + ret = btrfs_inc_ref(trans, root, cow, 1, 1); BUG_ON(ret); /* -ENOMEM */ } new_flags |= BTRFS_BLOCK_FLAG_FULL_BACKREF; @@ -1039,9 +1039,9 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) - ret = btrfs_inc_ref(trans, root, cow, 1); + ret = btrfs_inc_ref(trans, root, cow, 1, 1); else - ret = btrfs_inc_ref(trans, root, cow, 0); + ret = btrfs_inc_ref(trans, root, cow, 0, 1); BUG_ON(ret); /* -ENOMEM */ } if (new_flags != 0) { @@ -1058,11 +1058,11 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) { if (root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID) - ret = btrfs_inc_ref(trans, root, cow, 1); + ret = btrfs_inc_ref(trans, root, cow, 1, 1); else - ret = btrfs_inc_ref(trans, root, cow, 0); + ret = btrfs_inc_ref(trans, root, cow, 0, 1); BUG_ON(ret); /* -ENOMEM */ - ret = btrfs_dec_ref(trans, root, buf, 1); + ret = btrfs_dec_ref(trans, root, buf, 1, 1); BUG_ON(ret); /* -ENOMEM */ } clean_tree_block(trans, root, buf); diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 8034d07..d58a6d3 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -3387,9 +3387,9 @@ int btrfs_reserve_extent(struct btrfs_root *root, u64 num_bytes, u64 min_alloc_size, u64 empty_size, u64 hint_byte, struct btrfs_key *ins, int is_data, int delalloc); int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct extent_buffer *buf, int full_backref); + struct extent_buffer *buf, int full_backref, int no_quota); int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct extent_buffer *buf, int full_backref); + struct extent_buffer *buf, int full_backref, int no_quota); int btrfs_set_disk_extent_flags(struct btrfs_trans_handle *trans, struct btrfs_root *root, u64 bytenr, u64 num_bytes, u64 flags, diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index ae305fa..313aff9 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -3049,7 +3049,7 @@ out: static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root, struct extent_buffer *buf, - int full_backref, int inc) + int full_backref, int inc, int no_quota) { u64 bytenr; u64 num_bytes; @@ -3103,7 +3103,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, key.offset -= btrfs_file_extent_offset(buf, fi); ret = process_func(trans, root, bytenr, num_bytes, parent, ref_root, key.objectid, - key.offset, 1); + key.offset, no_quota); if (ret) goto fail; } else { @@ -3111,7 +3111,7 @@ static int __btrfs_mod_ref(struct btrfs_trans_handle *trans, num_bytes = root->nodesize; ret = process_func(trans, root, bytenr, num_bytes, parent, ref_root, level - 1, 0, - 1); + no_quota); if (ret) goto fail; } @@ -3122,15 +3122,15 @@ fail: } int btrfs_inc_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct extent_buffer *buf, int full_backref) + struct extent_buffer *buf, int full_backref, int no_quota) { - return __btrfs_mod_ref(trans, root, buf, full_backref, 1); + return __btrfs_mod_ref(trans, root, buf, full_backref, 1, no_quota); } int btrfs_dec_ref(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct extent_buffer *buf, int full_backref) + struct extent_buffer *buf, int full_backref, int no_quota) { - return __btrfs_mod_ref(trans, root, buf, full_backref, 0); + return __btrfs_mod_ref(trans, root, buf, full_backref, 0, no_quota); } static int write_one_cache_group(struct btrfs_trans_handle *trans, @@ -7553,9 +7553,9 @@ static noinline int walk_down_proc(struct btrfs_trans_handle *trans, /* wc->stage == UPDATE_BACKREF */ if (!(wc->flags[level] & flag)) { BUG_ON(!path->locks[level]); - ret = btrfs_inc_ref(trans, root, eb, 1); + ret = btrfs_inc_ref(trans, root, eb, 1, wc->for_reloc); BUG_ON(ret); /* -ENOMEM */ - ret = btrfs_dec_ref(trans, root, eb, 0); + ret = btrfs_dec_ref(trans, root, eb, 0, wc->for_reloc); BUG_ON(ret); /* -ENOMEM */ ret = btrfs_set_disk_extent_flags(trans, root, eb->start, eb->len, flag, @@ -7790,9 +7790,11 @@ static noinline int walk_up_proc(struct btrfs_trans_handle *trans, if (wc->refs[level] == 1) { if (level == 0) { if (wc->flags[level] & BTRFS_BLOCK_FLAG_FULL_BACKREF) - ret = btrfs_dec_ref(trans, root, eb, 1); + ret = btrfs_dec_ref(trans, root, eb, 1, + wc->for_reloc); else - ret = btrfs_dec_ref(trans, root, eb, 0); + ret = btrfs_dec_ref(trans, root, eb, 0, + wc->for_reloc); BUG_ON(ret); /* -ENOMEM */ } /* make block locked assertion in clean_tree_block happy */ -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-03-03 11:18 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang @ 2015-03-03 14:00 ` Josef Bacik 2015-03-03 20:20 ` Mark Fasheh 0 siblings, 1 reply; 19+ messages in thread From: Josef Bacik @ 2015-03-03 14:00 UTC (permalink / raw) To: Dongsheng Yang, mfasheh; +Cc: linux-btrfs On 03/03/2015 06:18 AM, Dongsheng Yang wrote: > On 02/26/2015 02:05 PM, Dongsheng Yang wrote: >> Wait a minute, this patch seems not working well in accounting quota >> number when >> deleting data shared by different subvolumes. >> >> I will investigate more about it and send a V2 out. > I have sent a fstest > [PATCH] fstest: btrfs: add a test for quota number when deleting a > subvol. > for this problem I was trying to solve in this patch. > > Please consider reverting the two commits introduced the problem: > > e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) > 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) > We aren't reverting these two commits, what we had was worse before than it is now. We need to figure out why this is resulting in problems and fix that rather than throwing out a whole bunch of work. Thanks, Josef ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-03-03 14:00 ` Josef Bacik @ 2015-03-03 20:20 ` Mark Fasheh 2015-03-16 4:59 ` Dongsheng Yang 0 siblings, 1 reply; 19+ messages in thread From: Mark Fasheh @ 2015-03-03 20:20 UTC (permalink / raw) To: Josef Bacik; +Cc: Dongsheng Yang, linux-btrfs On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote: > On 03/03/2015 06:18 AM, Dongsheng Yang wrote: >> On 02/26/2015 02:05 PM, Dongsheng Yang wrote: >>> Wait a minute, this patch seems not working well in accounting quota >>> number when >>> deleting data shared by different subvolumes. >>> >>> I will investigate more about it and send a V2 out. >> I have sent a fstest >> [PATCH] fstest: btrfs: add a test for quota number when deleting a >> subvol. >> for this problem I was trying to solve in this patch. >> >> Please consider reverting the two commits introduced the problem: >> >> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) >> > > We aren't reverting these two commits, what we had was worse before than it > is now. We need to figure out why this is resulting in problems and fix > that rather than throwing out a whole bunch of work. Thanks, Agreed, reverting these would re-introduce far more problems than it would solve. --Mark -- Mark Fasheh ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-03-03 20:20 ` Mark Fasheh @ 2015-03-16 4:59 ` Dongsheng Yang 2015-03-17 15:27 ` Dongsheng Yang 0 siblings, 1 reply; 19+ messages in thread From: Dongsheng Yang @ 2015-03-16 4:59 UTC (permalink / raw) To: Mark Fasheh, Josef Bacik; +Cc: linux-btrfs On 03/04/2015 04:20 AM, Mark Fasheh wrote: > On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote: >> On 03/03/2015 06:18 AM, Dongsheng Yang wrote: >>> On 02/26/2015 02:05 PM, Dongsheng Yang wrote: >>>> Wait a minute, this patch seems not working well in accounting quota >>>> number when >>>> deleting data shared by different subvolumes. >>>> >>>> I will investigate more about it and send a V2 out. >>> I have sent a fstest >>> [PATCH] fstest: btrfs: add a test for quota number when deleting a >>> subvol. >>> for this problem I was trying to solve in this patch. >>> >>> Please consider reverting the two commits introduced the problem: >>> >>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) >>> >> We aren't reverting these two commits, what we had was worse before than it >> is now. We need to figure out why this is resulting in problems and fix >> that rather than throwing out a whole bunch of work. Thanks, > Agreed, reverting these would re-introduce far more problems than it would > solve. Hi Josef and Mark, Sorry for the late. Thanx for your reply and sorry I am not clear about the "far more problems". From the result below, I saw there are some diff reports in V4.0-rc1 and when I revert these two commits. It's consistent now. But I think there must be something I am missing. I am glad to investigate more about it and solve it. But at first I need to know what's the problem SUBTREE is trying to solve. Example: mkfs.btrfs /dev/sdc -f mount /dev/sdc /mnt btrfs quota enable /mnt btrfs sub create /mnt/sub dd if=/dev/zero of=/mnt/sub/data bs=1024 count=1000 btrfs sub snapshot /mnt/sub /mnt/snap btrfs qgroup create 1/1 /mnt btrfs qgroup assign 0/257 1/1 /mnt btrfs quota rescan -w /mnt sync btrfs sub delete /mnt/sub ----------------------- <-------------wait for cleaner_kthread. btrfs qgroup show /mnt umount /mnt btrfs check --qgroup-report /dev/sdc |grep diff Result: (1). V4.0-rc1 # btrfs qgroup show /mnt qgroupid rfer excl -------- ---- ---- 0/5 16.00KiB 16.00KiB 0/257 1000.00KiB 0.00B 0/258 1016.00KiB 1016.00KiB 1/1 1000.00KiB 0.00B # btrfs check --qgroup-report /dev/sdc |grep diff Counts for qgroup id: 257 are different diff: referenced -1024000 referenced compressed -1024000 Counts for qgroup id: 281474976710657 are different diff: referenced -1024000 referenced compressed -1024000 (2). V4.0-rc1 with reverts of "96c92f" and "c04cad" # btrfs qgroup show /mnt qgroupid rfer excl -------- ---- ---- 0/5 16.00KiB 16.00KiB 0/257 0.00B 0.00B 0/258 1016.00KiB 1016.00KiB 1/1 0.00B 0.00B # btrfs check --qgroup-report /dev/sdc |grep diff # *No diff in btrfs check --qgroup-report* Thanx > --Mark > > -- > Mark Fasheh > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota. 2015-03-16 4:59 ` Dongsheng Yang @ 2015-03-17 15:27 ` Dongsheng Yang 0 siblings, 0 replies; 19+ messages in thread From: Dongsheng Yang @ 2015-03-17 15:27 UTC (permalink / raw) To: Dongsheng Yang; +Cc: Mark Fasheh, Josef Bacik, linux-btrfs Okey, I found a patch from Mark 35 weeks ago which was not merged into xfstest: [PATCH] xfstests/btrfs: add test for quota groups and drop snapshot It looks can answer my question here. Thanx Yang On Mon, Mar 16, 2015 at 12:59 PM, Dongsheng Yang <yangds.fnst@cn.fujitsu.com> wrote: > On 03/04/2015 04:20 AM, Mark Fasheh wrote: >> >> On Tue, Mar 03, 2015 at 09:00:36AM -0500, Josef Bacik wrote: >>> >>> On 03/03/2015 06:18 AM, Dongsheng Yang wrote: >>>> >>>> On 02/26/2015 02:05 PM, Dongsheng Yang wrote: >>>>> >>>>> Wait a minute, this patch seems not working well in accounting quota >>>>> number when >>>>> deleting data shared by different subvolumes. >>>>> >>>>> I will investigate more about it and send a V2 out. >>>> >>>> I have sent a fstest >>>> [PATCH] fstest: btrfs: add a test for quota number when deleting a >>>> subvol. >>>> for this problem I was trying to solve in this patch. >>>> >>>> Please consider reverting the two commits introduced the problem: >>>> >>>> e339a6b0 (Btrfs: __btrfs_mod_ref should always use no_quota) >>>> 1152651a (btrfs: qgroup: account shared subtrees during snapshot delete) >>>> >>> We aren't reverting these two commits, what we had was worse before than >>> it >>> is now. We need to figure out why this is resulting in problems and fix >>> that rather than throwing out a whole bunch of work. Thanks, >> >> Agreed, reverting these would re-introduce far more problems than it would >> solve. > > > Hi Josef and Mark, > > Sorry for the late. > > Thanx for your reply and sorry I am not clear about the "far more problems". > > From the result below, I saw there are some diff reports in V4.0-rc1 and > when I > revert these two commits. It's consistent now. > > But I think there must be something I am missing. I am glad to investigate > more > about it and solve it. But at first I need to know what's the problem > SUBTREE is > trying to solve. > > Example: > mkfs.btrfs /dev/sdc -f > mount /dev/sdc /mnt > btrfs quota enable /mnt > btrfs sub create /mnt/sub > dd if=/dev/zero of=/mnt/sub/data bs=1024 count=1000 > btrfs sub snapshot /mnt/sub /mnt/snap > btrfs qgroup create 1/1 /mnt > btrfs qgroup assign 0/257 1/1 /mnt > btrfs quota rescan -w /mnt > sync > btrfs sub delete /mnt/sub > ----------------------- <-------------wait for cleaner_kthread. > btrfs qgroup show /mnt > umount /mnt > btrfs check --qgroup-report /dev/sdc |grep diff > > Result: > (1). V4.0-rc1 > # btrfs qgroup show /mnt > qgroupid rfer excl > -------- ---- ---- > 0/5 16.00KiB 16.00KiB > 0/257 1000.00KiB 0.00B > 0/258 1016.00KiB 1016.00KiB > 1/1 1000.00KiB 0.00B > # btrfs check --qgroup-report /dev/sdc |grep diff > Counts for qgroup id: 257 are different > diff: referenced -1024000 referenced compressed -1024000 > Counts for qgroup id: 281474976710657 are different > diff: referenced -1024000 referenced compressed -1024000 > > (2). V4.0-rc1 with reverts of "96c92f" and "c04cad" > # btrfs qgroup show /mnt > qgroupid rfer excl > -------- ---- ---- > 0/5 16.00KiB 16.00KiB > 0/257 0.00B 0.00B > 0/258 1016.00KiB 1016.00KiB > 1/1 0.00B 0.00B > # btrfs check --qgroup-report /dev/sdc |grep diff > # > *No diff in btrfs check --qgroup-report* > > > Thanx >> >> --Mark >> >> -- >> Mark Fasheh >> . >> > > -- > 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] 19+ messages in thread
end of thread, other threads:[~2015-03-17 15:27 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-10 10:24 [PATCH 0/3] Btrfs: qgroup: part-3: bug fixes for deleting subvolume Dongsheng Yang 2015-02-10 10:24 ` [PATCH 1/3] btrfs: qgroup: return EINVAL if level of parent is not higher than child's Dongsheng Yang 2015-02-10 10:24 ` [PATCH 2/3] btrfs: qgroup: allow to remove qgroup which has parent but no child Dongsheng Yang 2015-02-10 10:24 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 2015-02-10 11:24 ` Filipe David Manana 2015-02-11 2:51 ` Dongsheng Yang 2015-02-13 9:38 ` Dongsheng Yang 2015-02-26 6:05 ` Dongsheng Yang 2015-03-03 11:13 ` [PATCH] fstest: btrfs: add a test for quota number when deleting a subvol Dongsheng Yang 2015-03-06 5:06 ` Eryu Guan 2015-03-16 5:06 ` Dongsheng Yang 2015-03-16 5:33 ` Eryu Guan 2015-03-16 5:47 ` Dongsheng Yang 2015-03-16 5:58 ` [PATCH v2] " Dongsheng Yang 2015-03-03 11:18 ` [PATCH 3/3] btrfs: qgroup: fix a wrong parameter of no_quota Dongsheng Yang 2015-03-03 14:00 ` Josef Bacik 2015-03-03 20:20 ` Mark Fasheh 2015-03-16 4:59 ` Dongsheng Yang 2015-03-17 15:27 ` Dongsheng Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).