* [PATCH 0/6] btrfs_search_slot cleanups
@ 2018-05-15 17:52 Liu Bo
2018-05-15 17:52 ` [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer Liu Bo
` (6 more replies)
0 siblings, 7 replies; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
These're the cleanups I made for btrfs_search_slot() and its callees.
Liu Bo (6):
Btrfs: remove superfluous free_extent_buffer
Btrfs: use more straightforward extent_buffer_uptodate
Btrfs: move get root of btrfs_search_slot to a helper
Btrfs: remove unused check of skip_locking
Btrfs: grab write lock directly if write_lock_level is the max level
Btrfs: remove always true check in unlock_up
fs/btrfs/ctree.c | 118 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 70 insertions(+), 48 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 6:40 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate Liu Bo
` (5 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
@tmp must be NULL at this point, free_extent_buffer is not needed at all.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index b3f6f300e492..9fa3d77c98d4 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2432,7 +2432,6 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
btrfs_unlock_up_safe(p, level + 1);
btrfs_set_path_blocking(p);
- free_extent_buffer(tmp);
if (p->reada != READA_NONE)
reada_for_search(fs_info, p, level, slot, key->objectid);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
2018-05-15 17:52 ` [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 6:43 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper Liu Bo
` (4 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
In read_block_for_search(), it's straightforward to use
extent_buffer_uptodate() instead since 0 is passed as parent transid to
btrfs_buffer_uptodate(), which means the check for parent transid is not
needed.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 9fa3d77c98d4..a96d308c51b8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
* and give up so that our caller doesn't loop forever
* on our EAGAINs.
*/
- if (!btrfs_buffer_uptodate(tmp, 0, 0))
+ if (!extent_buffer_uptodate(tmp))
ret = -EIO;
free_extent_buffer(tmp);
} else {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
2018-05-15 17:52 ` [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer Liu Bo
2018-05-15 17:52 ` [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 6:54 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 4/6] Btrfs: remove unused check of skip_locking Liu Bo
` (3 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
It's good to have a helper instead of having all get-root details
open-coded.
The new helper locks (if necessary) and sets root node of the path.
There is no functional change in this commit.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 112 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 67 insertions(+), 45 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index a96d308c51b8..399839df7a8f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2598,6 +2598,72 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
return 0;
}
+static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
+ struct btrfs_path *p,
+ int write_lock_level)
+{
+ struct btrfs_fs_info *fs_info = root->fs_info;
+ struct extent_buffer *b;
+ int root_lock;
+ int level = 0;
+
+ /*
+ * we try very hard to do read locks on the root
+ */
+ root_lock = BTRFS_READ_LOCK;
+
+ if (p->search_commit_root) {
+ /*
+ * the commit roots are read only so we always do read locks
+ */
+ if (p->need_commit_sem)
+ down_read(&fs_info->commit_root_sem);
+ b = root->commit_root;
+ extent_buffer_get(b);
+ level = btrfs_header_level(b);
+ if (p->need_commit_sem)
+ up_read(&fs_info->commit_root_sem);
+ if (!p->skip_locking)
+ btrfs_tree_read_lock(b);
+
+ goto out;
+ }
+
+ if (p->skip_locking) {
+ b = btrfs_root_node(root);
+ level = btrfs_header_level(b);
+ goto out;
+ }
+
+ /*
+ * we don't know the level of the root node until we actually
+ * have it read locked
+ */
+ b = btrfs_read_lock_root_node(root);
+ level = btrfs_header_level(b);
+ if (level > write_lock_level)
+ goto out;
+
+ /*
+ * whoops, must trade for write lock
+ */
+ btrfs_tree_read_unlock(b);
+ free_extent_buffer(b);
+ b = btrfs_lock_root_node(root);
+ root_lock = BTRFS_WRITE_LOCK;
+ /*
+ * the level might have changed, check again
+ */
+ level = btrfs_header_level(b);
+
+out:
+ p->nodes[level] = b;
+ if (!p->skip_locking)
+ p->locks[level] = root_lock;
+ return b;
+}
+
+
/*
* btrfs_search_slot - look for a key in a tree and perform necessary
* modifications to preserve tree invariants.
@@ -2634,7 +2700,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
int err;
int level;
int lowest_unlock = 1;
- int root_lock;
/* everything at write_lock_level or lower must be write locked */
int write_lock_level = 0;
u8 lowest_level = 0;
@@ -2672,50 +2737,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
again:
prev_cmp = -1;
- /*
- * we try very hard to do read locks on the root
- */
- root_lock = BTRFS_READ_LOCK;
- level = 0;
- if (p->search_commit_root) {
- /*
- * the commit roots are read only
- * so we always do read locks
- */
- if (p->need_commit_sem)
- down_read(&fs_info->commit_root_sem);
- b = root->commit_root;
- extent_buffer_get(b);
- level = btrfs_header_level(b);
- if (p->need_commit_sem)
- up_read(&fs_info->commit_root_sem);
- if (!p->skip_locking)
- btrfs_tree_read_lock(b);
- } else {
- if (p->skip_locking) {
- b = btrfs_root_node(root);
- level = btrfs_header_level(b);
- } else {
- /* we don't know the level of the root node
- * until we actually have it read locked
- */
- b = btrfs_read_lock_root_node(root);
- level = btrfs_header_level(b);
- if (level <= write_lock_level) {
- /* whoops, must trade for write lock */
- btrfs_tree_read_unlock(b);
- free_extent_buffer(b);
- b = btrfs_lock_root_node(root);
- root_lock = BTRFS_WRITE_LOCK;
-
- /* the level might have changed, check again */
- level = btrfs_header_level(b);
- }
- }
- }
- p->nodes[level] = b;
- if (!p->skip_locking)
- p->locks[level] = root_lock;
+ b = btrfs_search_slot_get_root(root, p, write_lock_level);
while (b) {
level = btrfs_header_level(b);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/6] Btrfs: remove unused check of skip_locking
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
` (2 preceding siblings ...)
2018-05-15 17:52 ` [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 7:03 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level Liu Bo
` (2 subsequent siblings)
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
The check is superfluous since all of callers who set search_for_commit
also have skip_locking set.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 399839df7a8f..cf34eca41d4e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
level = btrfs_header_level(b);
if (p->need_commit_sem)
up_read(&fs_info->commit_root_sem);
- if (!p->skip_locking)
- btrfs_tree_read_lock(b);
goto out;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
` (3 preceding siblings ...)
2018-05-15 17:52 ` [PATCH 4/6] Btrfs: remove unused check of skip_locking Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 7:09 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 6/6] Btrfs: remove always true check in unlock_up Liu Bo
2018-05-15 18:15 ` [PATCH 0/6] btrfs_search_slot cleanups David Sterba
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
is the max level, and we should grab write lock of root node from the very
beginning.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cf34eca41d4e..f7c3f581f647 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2633,20 +2633,23 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
goto out;
}
- /*
- * we don't know the level of the root node until we actually
- * have it read locked
- */
- b = btrfs_read_lock_root_node(root);
- level = btrfs_header_level(b);
- if (level > write_lock_level)
- goto out;
+ if (write_lock_level < BTRFS_MAX_LEVEL) {
+ /*
+ * we don't know the level of the root node until we actually
+ * have it read locked
+ */
+ b = btrfs_read_lock_root_node(root);
+ level = btrfs_header_level(b);
+ if (level > write_lock_level)
+ goto out;
+
+ /*
+ * whoops, must trade for write lock
+ */
+ btrfs_tree_read_unlock(b);
+ free_extent_buffer(b);
+ }
- /*
- * whoops, must trade for write lock
- */
- btrfs_tree_read_unlock(b);
- free_extent_buffer(b);
b = btrfs_lock_root_node(root);
root_lock = BTRFS_WRITE_LOCK;
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 6/6] Btrfs: remove always true check in unlock_up
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
` (4 preceding siblings ...)
2018-05-15 17:52 ` [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level Liu Bo
@ 2018-05-15 17:52 ` Liu Bo
2018-05-16 6:47 ` Nikolay Borisov
2018-05-15 18:15 ` [PATCH 0/6] btrfs_search_slot cleanups David Sterba
6 siblings, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-15 17:52 UTC (permalink / raw)
To: linux-btrfs
@path->lock[i] is always true at this point.
Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
---
fs/btrfs/ctree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index f7c3f581f647..16d28a4ec54f 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2330,7 +2330,7 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
no_skips = 1;
t = path->nodes[i];
- if (i >= lowest_unlock && i > skip_level && path->locks[i]) {
+ if (i >= lowest_unlock && i > skip_level) {
btrfs_tree_unlock_rw(t, path->locks[i]);
path->locks[i] = 0;
if (write_lock_level &&
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/6] btrfs_search_slot cleanups
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
` (5 preceding siblings ...)
2018-05-15 17:52 ` [PATCH 6/6] Btrfs: remove always true check in unlock_up Liu Bo
@ 2018-05-15 18:15 ` David Sterba
6 siblings, 0 replies; 18+ messages in thread
From: David Sterba @ 2018-05-15 18:15 UTC (permalink / raw)
To: Liu Bo; +Cc: linux-btrfs
On Wed, May 16, 2018 at 01:52:02AM +0800, Liu Bo wrote:
> These're the cleanups I made for btrfs_search_slot() and its callees.
All the patches have very terse changelog and I have no idea why you
think the code change is correct so I can compare my review against
that.
It's touching code around locking that's never bad to be more
verbose and introduce a bit of the context, the simple sentence is maybe
clear to you when you go through the code, but imagine you read the
changelog in a week or month or a year. It still needs to make sense or
give enough pointers to find out from the code.
Please update and resend, thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer
2018-05-15 17:52 ` [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer Liu Bo
@ 2018-05-16 6:40 ` Nikolay Borisov
0 siblings, 0 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 6:40 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> @tmp must be NULL at this point, free_extent_buffer is not needed at all.
You need to explain that this code is executed only if
find_extent_buffer didn't return anything, meaning tmp is null,
otherwise it's hard to review this change without looking at the code.
Codewise it's correct:
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> fs/btrfs/ctree.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index b3f6f300e492..9fa3d77c98d4 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2432,7 +2432,6 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
> btrfs_unlock_up_safe(p, level + 1);
> btrfs_set_path_blocking(p);
>
> - free_extent_buffer(tmp);
> if (p->reada != READA_NONE)
> reada_for_search(fs_info, p, level, slot, key->objectid);
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate
2018-05-15 17:52 ` [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate Liu Bo
@ 2018-05-16 6:43 ` Nikolay Borisov
2018-05-18 2:09 ` Liu Bo
0 siblings, 1 reply; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 6:43 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> In read_block_for_search(), it's straightforward to use
> extent_buffer_uptodate() instead since 0 is passed as parent transid to
"instead of the more heavyweight btrfs_buffer_update"
> btrfs_buffer_uptodate(), which means the check for parent transid is not
> needed.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
Codewise LGTM:
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/ctree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 9fa3d77c98d4..a96d308c51b8 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
> * and give up so that our caller doesn't loop forever
> * on our EAGAINs.
> */
> - if (!btrfs_buffer_uptodate(tmp, 0, 0))
> + if (!extent_buffer_uptodate(tmp))
> ret = -EIO;
> free_extent_buffer(tmp);
> } else {
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 6/6] Btrfs: remove always true check in unlock_up
2018-05-15 17:52 ` [PATCH 6/6] Btrfs: remove always true check in unlock_up Liu Bo
@ 2018-05-16 6:47 ` Nikolay Borisov
0 siblings, 0 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 6:47 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> @path->lock[i] is always true at this point.
You must explain why it's true. The reason is since at the beginning of
the for loop the check is performed :
if (!path->locks[i])
break;
Codewise it's ok:
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> fs/btrfs/ctree.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index f7c3f581f647..16d28a4ec54f 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2330,7 +2330,7 @@ static noinline void unlock_up(struct btrfs_path *path, int level,
> no_skips = 1;
>
> t = path->nodes[i];
> - if (i >= lowest_unlock && i > skip_level && path->locks[i]) {
> + if (i >= lowest_unlock && i > skip_level) {
> btrfs_tree_unlock_rw(t, path->locks[i]);
> path->locks[i] = 0;
> if (write_lock_level &&
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper
2018-05-15 17:52 ` [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper Liu Bo
@ 2018-05-16 6:54 ` Nikolay Borisov
0 siblings, 0 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 6:54 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> It's good to have a helper instead of having all get-root details
> open-coded.
>
> The new helper locks (if necessary) and sets root node of the path.
>
> There is no functional change in this commit.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
Codewise it looks ok, you've inverted the conditional so as to give a
more linear flow of the code. I've checked all the various branches and
they seem semantically identical to the open coded version. One minor
nit is that I don't think this is the most suitable name for this
function but I don't really have a better suggestions which would be
more specific.
For the code (the changelog should be more explicit):
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/ctree.c | 112 +++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 67 insertions(+), 45 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index a96d308c51b8..399839df7a8f 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2598,6 +2598,72 @@ int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path,
> return 0;
> }
>
> +static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
> + struct btrfs_path *p,
> + int write_lock_level)
> +{
> + struct btrfs_fs_info *fs_info = root->fs_info;
> + struct extent_buffer *b;
> + int root_lock;
> + int level = 0;
> +
> + /*
> + * we try very hard to do read locks on the root
> + */
> + root_lock = BTRFS_READ_LOCK;
> +
> + if (p->search_commit_root) {
> + /*
> + * the commit roots are read only so we always do read locks
> + */
> + if (p->need_commit_sem)
> + down_read(&fs_info->commit_root_sem);
> + b = root->commit_root;
> + extent_buffer_get(b);
> + level = btrfs_header_level(b);
> + if (p->need_commit_sem)
> + up_read(&fs_info->commit_root_sem);
> + if (!p->skip_locking)
> + btrfs_tree_read_lock(b);
> +
> + goto out;
> + }
> +
> + if (p->skip_locking) {
> + b = btrfs_root_node(root);
> + level = btrfs_header_level(b);
> + goto out;
> + }
> +
> + /*
> + * we don't know the level of the root node until we actually
> + * have it read locked
> + */
> + b = btrfs_read_lock_root_node(root);
> + level = btrfs_header_level(b);
> + if (level > write_lock_level)
> + goto out;
> +
> + /*
> + * whoops, must trade for write lock
> + */
> + btrfs_tree_read_unlock(b);
> + free_extent_buffer(b);
> + b = btrfs_lock_root_node(root);
> + root_lock = BTRFS_WRITE_LOCK;
> + /*
> + * the level might have changed, check again
> + */
> + level = btrfs_header_level(b);
> +
> +out:
> + p->nodes[level] = b;
> + if (!p->skip_locking)
> + p->locks[level] = root_lock;
> + return b;
> +}
> +
> +
> /*
> * btrfs_search_slot - look for a key in a tree and perform necessary
> * modifications to preserve tree invariants.
> @@ -2634,7 +2700,6 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> int err;
> int level;
> int lowest_unlock = 1;
> - int root_lock;
> /* everything at write_lock_level or lower must be write locked */
> int write_lock_level = 0;
> u8 lowest_level = 0;
> @@ -2672,50 +2737,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>
> again:
> prev_cmp = -1;
> - /*
> - * we try very hard to do read locks on the root
> - */
> - root_lock = BTRFS_READ_LOCK;
> - level = 0;
> - if (p->search_commit_root) {
> - /*
> - * the commit roots are read only
> - * so we always do read locks
> - */
> - if (p->need_commit_sem)
> - down_read(&fs_info->commit_root_sem);
> - b = root->commit_root;
> - extent_buffer_get(b);
> - level = btrfs_header_level(b);
> - if (p->need_commit_sem)
> - up_read(&fs_info->commit_root_sem);
> - if (!p->skip_locking)
> - btrfs_tree_read_lock(b);
> - } else {
> - if (p->skip_locking) {
> - b = btrfs_root_node(root);
> - level = btrfs_header_level(b);
> - } else {
> - /* we don't know the level of the root node
> - * until we actually have it read locked
> - */
> - b = btrfs_read_lock_root_node(root);
> - level = btrfs_header_level(b);
> - if (level <= write_lock_level) {
> - /* whoops, must trade for write lock */
> - btrfs_tree_read_unlock(b);
> - free_extent_buffer(b);
> - b = btrfs_lock_root_node(root);
> - root_lock = BTRFS_WRITE_LOCK;
> -
> - /* the level might have changed, check again */
> - level = btrfs_header_level(b);
> - }
> - }
> - }
> - p->nodes[level] = b;
> - if (!p->skip_locking)
> - p->locks[level] = root_lock;
> + b = btrfs_search_slot_get_root(root, p, write_lock_level);
>
> while (b) {
> level = btrfs_header_level(b);
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking
2018-05-15 17:52 ` [PATCH 4/6] Btrfs: remove unused check of skip_locking Liu Bo
@ 2018-05-16 7:03 ` Nikolay Borisov
2018-05-16 7:06 ` Qu Wenruo
2018-05-18 2:55 ` Liu Bo
0 siblings, 2 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 7:03 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> The check is superfluous since all of callers who set search_for_commit
> also have skip_locking set.
This is false. For example btrfs_qgroup_rescan_worker sets
search_commit_root = 1 but doesn't set skip locking. So either your
assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
that (which seems more likely).
I think this change necessitates either:
a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
or
b) Unconditionally setting ->skip_locking if we have set
search_commit_root and removing opencoded set of skip_locking alongside
search_commit_root.
I think b) is the better option since it provides "fire and forget"
semantics when you want to search the commit root, since it's only read
only.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> fs/btrfs/ctree.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 399839df7a8f..cf34eca41d4e 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
> level = btrfs_header_level(b);
> if (p->need_commit_sem)
> up_read(&fs_info->commit_root_sem);
> - if (!p->skip_locking)
> - btrfs_tree_read_lock(b);
>
> goto out;
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking
2018-05-16 7:03 ` Nikolay Borisov
@ 2018-05-16 7:06 ` Qu Wenruo
2018-05-18 2:55 ` Liu Bo
1 sibling, 0 replies; 18+ messages in thread
From: Qu Wenruo @ 2018-05-16 7:06 UTC (permalink / raw)
To: Nikolay Borisov, Liu Bo, linux-btrfs
On 2018年05月16日 15:03, Nikolay Borisov wrote:
>
>
> On 15.05.2018 20:52, Liu Bo wrote:
>> The check is superfluous since all of callers who set search_for_commit
>> also have skip_locking set.
>
> This is false. For example btrfs_qgroup_rescan_worker sets
> search_commit_root = 1 but doesn't set skip locking. So either your
> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
> that (which seems more likely).
Liu's assumption on all commit root searcher don't need lock looks good
to me, thus qgroup code could set skip_locking.
>
> I think this change necessitates either:
>
> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
>
> or
>
> b) Unconditionally setting ->skip_locking if we have set
> search_commit_root and removing opencoded set of skip_locking alongside
> search_commit_root.
>
> I think b) is the better option since it provides "fire and forget"
> semantics when you want to search the commit root, since it's only read
> only.
b) looks better to me.
Thanks,
Qu
>
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>> fs/btrfs/ctree.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 399839df7a8f..cf34eca41d4e 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>> level = btrfs_header_level(b);
>> if (p->need_commit_sem)
>> up_read(&fs_info->commit_root_sem);
>> - if (!p->skip_locking)
>> - btrfs_tree_read_lock(b);
>>
>> goto out;
>> }
>>
> --
> 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] 18+ messages in thread
* Re: [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level
2018-05-15 17:52 ` [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level Liu Bo
@ 2018-05-16 7:09 ` Nikolay Borisov
0 siblings, 0 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-16 7:09 UTC (permalink / raw)
To: Liu Bo, linux-btrfs
On 15.05.2018 20:52, Liu Bo wrote:
> In case of (cow && (p->keep_locks || p->lowest_level)), write_lock_level
> is the max level, and we should grab write lock of root node from the very
> beginning.
THis needs to be expanded to explain what are the adverse effects (if
any) without this commit. And then explain how this commit actually
improve things.
>
> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
> ---
> fs/btrfs/ctree.c | 29 ++++++++++++++++-------------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index cf34eca41d4e..f7c3f581f647 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2633,20 +2633,23 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
> goto out;
> }
>
> - /*
> - * we don't know the level of the root node until we actually
> - * have it read locked
> - */
> - b = btrfs_read_lock_root_node(root);
> - level = btrfs_header_level(b);
> - if (level > write_lock_level)
> - goto out;
> + if (write_lock_level < BTRFS_MAX_LEVEL) {
> + /*
> + * we don't know the level of the root node until we actually
> + * have it read locked
> + */
> + b = btrfs_read_lock_root_node(root);
> + level = btrfs_header_level(b);
> + if (level > write_lock_level)
> + goto out;
> +
> + /*
> + * whoops, must trade for write lock
> + */
> + btrfs_tree_read_unlock(b);
> + free_extent_buffer(b);
> + }
Here just seems you are adding 1 extra branch so more context please.
>
> - /*
> - * whoops, must trade for write lock
> - */
> - btrfs_tree_read_unlock(b);
> - free_extent_buffer(b);
> b = btrfs_lock_root_node(root);
> root_lock = BTRFS_WRITE_LOCK;
> /*
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate
2018-05-16 6:43 ` Nikolay Borisov
@ 2018-05-18 2:09 ` Liu Bo
0 siblings, 0 replies; 18+ messages in thread
From: Liu Bo @ 2018-05-18 2:09 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Liu Bo, linux-btrfs
On Wed, May 16, 2018 at 2:43 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 15.05.2018 20:52, Liu Bo wrote:
>> In read_block_for_search(), it's straightforward to use
>> extent_buffer_uptodate() instead since 0 is passed as parent transid to
>
> "instead of the more heavyweight btrfs_buffer_update"
>
I don't think it's about heavyweight, they're actually equivalent in this case.
I just want to reduce the burden of reading these code as
verify_parent_transid() really has some corner cases to think about.
>> btrfs_buffer_uptodate(), which means the check for parent transid is not
>> needed.
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>
> Codewise LGTM:
>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Thanks for taking a look at this.
thanks,
liubo
>> ---
>> fs/btrfs/ctree.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 9fa3d77c98d4..a96d308c51b8 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2445,7 +2445,7 @@ noinline void btrfs_unlock_up_safe(struct btrfs_path *path, int level)
>> * and give up so that our caller doesn't loop forever
>> * on our EAGAINs.
>> */
>> - if (!btrfs_buffer_uptodate(tmp, 0, 0))
>> + if (!extent_buffer_uptodate(tmp))
>> ret = -EIO;
>> free_extent_buffer(tmp);
>> } else {
>>
> --
> 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] 18+ messages in thread
* Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking
2018-05-16 7:03 ` Nikolay Borisov
2018-05-16 7:06 ` Qu Wenruo
@ 2018-05-18 2:55 ` Liu Bo
2018-05-18 5:36 ` Nikolay Borisov
1 sibling, 1 reply; 18+ messages in thread
From: Liu Bo @ 2018-05-18 2:55 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Liu Bo, linux-btrfs
On Wed, May 16, 2018 at 3:03 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 15.05.2018 20:52, Liu Bo wrote:
>> The check is superfluous since all of callers who set search_for_commit
>> also have skip_locking set.
>
> This is false. For example btrfs_qgroup_rescan_worker sets
> search_commit_root = 1 but doesn't set skip locking. So either your
> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
> that (which seems more likely).
>
I'm a bit confused as I didn't see btrfs_qgroup_rescan_worker() set
path->search_commit_root.
thanks,
liubo
> I think this change necessitates either:
>
> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
>
> or
>
> b) Unconditionally setting ->skip_locking if we have set
> search_commit_root and removing opencoded set of skip_locking alongside
> search_commit_root.
>
> I think b) is the better option since it provides "fire and forget"
> semantics when you want to search the commit root, since it's only read
> only.
>
>>
>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>> ---
>> fs/btrfs/ctree.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 399839df7a8f..cf34eca41d4e 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>> level = btrfs_header_level(b);
>> if (p->need_commit_sem)
>> up_read(&fs_info->commit_root_sem);
>> - if (!p->skip_locking)
>> - btrfs_tree_read_lock(b);
>>
>> goto out;
>> }
>>
> --
> 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] 18+ messages in thread
* Re: [PATCH 4/6] Btrfs: remove unused check of skip_locking
2018-05-18 2:55 ` Liu Bo
@ 2018-05-18 5:36 ` Nikolay Borisov
0 siblings, 0 replies; 18+ messages in thread
From: Nikolay Borisov @ 2018-05-18 5:36 UTC (permalink / raw)
To: Liu Bo; +Cc: Liu Bo, linux-btrfs
On 18.05.2018 05:55, Liu Bo wrote:
> On Wed, May 16, 2018 at 3:03 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>
>> On 15.05.2018 20:52, Liu Bo wrote:
>>> The check is superfluous since all of callers who set search_for_commit
>>> also have skip_locking set.
>>
>> This is false. For example btrfs_qgroup_rescan_worker sets
>> search_commit_root = 1 but doesn't set skip locking. So either your
>> assumption is wrong or the code in btrfs_qgroup_rescan_worker has missed
>> that (which seems more likely).
>>
>
> I'm a bit confused as I didn't see btrfs_qgroup_rescan_worker() set
> path->search_commit_root.
Then you are not basing your patches on the latest development branch
(misc-next) from David's github. The code in question is indeed very
recent:
e3884f5bd7cc ("btrfs: qgroup: Search commit root for rescan to avoid
missing extent")
>
> thanks,
> liubo
>
>> I think this change necessitates either:
>>
>> a) ASSERT(p->skip_locking) inside the if (p->search_commit_root) branch
>>
>> or
>>
>> b) Unconditionally setting ->skip_locking if we have set
>> search_commit_root and removing opencoded set of skip_locking alongside
>> search_commit_root.
>>
>> I think b) is the better option since it provides "fire and forget"
>> semantics when you want to search the commit root, since it's only read
>> only.
>>
>>>
>>> Signed-off-by: Liu Bo <bo.liu@linux.alibaba.com>
>>> ---
>>> fs/btrfs/ctree.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>>> index 399839df7a8f..cf34eca41d4e 100644
>>> --- a/fs/btrfs/ctree.c
>>> +++ b/fs/btrfs/ctree.c
>>> @@ -2623,8 +2623,6 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>>> level = btrfs_header_level(b);
>>> if (p->need_commit_sem)
>>> up_read(&fs_info->commit_root_sem);
>>> - if (!p->skip_locking)
>>> - btrfs_tree_read_lock(b);
>>>
>>> goto out;
>>> }
>>>
>> --
>> 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] 18+ messages in thread
end of thread, other threads:[~2018-05-18 5:36 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-15 17:52 [PATCH 0/6] btrfs_search_slot cleanups Liu Bo
2018-05-15 17:52 ` [PATCH 1/6] Btrfs: remove superfluous free_extent_buffer Liu Bo
2018-05-16 6:40 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 2/6] Btrfs: use more straightforward extent_buffer_uptodate Liu Bo
2018-05-16 6:43 ` Nikolay Borisov
2018-05-18 2:09 ` Liu Bo
2018-05-15 17:52 ` [PATCH 3/6] Btrfs: move get root of btrfs_search_slot to a helper Liu Bo
2018-05-16 6:54 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 4/6] Btrfs: remove unused check of skip_locking Liu Bo
2018-05-16 7:03 ` Nikolay Borisov
2018-05-16 7:06 ` Qu Wenruo
2018-05-18 2:55 ` Liu Bo
2018-05-18 5:36 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 5/6] Btrfs: grab write lock directly if write_lock_level is the max level Liu Bo
2018-05-16 7:09 ` Nikolay Borisov
2018-05-15 17:52 ` [PATCH 6/6] Btrfs: remove always true check in unlock_up Liu Bo
2018-05-16 6:47 ` Nikolay Borisov
2018-05-15 18:15 ` [PATCH 0/6] btrfs_search_slot cleanups David Sterba
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).