Re: [PATCH] Btrfs: stop using GFP_ATOMIC for the tree mod log allocations
On Mon, July 01, 2013 at 22:25 (+0200), Josef Bacik wrote:
> Previously we held the tree mod lock when adding stuff because we use it to
> check and see if we truly do want to track tree modifications. This is
> admirable, but GFP_ATOMIC in a critical area that is going to get hit pretty
> hard and often is not nice. So instead do our basic checks to see if we don't
> need to track modifications, and if those pass then do our allocation, and
> then
> when we go to insert the new modification check if we still care, and if we
> don't just free up our mod and return. Otherwise we're good to go and we can
> carry on. Thanks,
I'd like to look at a side-by-side diff of that patch in my editor. However, it
does not apply to your current master branch, and git even refuses trying a
3-way-merge because your "Repository lacks necessary blobs". Can you please push
something?
Thanks,
-Jan
> Signed-off-by: Josef Bacik
> ---
> fs/btrfs/ctree.c | 161
> ++
> 1 files changed, 54 insertions(+), 107 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 127e1fd..fff08f9 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -484,8 +484,27 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info,
> struct tree_mod_elem *tm)
> struct rb_node **new;
> struct rb_node *parent = NULL;
> struct tree_mod_elem *cur;
> + int ret = 0;
> +
> + BUG_ON(!tm);
> +
> + tree_mod_log_write_lock(fs_info);
> + if (list_empty(&fs_info->tree_mod_seq_list)) {
> + tree_mod_log_write_unlock(fs_info);
> + /*
> + * Ok we no longer care about logging modifications, free up tm
> + * and return 0. Any callers shouldn't be using tm after
> + * calling tree_mod_log_insert, but if they do we can just
> + * change this to return a special error code to let the callers
> + * do their own thing.
> + */
> + kfree(tm);
> + return 0;
> + }
>
> - BUG_ON(!tm || !tm->seq);
> + spin_lock(&fs_info->tree_mod_seq_lock);
> + tm->seq = btrfs_inc_tree_mod_seq_minor(fs_info);
> + spin_unlock(&fs_info->tree_mod_seq_lock);
>
> tm_root = &fs_info->tree_mod_log;
> new = &tm_root->rb_node;
> @@ -501,14 +520,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info,
> struct tree_mod_elem *tm)
> else if (cur->seq > tm->seq)
> new = &((*new)->rb_right);
> else {
> + ret = -EEXIST;
> kfree(tm);
> - return -EEXIST;
> + goto out;
> }
> }
>
> rb_link_node(&tm->node, parent, new);
> rb_insert_color(&tm->node, tm_root);
> - return 0;
> +out:
> + tree_mod_log_write_unlock(fs_info);
> + return ret;
> }
>
> /*
> @@ -524,55 +546,17 @@ static inline int tree_mod_dont_log(struct
> btrfs_fs_info *fs_info,
> return 1;
> if (eb && btrfs_header_level(eb) == 0)
> return 1;
> -
> - tree_mod_log_write_lock(fs_info);
> - if (list_empty(&fs_info->tree_mod_seq_list)) {
> - /*
> - * someone emptied the list while we were waiting for the lock.
> - * we must not add to the list when no blocker exists.
> - */
> - tree_mod_log_write_unlock(fs_info);
> - return 1;
> - }
> -
> return 0;
> }
>
> -/*
> - * This allocates memory and gets a tree modification sequence number.
> - *
> - * Returns <0 on error.
> - * Returns >0 (the added sequence number) on success.
> - */
> -static inline struct tree_mod_elem *
> -tree_mod_alloc(struct btrfs_fs_info *fs_info, gfp_t flags)
> -{
> - struct tree_mod_elem *tm;
> -
> - /*
> - * once we switch from spin locks to something different, we should
> - * honor the flags parameter here.
> - */
> - tm = *tm_ret = kzalloc(sizeof(*tm), GFP_ATOMIC);
> - if (!tm)
> - return NULL;
> -
> - spin_lock(&fs_info->tree_mod_seq_lock);
> - tm->seq = btrfs_inc_tree_mod_seq_minor(fs_info);
> - spin_unlock(&fs_info->tree_mod_seq_lock);
> -
> - return tm;
> -}
> -
> static inline int
> __tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
> struct extent_buffer *eb, int slot,
> enum mod_log_op op, gfp_t flags)
> {
> - int ret;
> struct tree_mod_elem *tm;
>
> - tm = tree_mod_alloc(fs_info, flags);
> + tm = kzalloc(sizeof(*tm), flags);
> if (!tm)
> return -ENOMEM;
>
> @@ -589,34 +573,14 @@ __tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
> }
>
> static noinline int
> -tree_mod_log_insert_key_mask(struct btrfs_fs_info *fs_info,
> - struct extent_buffer *eb, int slot,
> - enum mod_log_op op, gfp_
[PATCH] Btrfs: stop using GFP_ATOMIC for the tree mod log allocations
Previously we held the tree mod lock when adding stuff because we use it to
check and see if we truly do want to track tree modifications. This is
admirable, but GFP_ATOMIC in a critical area that is going to get hit pretty
hard and often is not nice. So instead do our basic checks to see if we don't
need to track modifications, and if those pass then do our allocation, and then
when we go to insert the new modification check if we still care, and if we
don't just free up our mod and return. Otherwise we're good to go and we can
carry on. Thanks,
Signed-off-by: Josef Bacik
---
fs/btrfs/ctree.c | 161 ++
1 files changed, 54 insertions(+), 107 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 127e1fd..fff08f9 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -484,8 +484,27 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info,
struct tree_mod_elem *tm)
struct rb_node **new;
struct rb_node *parent = NULL;
struct tree_mod_elem *cur;
+ int ret = 0;
+
+ BUG_ON(!tm);
+
+ tree_mod_log_write_lock(fs_info);
+ if (list_empty(&fs_info->tree_mod_seq_list)) {
+ tree_mod_log_write_unlock(fs_info);
+ /*
+* Ok we no longer care about logging modifications, free up tm
+* and return 0. Any callers shouldn't be using tm after
+* calling tree_mod_log_insert, but if they do we can just
+* change this to return a special error code to let the callers
+* do their own thing.
+*/
+ kfree(tm);
+ return 0;
+ }
- BUG_ON(!tm || !tm->seq);
+ spin_lock(&fs_info->tree_mod_seq_lock);
+ tm->seq = btrfs_inc_tree_mod_seq_minor(fs_info);
+ spin_unlock(&fs_info->tree_mod_seq_lock);
tm_root = &fs_info->tree_mod_log;
new = &tm_root->rb_node;
@@ -501,14 +520,17 @@ __tree_mod_log_insert(struct btrfs_fs_info *fs_info,
struct tree_mod_elem *tm)
else if (cur->seq > tm->seq)
new = &((*new)->rb_right);
else {
+ ret = -EEXIST;
kfree(tm);
- return -EEXIST;
+ goto out;
}
}
rb_link_node(&tm->node, parent, new);
rb_insert_color(&tm->node, tm_root);
- return 0;
+out:
+ tree_mod_log_write_unlock(fs_info);
+ return ret;
}
/*
@@ -524,55 +546,17 @@ static inline int tree_mod_dont_log(struct btrfs_fs_info
*fs_info,
return 1;
if (eb && btrfs_header_level(eb) == 0)
return 1;
-
- tree_mod_log_write_lock(fs_info);
- if (list_empty(&fs_info->tree_mod_seq_list)) {
- /*
-* someone emptied the list while we were waiting for the lock.
-* we must not add to the list when no blocker exists.
-*/
- tree_mod_log_write_unlock(fs_info);
- return 1;
- }
-
return 0;
}
-/*
- * This allocates memory and gets a tree modification sequence number.
- *
- * Returns <0 on error.
- * Returns >0 (the added sequence number) on success.
- */
-static inline struct tree_mod_elem *
-tree_mod_alloc(struct btrfs_fs_info *fs_info, gfp_t flags)
-{
- struct tree_mod_elem *tm;
-
- /*
-* once we switch from spin locks to something different, we should
-* honor the flags parameter here.
-*/
- tm = *tm_ret = kzalloc(sizeof(*tm), GFP_ATOMIC);
- if (!tm)
- return NULL;
-
- spin_lock(&fs_info->tree_mod_seq_lock);
- tm->seq = btrfs_inc_tree_mod_seq_minor(fs_info);
- spin_unlock(&fs_info->tree_mod_seq_lock);
-
- return tm;
-}
-
static inline int
__tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
struct extent_buffer *eb, int slot,
enum mod_log_op op, gfp_t flags)
{
- int ret;
struct tree_mod_elem *tm;
- tm = tree_mod_alloc(fs_info, flags);
+ tm = kzalloc(sizeof(*tm), flags);
if (!tm)
return -ENOMEM;
@@ -589,34 +573,14 @@ __tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
}
static noinline int
-tree_mod_log_insert_key_mask(struct btrfs_fs_info *fs_info,
-struct extent_buffer *eb, int slot,
-enum mod_log_op op, gfp_t flags)
+tree_mod_log_insert_key(struct btrfs_fs_info *fs_info,
+ struct extent_buffer *eb, int slot,
+ enum mod_log_op op, gfp_t flags)
{
- int ret;
-
if (tree_mod_dont_log(fs_info, eb))
return 0;
- ret = __tree_mod_log_insert_key(fs_info, eb, slot, op, flags);
-
- tree_mod_log_write_unlock(fs_info);
- return ret;
-}
-
-static noinline int
-tree_mod
