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....@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 <nbori...@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); > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html