| Message ID | 1366101920-13083-2-git-send-email-list.btrfs@jan-o-sch.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Jan, > The function is separated into a preparation part and the three accounting > steps mentioned in the qgroups documentation. The goal is to make steps two > and three usable by the rescan functionality. A side effect is that the > function is restructured into readable subunits. How about renaming the three functions like: 1> qgroup_walk_old_roots() 2> qgroup_walk_new_root() 3> qgroup_rewalk_old_root() I'd like this function to be meaningful, but not just step1,2,3. Maybe you can think out better function name. Thanks, Wang > > Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net> > --- > fs/btrfs/qgroup.c | 212 ++++++++++++++++++++++++++++++----------------------- > 1 files changed, 121 insertions(+), 91 deletions(-) > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c > index b44124d..c38a0c5 100644 > --- a/fs/btrfs/qgroup.c > +++ b/fs/btrfs/qgroup.c > @@ -1075,6 +1075,122 @@ int btrfs_qgroup_record_ref(struct btrfs_trans_handle *trans, > return 0; > } > > +static void qgroup_account_ref_step1(struct btrfs_fs_info *fs_info, > + struct ulist *roots, struct ulist *tmp, > + u64 seq) > +{ > + struct ulist_node *unode; > + struct ulist_iterator uiter; > + struct ulist_node *tmp_unode; > + struct ulist_iterator tmp_uiter; > + struct btrfs_qgroup *qg; > + > + ULIST_ITER_INIT(&uiter); > + while ((unode = ulist_next(roots, &uiter))) { > + qg = find_qgroup_rb(fs_info, unode->val); > + if (!qg) > + continue; > + > + ulist_reinit(tmp); > + /* XXX id not needed */ > + ulist_add(tmp, qg->qgroupid, (u64)(uintptr_t)qg, GFP_ATOMIC); > + ULIST_ITER_INIT(&tmp_uiter); > + while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { > + struct btrfs_qgroup_list *glist; > + > + qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; > + if (qg->refcnt < seq) > + qg->refcnt = seq + 1; > + else > + ++qg->refcnt; > + > + list_for_each_entry(glist, &qg->groups, next_group) { > + ulist_add(tmp, glist->group->qgroupid, > + (u64)(uintptr_t)glist->group, > + GFP_ATOMIC); > + } > + } > + } > +} > + > +static void qgroup_account_ref_step2(struct btrfs_fs_info *fs_info, > + struct ulist *roots, struct ulist *tmp, > + u64 seq, int sgn, u64 num_bytes, > + struct btrfs_qgroup *qgroup) > +{ > + struct ulist_node *unode; > + struct ulist_iterator uiter; > + struct btrfs_qgroup *qg; > + struct btrfs_qgroup_list *glist; > + > + ulist_reinit(tmp); > + ulist_add(tmp, qgroup->qgroupid, (uintptr_t)qgroup, GFP_ATOMIC); > + > + ULIST_ITER_INIT(&uiter); > + while ((unode = ulist_next(tmp, &uiter))) { > + > + qg = (struct btrfs_qgroup *)(uintptr_t)unode->aux; > + if (qg->refcnt < seq) { > + /* not visited by step 1 */ > + qg->rfer += sgn * num_bytes; > + qg->rfer_cmpr += sgn * num_bytes; > + if (roots->nnodes == 0) { > + qg->excl += sgn * num_bytes; > + qg->excl_cmpr += sgn * num_bytes; > + } > + qgroup_dirty(fs_info, qg); > + } > + WARN_ON(qg->tag >= seq); > + qg->tag = seq; > + > + list_for_each_entry(glist, &qg->groups, next_group) { > + ulist_add(tmp, glist->group->qgroupid, > + (uintptr_t)glist->group, GFP_ATOMIC); > + } > + } > +} > + > +static void qgroup_account_ref_step3(struct btrfs_fs_info *fs_info, > + struct ulist *roots, struct ulist *tmp, > + u64 seq, int sgn, u64 num_bytes) > +{ > + struct ulist_node *unode; > + struct ulist_iterator uiter; > + struct btrfs_qgroup *qg; > + struct ulist_node *tmp_unode; > + struct ulist_iterator tmp_uiter; > + > + ULIST_ITER_INIT(&uiter); > + while ((unode = ulist_next(roots, &uiter))) { > + qg = find_qgroup_rb(fs_info, unode->val); > + if (!qg) > + continue; > + > + ulist_reinit(tmp); > + ulist_add(tmp, qg->qgroupid, (uintptr_t)qg, GFP_ATOMIC); > + ULIST_ITER_INIT(&tmp_uiter); > + while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { > + struct btrfs_qgroup_list *glist; > + > + qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; > + if (qg->tag == seq) > + continue; > + > + if (qg->refcnt - seq == roots->nnodes) { > + qg->excl -= sgn * num_bytes; > + qg->excl_cmpr -= sgn * num_bytes; > + qgroup_dirty(fs_info, qg); > + } > + > + list_for_each_entry(glist, &qg->groups, next_group) { > + ulist_add(tmp, glist->group->qgroupid, > + (uintptr_t)glist->group, > + GFP_ATOMIC); > + } > + } > + } > +} > + > /* > * 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 > @@ -1090,10 +1206,8 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, > struct btrfs_root *quota_root; > u64 ref_root; > struct btrfs_qgroup *qgroup; > - struct ulist_node *unode; > struct ulist *roots = NULL; > struct ulist *tmp = NULL; > - struct ulist_iterator uiter; > u64 seq; > int ret = 0; > int sgn; > @@ -1175,103 +1289,19 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, > seq = fs_info->qgroup_seq; > fs_info->qgroup_seq += roots->nnodes + 1; /* max refcnt */ > > - ULIST_ITER_INIT(&uiter); > - while ((unode = ulist_next(roots, &uiter))) { > - struct ulist_node *tmp_unode; > - struct ulist_iterator tmp_uiter; > - struct btrfs_qgroup *qg; > - > - qg = find_qgroup_rb(fs_info, unode->val); > - if (!qg) > - continue; > - > - ulist_reinit(tmp); > - /* XXX id not needed */ > - ulist_add(tmp, qg->qgroupid, (u64)(uintptr_t)qg, GFP_ATOMIC); > - ULIST_ITER_INIT(&tmp_uiter); > - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { > - struct btrfs_qgroup_list *glist; > - > - qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; > - if (qg->refcnt < seq) > - qg->refcnt = seq + 1; > - else > - ++qg->refcnt; > - > - list_for_each_entry(glist, &qg->groups, next_group) { > - ulist_add(tmp, glist->group->qgroupid, > - (u64)(uintptr_t)glist->group, > - GFP_ATOMIC); > - } > - } > - } > + qgroup_account_ref_step1(fs_info, roots, tmp, seq); > > /* > * step 2: walk from the new root > */ > - ulist_reinit(tmp); > - ulist_add(tmp, qgroup->qgroupid, (uintptr_t)qgroup, GFP_ATOMIC); > - ULIST_ITER_INIT(&uiter); > - while ((unode = ulist_next(tmp, &uiter))) { > - struct btrfs_qgroup *qg; > - struct btrfs_qgroup_list *glist; > - > - qg = (struct btrfs_qgroup *)(uintptr_t)unode->aux; > - if (qg->refcnt < seq) { > - /* not visited by step 1 */ > - qg->rfer += sgn * node->num_bytes; > - qg->rfer_cmpr += sgn * node->num_bytes; > - if (roots->nnodes == 0) { > - qg->excl += sgn * node->num_bytes; > - qg->excl_cmpr += sgn * node->num_bytes; > - } > - qgroup_dirty(fs_info, qg); > - } > - WARN_ON(qg->tag >= seq); > - qg->tag = seq; > - > - list_for_each_entry(glist, &qg->groups, next_group) { > - ulist_add(tmp, glist->group->qgroupid, > - (uintptr_t)glist->group, GFP_ATOMIC); > - } > - } > + qgroup_account_ref_step2(fs_info, roots, tmp, seq, sgn, > + node->num_bytes, qgroup); > > /* > * step 3: walk again from old refs > */ > - ULIST_ITER_INIT(&uiter); > - while ((unode = ulist_next(roots, &uiter))) { > - struct btrfs_qgroup *qg; > - struct ulist_node *tmp_unode; > - struct ulist_iterator tmp_uiter; > - > - qg = find_qgroup_rb(fs_info, unode->val); > - if (!qg) > - continue; > - > - ulist_reinit(tmp); > - ulist_add(tmp, qg->qgroupid, (uintptr_t)qg, GFP_ATOMIC); > - ULIST_ITER_INIT(&tmp_uiter); > - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { > - struct btrfs_qgroup_list *glist; > - > - qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; > - if (qg->tag == seq) > - continue; > - > - if (qg->refcnt - seq == roots->nnodes) { > - qg->excl -= sgn * node->num_bytes; > - qg->excl_cmpr -= sgn * node->num_bytes; > - qgroup_dirty(fs_info, qg); > - } > - > - list_for_each_entry(glist, &qg->groups, next_group) { > - ulist_add(tmp, glist->group->qgroupid, > - (uintptr_t)glist->group, > - GFP_ATOMIC); > - } > - } > - } > + qgroup_account_ref_step3(fs_info, roots, tmp, seq, sgn, > + node->num_bytes); > ret = 0; > unlock: > spin_unlock(&fs_info->qgroup_lock); -- 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
On Tue, April 16, 2013 at 11:20 (+0200), Wang Shilong wrote: > Hello Jan, > >> The function is separated into a preparation part and the three accounting >> steps mentioned in the qgroups documentation. The goal is to make steps two >> and three usable by the rescan functionality. A side effect is that the >> function is restructured into readable subunits. > > > How about renaming the three functions like: > > 1> qgroup_walk_old_roots() > 2> qgroup_walk_new_root() > 3> qgroup_rewalk_old_root() > > I'd like this function to be meaningful, but not just step1,2,3. > Maybe you can think out better function name. I'd like to keep it like 1, 2, 3, because that matches the documentation in the qgroup pdf and the code has always been documented in those three steps. Thanks, -Jan -- 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
Hello Jan, > On Tue, April 16, 2013 at 11:20 (+0200), Wang Shilong wrote: >> Hello Jan, >> >>> The function is separated into a preparation part and the three accounting >>> steps mentioned in the qgroups documentation. The goal is to make steps two >>> and three usable by the rescan functionality. A side effect is that the >>> function is restructured into readable subunits. >> >> How about renaming the three functions like: >> >> 1> qgroup_walk_old_roots() >> 2> qgroup_walk_new_root() >> 3> qgroup_rewalk_old_root() >> >> I'd like this function to be meaningful, but not just step1,2,3. >> Maybe you can think out better function name. > > I'd like to keep it like 1, 2, 3, because that matches the documentation in the > qgroup pdf and the code has always been documented in those three steps. Oh, Yes, i have read the pdf carefully. I think the pdf document it three steps just to make it clear that we need 3 steps. But static checker may want to know what is 3 steps just by the function name but not to read the pdf. In fact the tree steps are just do: 1>walk old roots 2>walk new root 3>rewalk old root So i think rename the function like these will make things better. ^_^ Thanks, Wang > > Thanks, > -Jan > -- > 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
On Tue, Apr 16, 2013 at 05:56:05PM +0800, Wang Shilong wrote: > But static checker may want to know what is 3 steps > just by the function name but not to read the pdf. I'm curious what static checker you mean and how a function name helps there. thanks, david -- 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
Hi David, > On Tue, Apr 16, 2013 at 05:56:05PM +0800, Wang Shilong wrote: >> But static checker may want to know what is 3 steps >> just by the function name but not to read the pdf. > > I'm curious what static checker you mean and how a function name helps > there. I mean that other developers will get more information by the function name. In fact, the tree steps they do more things.So maybe the function name of mine is not good enough. But i really hope that a meaningful function name gives more information. I don't insist on this anyway ^_^ Thanks, Wang > > thanks, > david > -- > 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
On Tue, Apr 16, 2013 at 06:47:15PM +0800, Wang Shilong wrote:
> I mean that other developers will get more information by the function name.
I don't dare to suggest putting a comment before the functions
david
--
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
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c index b44124d..c38a0c5 100644 --- a/fs/btrfs/qgroup.c +++ b/fs/btrfs/qgroup.c @@ -1075,6 +1075,122 @@ int btrfs_qgroup_record_ref(struct btrfs_trans_handle *trans, return 0; } +static void qgroup_account_ref_step1(struct btrfs_fs_info *fs_info, + struct ulist *roots, struct ulist *tmp, + u64 seq) +{ + struct ulist_node *unode; + struct ulist_iterator uiter; + struct ulist_node *tmp_unode; + struct ulist_iterator tmp_uiter; + struct btrfs_qgroup *qg; + + ULIST_ITER_INIT(&uiter); + while ((unode = ulist_next(roots, &uiter))) { + qg = find_qgroup_rb(fs_info, unode->val); + if (!qg) + continue; + + ulist_reinit(tmp); + /* XXX id not needed */ + ulist_add(tmp, qg->qgroupid, (u64)(uintptr_t)qg, GFP_ATOMIC); + ULIST_ITER_INIT(&tmp_uiter); + while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { + struct btrfs_qgroup_list *glist; + + qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; + if (qg->refcnt < seq) + qg->refcnt = seq + 1; + else + ++qg->refcnt; + + list_for_each_entry(glist, &qg->groups, next_group) { + ulist_add(tmp, glist->group->qgroupid, + (u64)(uintptr_t)glist->group, + GFP_ATOMIC); + } + } + } +} + +static void qgroup_account_ref_step2(struct btrfs_fs_info *fs_info, + struct ulist *roots, struct ulist *tmp, + u64 seq, int sgn, u64 num_bytes, + struct btrfs_qgroup *qgroup) +{ + struct ulist_node *unode; + struct ulist_iterator uiter; + struct btrfs_qgroup *qg; + struct btrfs_qgroup_list *glist; + + ulist_reinit(tmp); + ulist_add(tmp, qgroup->qgroupid, (uintptr_t)qgroup, GFP_ATOMIC); + + ULIST_ITER_INIT(&uiter); + while ((unode = ulist_next(tmp, &uiter))) { + + qg = (struct btrfs_qgroup *)(uintptr_t)unode->aux; + if (qg->refcnt < seq) { + /* not visited by step 1 */ + qg->rfer += sgn * num_bytes; + qg->rfer_cmpr += sgn * num_bytes; + if (roots->nnodes == 0) { + qg->excl += sgn * num_bytes; + qg->excl_cmpr += sgn * num_bytes; + } + qgroup_dirty(fs_info, qg); + } + WARN_ON(qg->tag >= seq); + qg->tag = seq; + + list_for_each_entry(glist, &qg->groups, next_group) { + ulist_add(tmp, glist->group->qgroupid, + (uintptr_t)glist->group, GFP_ATOMIC); + } + } +} + +static void qgroup_account_ref_step3(struct btrfs_fs_info *fs_info, + struct ulist *roots, struct ulist *tmp, + u64 seq, int sgn, u64 num_bytes) +{ + struct ulist_node *unode; + struct ulist_iterator uiter; + struct btrfs_qgroup *qg; + struct ulist_node *tmp_unode; + struct ulist_iterator tmp_uiter; + + ULIST_ITER_INIT(&uiter); + while ((unode = ulist_next(roots, &uiter))) { + qg = find_qgroup_rb(fs_info, unode->val); + if (!qg) + continue; + + ulist_reinit(tmp); + ulist_add(tmp, qg->qgroupid, (uintptr_t)qg, GFP_ATOMIC); + ULIST_ITER_INIT(&tmp_uiter); + while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { + struct btrfs_qgroup_list *glist; + + qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; + if (qg->tag == seq) + continue; + + if (qg->refcnt - seq == roots->nnodes) { + qg->excl -= sgn * num_bytes; + qg->excl_cmpr -= sgn * num_bytes; + qgroup_dirty(fs_info, qg); + } + + list_for_each_entry(glist, &qg->groups, next_group) { + ulist_add(tmp, glist->group->qgroupid, + (uintptr_t)glist->group, + GFP_ATOMIC); + } + } + } +} + /* * 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 @@ -1090,10 +1206,8 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, struct btrfs_root *quota_root; u64 ref_root; struct btrfs_qgroup *qgroup; - struct ulist_node *unode; struct ulist *roots = NULL; struct ulist *tmp = NULL; - struct ulist_iterator uiter; u64 seq; int ret = 0; int sgn; @@ -1175,103 +1289,19 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, seq = fs_info->qgroup_seq; fs_info->qgroup_seq += roots->nnodes + 1; /* max refcnt */ - ULIST_ITER_INIT(&uiter); - while ((unode = ulist_next(roots, &uiter))) { - struct ulist_node *tmp_unode; - struct ulist_iterator tmp_uiter; - struct btrfs_qgroup *qg; - - qg = find_qgroup_rb(fs_info, unode->val); - if (!qg) - continue; - - ulist_reinit(tmp); - /* XXX id not needed */ - ulist_add(tmp, qg->qgroupid, (u64)(uintptr_t)qg, GFP_ATOMIC); - ULIST_ITER_INIT(&tmp_uiter); - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { - struct btrfs_qgroup_list *glist; - - qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; - if (qg->refcnt < seq) - qg->refcnt = seq + 1; - else - ++qg->refcnt; - - list_for_each_entry(glist, &qg->groups, next_group) { - ulist_add(tmp, glist->group->qgroupid, - (u64)(uintptr_t)glist->group, - GFP_ATOMIC); - } - } - } + qgroup_account_ref_step1(fs_info, roots, tmp, seq); /* * step 2: walk from the new root */ - ulist_reinit(tmp); - ulist_add(tmp, qgroup->qgroupid, (uintptr_t)qgroup, GFP_ATOMIC); - ULIST_ITER_INIT(&uiter); - while ((unode = ulist_next(tmp, &uiter))) { - struct btrfs_qgroup *qg; - struct btrfs_qgroup_list *glist; - - qg = (struct btrfs_qgroup *)(uintptr_t)unode->aux; - if (qg->refcnt < seq) { - /* not visited by step 1 */ - qg->rfer += sgn * node->num_bytes; - qg->rfer_cmpr += sgn * node->num_bytes; - if (roots->nnodes == 0) { - qg->excl += sgn * node->num_bytes; - qg->excl_cmpr += sgn * node->num_bytes; - } - qgroup_dirty(fs_info, qg); - } - WARN_ON(qg->tag >= seq); - qg->tag = seq; - - list_for_each_entry(glist, &qg->groups, next_group) { - ulist_add(tmp, glist->group->qgroupid, - (uintptr_t)glist->group, GFP_ATOMIC); - } - } + qgroup_account_ref_step2(fs_info, roots, tmp, seq, sgn, + node->num_bytes, qgroup); /* * step 3: walk again from old refs */ - ULIST_ITER_INIT(&uiter); - while ((unode = ulist_next(roots, &uiter))) { - struct btrfs_qgroup *qg; - struct ulist_node *tmp_unode; - struct ulist_iterator tmp_uiter; - - qg = find_qgroup_rb(fs_info, unode->val); - if (!qg) - continue; - - ulist_reinit(tmp); - ulist_add(tmp, qg->qgroupid, (uintptr_t)qg, GFP_ATOMIC); - ULIST_ITER_INIT(&tmp_uiter); - while ((tmp_unode = ulist_next(tmp, &tmp_uiter))) { - struct btrfs_qgroup_list *glist; - - qg = (struct btrfs_qgroup *)(uintptr_t)tmp_unode->aux; - if (qg->tag == seq) - continue; - - if (qg->refcnt - seq == roots->nnodes) { - qg->excl -= sgn * node->num_bytes; - qg->excl_cmpr -= sgn * node->num_bytes; - qgroup_dirty(fs_info, qg); - } - - list_for_each_entry(glist, &qg->groups, next_group) { - ulist_add(tmp, glist->group->qgroupid, - (uintptr_t)glist->group, - GFP_ATOMIC); - } - } - } + qgroup_account_ref_step3(fs_info, roots, tmp, seq, sgn, + node->num_bytes); ret = 0; unlock: spin_unlock(&fs_info->qgroup_lock);
The function is separated into a preparation part and the three accounting steps mentioned in the qgroups documentation. The goal is to make steps two and three usable by the rescan functionality. A side effect is that the function is restructured into readable subunits. Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net> --- fs/btrfs/qgroup.c | 212 ++++++++++++++++++++++++++++++----------------------- 1 files changed, 121 insertions(+), 91 deletions(-)