| Message ID | 1526406728-109055-4-git-send-email-bo.liu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > -- 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
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);
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(-)