btrfs: relocation: Cleanup while() loop using for()

PATCH Superseded Qu Wenruo 3 participants 5 messages Sep 21, 2018 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2018-09-21 12:33 UTC #0 lore raw

PATCH 1/2 btrfs: relocation: Cleanup while() loop using for()

And add one line comment explaining what we're doing for each loop.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/relocation.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

fs/btrfs/relocation.c +6 -9

index 8783a1776540..d7f5a11dde20 100644
@@ -2997,27 +2997,25 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		goto out_free_blocks;
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Kick in readahead for tree blocks with missing keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready)
 			readahead_tree_block(fs_info, block->bytenr);
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Get first keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready) {
 			err = get_tree_block_key(fs_info, block);
 			if (err)
 				goto out_free_path;
 		}
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Do tree relocation */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 
 		node = build_backref_tree(rc, &block->key,

@@ -3034,7 +3032,6 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 				err = ret;
 			goto out;
 		}
-		rb_node = rb_next(rb_node);
 	}
 out:
 	err = finish_pending_nodes(trans, rc, path, err);

Signature · 1 line

2.19.0

Qu Wenruo

author

2018-09-21 12:33 UTC #1 lore raw

PATCH 2/2 btrfs: relocation: Remove redundant tree level check

Commit 581c1760415c ("btrfs: Validate child tree block's level and first key") has made tree block level check mandatory.

So if tree block level doesn't match, we won't get a valid extent buffer.
The extra WARN_ON() check can be removed completely.

Signed-off-by Qu Wenruo <[email protected]>

---
 fs/btrfs/relocation.c | 1 -
 1 file changed, 1 deletion(-)

fs/btrfs/relocation.c +0 -1

index d7f5a11dde20..56e2fab2926d 100644
@@ -2911,7 +2911,6 @@ static int get_tree_block_key(struct btrfs_fs_info *fs_info,
 		free_extent_buffer(eb);
 		return -EIO;
 	}
-	WARN_ON(btrfs_header_level(eb) != block->level);
 	if (block->level == 0)
 		btrfs_item_key_to_cpu(eb, &block->key, 0);
 	else

Signature · 1 line

2.19.0

Nikolay Borisov

Reviewed-by on v1 2/2

2018-09-21 12:43 UTC #2 lore raw

Re: [PATCH 2/2] btrfs: relocation: Remove redundant tree level check

Quoted 8 lines · Qu Wenruo

Commit 581c1760415c ("btrfs: Validate child tree block's level and first key") has made tree block level check mandatory.
So if tree block level doesn't match, we won't get a valid extent buffer.
The extra WARN_ON() check can be removed completely.
Signed-off-by Qu Wenruo <[email protected]>

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 17 lines

---
 fs/btrfs/relocation.c | 1 -
 1 file changed, 1 deletion(-)

fs/btrfs/relocation.c +0 -1

index d7f5a11dde20..56e2fab2926d 100644
@@ -2911,7 +2911,6 @@ static int get_tree_block_key(struct btrfs_fs_info *fs_info,
 		free_extent_buffer(eb);
 		return -EIO;
 	}
-	WARN_ON(btrfs_header_level(eb) != block->level);
 	if (block->level == 0)
 		btrfs_item_key_to_cpu(eb, &block->key, 0);
 	else

Nikolay Borisov

2018-09-21 12:40 UTC #3 lore raw

In reply to Qu Wenruo

Quoted 12 lines · Qu Wenruo

And add one line comment explaining what we're doing for each loop.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/relocation.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
fs/btrfs/relocation.c +0 -0
index 8783a1776540..d7f5a11dde20 100644

fs/btrfs/relocation.c int relocate_tree_blocks(struct btrfs_trans_handle *trans, · line 3001

Quoted 7 lines

fs/btrfs/relocation.c +2 -2

@@ -2997,27 +2997,25 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		goto out_free_blocks;
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Kick in readahead for tree blocks with missing keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {

FYI there is already a rbtree_postorder_for_each_entry_safe functino which allows to iterate the rb tree in post order. It provides stronger guarantees than you actually need (namely the _safe ) but in any case it will allows you to get rid of the "block = " line as well as the "rb_node =" line. This will remove all superfluous code that's needed to handle the iteration.

Quoted 35 lines

fs/btrfs/relocation.c +4 -7

 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready)
 			readahead_tree_block(fs_info, block->bytenr);
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Get first keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready) {
 			err = get_tree_block_key(fs_info, block);
 			if (err)
 				goto out_free_path;
 		}
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Do tree relocation */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 
 		node = build_backref_tree(rc, &block->key,

@@ -3034,7 +3032,6 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 				err = ret;
 			goto out;
 		}
-		rb_node = rb_next(rb_node);
 	}
 out:
 	err = finish_pending_nodes(trans, rc, path, err);

Qu Wenruo

2018-09-21 12:44 UTC #4 lore raw

Quoted 18 lines · Nikolay Borisov

Quoted 15 lines · Qu Wenruo
And add one line comment explaining what we're doing for each loop.
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/relocation.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)
fs/btrfs/relocation.c +0 -0
index 8783a1776540..d7f5a11dde20 100644
@@ -2997,27 +2997,25 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 		goto out_free_blocks;
 	}
 

Quoted 11 lines

fs/btrfs/relocation.c int relocate_tree_blocks(struct btrfs_trans_handle *trans, · line 3001
Quoted 4 lines
fs/btrfs/relocation.c +2 -2
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Kick in readahead for tree blocks with missing keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
FYI there is already a rbtree_postorder_for_each_entry_safe functino which allows to iterate the rb tree in post order. It provides stronger guarantees than you actually need (namely the _safe ) but in any case it will allows you to get rid of the "block = " line as well as the "rb_node =" line. This will remove all superfluous code that's needed to handle the iteration.

Great thanks!

This is much better solution, I'll use them in next version.

Thanks,
Qu

Quoted 36 lines

fs/btrfs/relocation.c +4 -7

 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready)
 			readahead_tree_block(fs_info, block->bytenr);
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Get first keys */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 		if (!block->key_ready) {
 			err = get_tree_block_key(fs_info, block);
 			if (err)
 				goto out_free_path;
 		}
-		rb_node = rb_next(rb_node);
 	}
 
-	rb_node = rb_first(blocks);
-	while (rb_node) {
+	/* Do tree relocation */
+	for (rb_node = rb_first(blocks); rb_node; rb_node = rb_next(rb_node)) {
 		block = rb_entry(rb_node, struct tree_block, rb_node);
 
 		node = build_backref_tree(rc, &block->key,

@@ -3034,7 +3032,6 @@ int relocate_tree_blocks(struct btrfs_trans_handle *trans,
 				err = ret;
 			goto out;
 		}
-		rb_node = rb_next(rb_node);
 	}
 out:
 	err = finish_pending_nodes(trans, rc, path, err);