Mark Fasheh wrote on 2015/11/03 15:56 -0800:
On Mon, Nov 02, 2015 at 09:59:01AM +0800, Qu Wenruo wrote:
Mark Fasheh wrote on 2015/09/22 13:15 -0700:
Commit 0ed4792 ('btrfs: qgroup: Switch to new extent-oriented qgroup
mechanism.') removed our qgroup accounting during
btrfs_drop_snapshot(). Predictably, this results in qgroup numbers
going bad shortly after a snapshot is removed.
Fix this by adding a dirty extent record when we encounter extents during
our shared subtree walk. This effectively restores the functionality we had
with the original shared subtree walking code in 1152651 (btrfs: qgroup:
account shared subtrees during snapshot delete).
The idea with the original patch (and this one) is that shared subtrees can
get skipped during drop_snapshot. The shared subtree walk then allows us a
chance to visit those extents and add them to the qgroup work for later
processing. This ultimately makes the accounting for drop snapshot work.
The new qgroup code nicely handles all the other extents during the tree
walk via the ref dec/inc functions so we don't have to add actions beyond
what we had originally.
Signed-off-by: Mark Fasheh <mfasheh@xxxxxxx>
Hi Mark,
Despite the performance regression reported from Stefan Priebe,
there is another problem, I'll comment inlined below.
---
fs/btrfs/extent-tree.c | 41 ++++++++++++++++++++++++++++++++++-------
1 file changed, 34 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3a70e6c..89be620 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -7757,17 +7757,37 @@ reada:
}
/*
- * TODO: Modify related function to add related node/leaf to dirty_extent_root,
- * for later qgroup accounting.
- *
- * Current, this function does nothing.
+ * These may not be seen by the usual inc/dec ref code so we have to
+ * add them here.
*/
+static int record_one_subtree_extent(struct btrfs_trans_handle *trans,
+ struct btrfs_root *root, u64 bytenr,
+ u64 num_bytes)
+{
+ struct btrfs_qgroup_extent_record *qrecord;
+ struct btrfs_delayed_ref_root *delayed_refs;
+
+ qrecord = kmalloc(sizeof(*qrecord), GFP_NOFS);
+ if (!qrecord)
+ return -ENOMEM;
+
+ qrecord->bytenr = bytenr;
+ qrecord->num_bytes = num_bytes;
+ qrecord->old_roots = NULL;
+
+ delayed_refs = &trans->transaction->delayed_refs;
+ if (btrfs_qgroup_insert_dirty_extent(delayed_refs, qrecord))
+ kfree(qrecord);
1) Unprotected dirty_extent_root.
Unfortunately, btrfs_qgroup_insert_dirty_exntet() is not protected
by any lock/mutex.
And I'm sorry not to add comment about that.
In fact, btrfs_qgroup_insert_dirty_extent should always be used with
delayed_refs->lock hold.
Just like add_delayed_ref_head(), where every caller of
add_delayed_ref_head() holds delayed_refs->lock.
So here you will nned to hold delayed_refs->lock.
Ok, thanks for pointing this out. To your knowledge is there any reasion why
the followup patch shouldn't just wrap the call to
btrfs_qgroup_insert_dirty_extent() in the correct lock?
Just as explained in previous reply, all caller (add_delayed_ref_head)
has the correct lock(delayed_refs->lock).
So I didn't see the need to add a new lock for it or wrap the new lock
into insert_dirty_extent() at that time.
(Just lock delayed_refs->lock will cause deadlock)
But now since the function is called elsewhere, I'm OK not to reuse
delayed_ref->lock, add a new lock and integrate it into
insert_dirty_extent()
Thanks,
Qu
2) Performance regression.(Reported by Stefan Priebe)
The performance regression is not caused by your codes, at least not
completely.
It's my fault not adding enough comment for insert_dirty_extent()
function. (just like 1, I must say I'm a bad reviewer until there is
bug report)
As I was only expecting it called inside add_delayed_ref_head(),
and caller of add_delayed_ref_head() has judged whether qgroup is
enabled before calling add_delayed_ref_head().
So for qgroup disabled case, insert_dirty_extent() won't ever be called.
As a result, if you want to call btrfs_qgroup_insert_dirty_extent()
out of add_delayed_ref_head(), you will need to handle the
delayed_refs->lock and judge whether qgroup is enabled.
Ok, so callers of btrfs_qgroup_insert_dirty_extent() also have to check
whether qgroups are enabled.
BTW, if it's OK for you, you can also further improve the
performance of qgroup by using kmem_cache for struct
btrfs_qgroup_extent_record.
I assume the kmalloc() may be one performance hot spot considering
the amount it called in qgroup enabled case.
We're reading disk in that case, I hardly think the small kmalloc() matters.
--Mark
--
Mark Fasheh
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
