| Message ID | 20191205042921.25316-5-Damenly_Su@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unify origanization structure of block group cache | expand |
On 2019/12/5 下午12:29, damenly.su@gmail.com wrote: > From: Su Yue <Damenly_Su@gmx.com> > > Add the new value 2 of @contains in block_group_cache_tree_search(). > The new values means the function will return the block group that > contains bytenr, otherwise return the next one that starts after > @bytenr. Will be used in later commit. > > Signed-off-by: Su Yue <Damenly_Su@gmx.com> > --- > extent-tree.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/extent-tree.c b/extent-tree.c > index ab576f8732a2..1d8535049eaf 100644 > --- a/extent-tree.c > +++ b/extent-tree.c > @@ -196,13 +196,16 @@ static int btrfs_add_block_group_cache(struct btrfs_fs_info *info, > } > > /* > - * This will return the block group at or after bytenr if contains is 0, else > - * it will return the block group that contains the bytenr > + * @contains: > + * if 0, return the block group at or after bytenr if contains is 0. > + * if 1, return the block group that contains the bytenr. > + * if 2, return the block group that contains bytenr, otherwise return the > + * next one that starts after @bytenr. Thats a creative solution, good job on that. However since contains is no longer just simple 1 or 0, it's better to enum to define the behavior, other than using the immediate numbers. > */ > static struct btrfs_block_group_cache *block_group_cache_tree_search( > struct btrfs_fs_info *info, u64 bytenr, int contains) > { > - struct btrfs_block_group_cache *cache, *ret = NULL; > + struct btrfs_block_group_cache *cache, *ret = NULL, *tmp = NULL; > struct rb_node *n; > u64 end, start; > > @@ -215,8 +218,8 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( > start = cache->key.objectid; > > if (bytenr < start) { > - if (!contains && (!ret || start < ret->key.objectid)) > - ret = cache; > + if (!tmp || start < tmp->key.objectid) > + tmp = cache; This doesn't look correct. I was expecting something based on last found node, other than doing something strange in the rb-tree iteration code. At least this breaks readability. It would be much better to handle this after the rb tree while loop. Thanks, Qu > n = n->rb_left; > } else if (bytenr > start) { > if (contains && bytenr <= end) { > @@ -229,6 +232,13 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( > break; > } > } > + > + /* > + * If ret is NULL, means not found any block group cotanins @bytenr. > + * So just except the case that cotanins equals 1. > + */ > + if (!ret && contains != 1) > + ret = tmp; > return ret; > } > >
On 2019/12/5 3:38 PM, Qu Wenruo wrote: > > > On 2019/12/5 下午12:29, damenly.su@gmail.com wrote: >> From: Su Yue <Damenly_Su@gmx.com> >> >> Add the new value 2 of @contains in block_group_cache_tree_search(). >> The new values means the function will return the block group that >> contains bytenr, otherwise return the next one that starts after >> @bytenr. Will be used in later commit. >> >> Signed-off-by: Su Yue <Damenly_Su@gmx.com> >> --- >> extent-tree.c | 20 +++++++++++++++----- >> 1 file changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/extent-tree.c b/extent-tree.c >> index ab576f8732a2..1d8535049eaf 100644 >> --- a/extent-tree.c >> +++ b/extent-tree.c >> @@ -196,13 +196,16 @@ static int btrfs_add_block_group_cache(struct btrfs_fs_info *info, >> } >> >> /* >> - * This will return the block group at or after bytenr if contains is 0, else >> - * it will return the block group that contains the bytenr >> + * @contains: >> + * if 0, return the block group at or after bytenr if contains is 0. >> + * if 1, return the block group that contains the bytenr. >> + * if 2, return the block group that contains bytenr, otherwise return the >> + * next one that starts after @bytenr. > > Thats a creative solution, good job on that. > > However since contains is no longer just simple 1 or 0, it's better to > enum to define the behavior, other than using the immediate numbers. Nice suggestion, will do. > >> */ >> static struct btrfs_block_group_cache *block_group_cache_tree_search( >> struct btrfs_fs_info *info, u64 bytenr, int contains) >> { >> - struct btrfs_block_group_cache *cache, *ret = NULL; >> + struct btrfs_block_group_cache *cache, *ret = NULL, *tmp = NULL; >> struct rb_node *n; >> u64 end, start; >> >> @@ -215,8 +218,8 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( >> start = cache->key.objectid; >> >> if (bytenr < start) { >> - if (!contains && (!ret || start < ret->key.objectid)) >> - ret = cache; >> + if (!tmp || start < tmp->key.objectid) >> + tmp = cache; > > This doesn't look correct. > > I was expecting something based on last found node, other than doing > something strange in the rb-tree iteration code. > > At least this breaks readability. It would be much better to handle this > after the rb tree while loop. > Spent much brain power on this trick, this line means we always keep the next block block to @bytenr. The original code stores the block group cache in the @ret, which here I do is to store it into @tmp. This method doesn't change efficiency only little memory copies. If put the logic after the whole loop, I'm afraid it will require more code lines and lower the search efficiency. Thanks > Thanks, > Qu >> n = n->rb_left; >> } else if (bytenr > start) { >> if (contains && bytenr <= end) { >> @@ -229,6 +232,13 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( >> break; >> } >> } >> + >> + /* >> + * If ret is NULL, means not found any block group cotanins @bytenr. >> + * So just except the case that cotanins equals 1. >> + */ >> + if (!ret && contains != 1) >> + ret = tmp; >> return ret; >> } >> >> >
diff --git a/extent-tree.c b/extent-tree.c index ab576f8732a2..1d8535049eaf 100644 --- a/extent-tree.c +++ b/extent-tree.c @@ -196,13 +196,16 @@ static int btrfs_add_block_group_cache(struct btrfs_fs_info *info, } /* - * This will return the block group at or after bytenr if contains is 0, else - * it will return the block group that contains the bytenr + * @contains: + * if 0, return the block group at or after bytenr if contains is 0. + * if 1, return the block group that contains the bytenr. + * if 2, return the block group that contains bytenr, otherwise return the + * next one that starts after @bytenr. */ static struct btrfs_block_group_cache *block_group_cache_tree_search( struct btrfs_fs_info *info, u64 bytenr, int contains) { - struct btrfs_block_group_cache *cache, *ret = NULL; + struct btrfs_block_group_cache *cache, *ret = NULL, *tmp = NULL; struct rb_node *n; u64 end, start; @@ -215,8 +218,8 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( start = cache->key.objectid; if (bytenr < start) { - if (!contains && (!ret || start < ret->key.objectid)) - ret = cache; + if (!tmp || start < tmp->key.objectid) + tmp = cache; n = n->rb_left; } else if (bytenr > start) { if (contains && bytenr <= end) { @@ -229,6 +232,13 @@ static struct btrfs_block_group_cache *block_group_cache_tree_search( break; } } + + /* + * If ret is NULL, means not found any block group cotanins @bytenr. + * So just except the case that cotanins equals 1. + */ + if (!ret && contains != 1) + ret = tmp; return ret; }