From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net ([212.227.17.22]:39303 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389229AbeIUMo2 (ORCPT ); Fri, 21 Sep 2018 08:44:28 -0400 Subject: Re: [PATCH 1/2] btrfs: relocation: Cleanup while() loop using for() To: Nikolay Borisov , Qu Wenruo , linux-btrfs@vger.kernel.org References: <20180921064521.13102-1-wqu@suse.com> From: Qu Wenruo Message-ID: Date: Fri, 21 Sep 2018 14:56:47 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 2018/9/21 下午2:53, Nikolay Borisov wrote: > > > On 21.09.2018 09:45, Qu Wenruo wrote: >> And add one line comment explaining what we're doing for each loop. >> >> Signed-off-by: Qu Wenruo >> --- >> fs/btrfs/relocation.c | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >> index 8783a1776540..d7f5a11dde20 100644 >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -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. Great thanks! This is much better solution, I'll use them in next version. Thanks, Qu > >> 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); >>