| Message ID | 20200527101053.7340-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Open code key_search | expand |
On 27/05/2020 12:11, Nikolay Borisov wrote: Looks good, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> One nit though: > + ret = btrfs_bin_search(b, key, &slot); > + prev_cmp = ret; I actually find: ret = prev_cmp = brtfs_bin_search(b, key, &slot); a bit more readable in this case.
On 27.05.20 г. 15:25 ч., Johannes Thumshirn wrote: > On 27/05/2020 12:11, Nikolay Borisov wrote: > > Looks good, > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> > > One nit though: > >> + ret = btrfs_bin_search(b, key, &slot); >> + prev_cmp = ret; > > I actually find: > ret = prev_cmp = brtfs_bin_search(b, key, &slot); > > a bit more readable in this case. > True, AFAIK David is not a fan of multiple assignments on the same line ;)
On 27/05/2020 14:36, Nikolay Borisov wrote:
> True, AFAIK David is not a fan of multiple assignments on the same line ;)
Yes I remember him saying something like this.
On Wed, May 27, 2020 at 12:39:01PM +0000, Johannes Thumshirn wrote: > On 27/05/2020 14:36, Nikolay Borisov wrote: > > True, AFAIK David is not a fan of multiple assignments on the same line ;) > > Yes I remember him saying something like this. I can point you to the kernel coding style any number of times: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation "Don’t put multiple assignments on a single line either. Kernel coding style is super simple. Avoid tricky expressions."
On Wed, May 27, 2020 at 01:10:53PM +0300, Nikolay Borisov wrote: > This function wraps the optimisation implemented by d7396f07358a, > however this optimisation is really used in only one place - > btrfs_search_slot. Just open code the optimisation and also add a > comment explaining how it works since it's not clear just by looking > at the code - the key point here is it depends on an internal invariant > that BTRFS' btree provides, namely intermediate pointers always contain > the key at slot0 at the child node. So in the case of exact match we > can safely assume that the given key will always be in slot 0 on lower > levels. > > Furthermore this results in a reduction of btrfs_search_slot's size: > > ./scripts/bloat-o-meter ctree.orig fs/btrfs/ctree.o > add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-75 (-75) > Function old new delta > btrfs_search_slot 2783 2708 -75 > Total: Before=50423, After=50348, chg -0.15% > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Added to misc-next, thanks.
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 92775554d1cc..3ab307b4b294 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -2488,19 +2488,6 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans, return ret; } -static int key_search(struct extent_buffer *b, const struct btrfs_key *key, - int *prev_cmp, int *slot) -{ - if (*prev_cmp != 0) { - *prev_cmp = btrfs_bin_search(b, key, slot); - return *prev_cmp; - } - - *slot = 0; - - return 0; -} - int btrfs_find_item(struct btrfs_root *fs_root, struct btrfs_path *path, u64 iobjectid, u64 ioff, u8 key_type, struct btrfs_key *found_key) @@ -2770,9 +2757,23 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root, } } - ret = key_search(b, key, &prev_cmp, &slot); - if (ret < 0) - goto done; + /* + * If btrfs_bin_search returns an exact match (prev_cmp == 0) + * we can safely assume the target key will always be in + * slot 0 on lower levels due to the invariants BTRFS' btree + * provides, namely that a btrfs_key_ptr entry always points + * to the lowest key in the child node, thus we can skip + * searching lower levels + */ + if (prev_cmp == 0) { + slot = 0; + ret = 0; + } else { + ret = btrfs_bin_search(b, key, &slot); + prev_cmp = ret; + if (ret < 0) + goto done; + } if (level == 0) { p->slots[level] = slot; @@ -2896,7 +2897,6 @@ int btrfs_search_old_slot(struct btrfs_root *root, const struct btrfs_key *key, int level; int lowest_unlock = 1; u8 lowest_level = 0; - int prev_cmp = -1; lowest_level = p->lowest_level; WARN_ON(p->nodes[0] != NULL); @@ -2929,12 +2929,7 @@ int btrfs_search_old_slot(struct btrfs_root *root, const struct btrfs_key *key, */ btrfs_unlock_up_safe(p, level + 1); - /* - * Since we can unwind ebs we want to do a real search every - * time. - */ - prev_cmp = -1; - ret = key_search(b, key, &prev_cmp, &slot); + ret = btrfs_bin_search(b, key, &slot); if (ret < 0) goto done;
This function wraps the optimisation implemented by d7396f07358a, however this optimisation is really used in only one place - btrfs_search_slot. Just open code the optimisation and also add a comment explaining how it works since it's not clear just by looking at the code - the key point here is it depends on an internal invariant that BTRFS' btree provides, namely intermediate pointers always contain the key at slot0 at the child node. So in the case of exact match we can safely assume that the given key will always be in slot 0 on lower levels. Furthermore this results in a reduction of btrfs_search_slot's size: ./scripts/bloat-o-meter ctree.orig fs/btrfs/ctree.o add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-75 (-75) Function old new delta btrfs_search_slot 2783 2708 -75 Total: Before=50423, After=50348, chg -0.15% Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/ctree.c | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) -- 2.17.1