| Message ID | 20230131144701.34418-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,1/3] f2fs: clean up __update_extent_tree_range() | expand |
On 01/31, Chao Yu wrote: > No logic change, just avoid goto statement. I wanted to avoid a deep if/else statement. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 35 deletions(-) > > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c > index d70ad6a44cbf..cf65a188d112 100644 > --- a/fs/f2fs/extent_cache.c > +++ b/fs/f2fs/extent_cache.c > @@ -782,47 +782,43 @@ static void __update_extent_tree_range(struct inode *inode, > en = next_en; > } > > - if (type == EX_BLOCK_AGE) > - goto update_age_extent_cache; > - > - /* 3. update extent in read extent cache */ > - BUG_ON(type != EX_READ); > - > - if (tei->blk) { > - __set_extent_info(&ei, fofs, len, tei->blk, false, > - 0, 0, EX_READ); > - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) > - __insert_extent_tree(sbi, et, &ei, > + /* 3. update extent in extent cache */ > + if (type == EX_READ) { > + if (tei->blk) { > + __set_extent_info(&ei, fofs, len, tei->blk, false, > + 0, 0, EX_READ); > + if (!__try_merge_extent_node(sbi, et, &ei, > + prev_en, next_en)) > + __insert_extent_tree(sbi, et, &ei, > insert_p, insert_parent, leftmost); > > - /* give up extent_cache, if split and small updates happen */ > - if (dei.len >= 1 && > - prev.len < F2FS_MIN_EXTENT_LEN && > - et->largest.len < F2FS_MIN_EXTENT_LEN) { > - et->largest.len = 0; > - et->largest_updated = true; > - set_inode_flag(inode, FI_NO_EXTENT); > + /* give up read extent cache, if split and small updates happen */ > + if (dei.len >= 1 && > + prev.len < F2FS_MIN_EXTENT_LEN && > + et->largest.len < F2FS_MIN_EXTENT_LEN) { > + et->largest.len = 0; > + et->largest_updated = true; > + set_inode_flag(inode, FI_NO_EXTENT); > + } > } > - } > - > - if (is_inode_flag_set(inode, FI_NO_EXTENT)) > - __free_extent_tree(sbi, et); > > - if (et->largest_updated) { > - et->largest_updated = false; > - updated = true; > - } > - goto out_read_extent_cache; > -update_age_extent_cache: > - if (!tei->last_blocks) > - goto out_read_extent_cache; > + if (is_inode_flag_set(inode, FI_NO_EXTENT)) > + __free_extent_tree(sbi, et); > > - __set_extent_info(&ei, fofs, len, 0, false, > - tei->age, tei->last_blocks, EX_BLOCK_AGE); > - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) > - __insert_extent_tree(sbi, et, &ei, > + if (et->largest_updated) { > + et->largest_updated = false; > + updated = true; > + } > + } else if (type == EX_BLOCK_AGE) { > + if (tei->last_blocks) { > + __set_extent_info(&ei, fofs, len, 0, false, > + tei->age, tei->last_blocks, EX_BLOCK_AGE); > + if (!__try_merge_extent_node(sbi, et, &ei, > + prev_en, next_en)) > + __insert_extent_tree(sbi, et, &ei, > insert_p, insert_parent, leftmost); > -out_read_extent_cache: > + } > + } > write_unlock(&et->lock); > > if (updated) > -- > 2.36.1
Hello: This series was applied to jaegeuk/f2fs.git (dev) by Jaegeuk Kim <jaegeuk@kernel.org>: On Tue, 31 Jan 2023 22:46:59 +0800 you wrote: > No logic change, just avoid goto statement. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++---------------------- > 1 file changed, 31 insertions(+), 35 deletions(-) Here is the summary with links: - [f2fs-dev,1/3] f2fs: clean up __update_extent_tree_range() (no matching commit) - [f2fs-dev,2/3] f2fs: fix to update age extent correctly during truncation https://git.kernel.org/jaegeuk/f2fs/c/334ce4a79c9e - [f2fs-dev,3/3] f2fs: fix to update age extent in f2fs_do_zero_range() https://git.kernel.org/jaegeuk/f2fs/c/de6b3a5e09b2 You are awesome, thank you!
On 2023/2/1 2:56, Jaegeuk Kim wrote: > I wanted to avoid a deep if/else statement. So how about this: From 418b408420367ac5491c97a7c4d26e3d0e68ea57 Mon Sep 17 00:00:00 2001 From: Chao Yu <chao@kernel.org> Date: Tue, 31 Jan 2023 22:46:59 +0800 Subject: [PATCH v2] f2fs: clean up __update_extent_tree_range() Introduce __update_read_extent_cache() and __update_age_extent_cache() to clean up __update_extent_tree_range(), no logic change. Signed-off-by: Chao Yu <chao@kernel.org> --- v2 - introduce __update_read_extent_cache() and __update_age_extent_cache() to avoid a deep if/else statement in __update_extent_tree_range(). fs/f2fs/extent_cache.c | 116 +++++++++++++++++++++++++++-------------- 1 file changed, 77 insertions(+), 39 deletions(-) diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c index d70ad6a44cbf..887b0b2898b9 100644 --- a/fs/f2fs/extent_cache.c +++ b/fs/f2fs/extent_cache.c @@ -666,6 +666,75 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi, return en; } +static inline bool __update_read_extent_cache(struct inode *inode, + struct extent_info *tei, + struct extent_info *ei, + struct extent_info *dei, + struct extent_info *prev, + unsigned int fofs, unsigned int len, + struct extent_node *prev_en, + struct extent_node *next_en, + struct rb_node **insert_p, + struct rb_node *insert_parent, + bool leftmost) +{ + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); + struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_READ]; + + if (!tei->blk) + return false; + + __set_extent_info(ei, fofs, len, tei->blk, false, + 0, 0, EX_READ); + if (!__try_merge_extent_node(sbi, et, ei, + prev_en, next_en)) + __insert_extent_tree(sbi, et, ei, + insert_p, insert_parent, leftmost); + + /* give up read extent cache, if split and small updates happen */ + if (dei->len >= 1 && + prev->len < F2FS_MIN_EXTENT_LEN && + et->largest.len < F2FS_MIN_EXTENT_LEN) { + et->largest.len = 0; + et->largest_updated = true; + set_inode_flag(inode, FI_NO_EXTENT); + } + + if (is_inode_flag_set(inode, FI_NO_EXTENT)) + __free_extent_tree(sbi, et); + + if (et->largest_updated) { + et->largest_updated = false; + return true; + } + + return false; +} + +static inline void __update_age_extent_cache(struct inode *inode, + struct extent_info *tei, + struct extent_info *ei, + unsigned int fofs, unsigned int len, + struct extent_node *prev_en, + struct extent_node *next_en, + struct rb_node **insert_p, + struct rb_node *insert_parent, + bool leftmost) +{ + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); + struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_BLOCK_AGE]; + + if (!tei->last_blocks) + return; + + __set_extent_info(ei, fofs, len, 0, false, + tei->age, tei->last_blocks, EX_BLOCK_AGE); + if (!__try_merge_extent_node(sbi, et, ei, + prev_en, next_en)) + __insert_extent_tree(sbi, et, ei, + insert_p, insert_parent, leftmost); +} + static void __update_extent_tree_range(struct inode *inode, struct extent_info *tei, enum extent_type type) { @@ -782,47 +851,16 @@ static void __update_extent_tree_range(struct inode *inode, en = next_en; } - if (type == EX_BLOCK_AGE) - goto update_age_extent_cache; - - /* 3. update extent in read extent cache */ - BUG_ON(type != EX_READ); - - if (tei->blk) { - __set_extent_info(&ei, fofs, len, tei->blk, false, - 0, 0, EX_READ); - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) - __insert_extent_tree(sbi, et, &ei, + /* 3. update extent in extent cache */ + if (type == EX_READ) + updated = __update_read_extent_cache(inode, tei, &ei, &dei, + &prev, fofs, len, prev_en, next_en, insert_p, insert_parent, leftmost); - - /* give up extent_cache, if split and small updates happen */ - if (dei.len >= 1 && - prev.len < F2FS_MIN_EXTENT_LEN && - et->largest.len < F2FS_MIN_EXTENT_LEN) { - et->largest.len = 0; - et->largest_updated = true; - set_inode_flag(inode, FI_NO_EXTENT); - } - } - - if (is_inode_flag_set(inode, FI_NO_EXTENT)) - __free_extent_tree(sbi, et); - - if (et->largest_updated) { - et->largest_updated = false; - updated = true; - } - goto out_read_extent_cache; -update_age_extent_cache: - if (!tei->last_blocks) - goto out_read_extent_cache; - - __set_extent_info(&ei, fofs, len, 0, false, - tei->age, tei->last_blocks, EX_BLOCK_AGE); - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) - __insert_extent_tree(sbi, et, &ei, + else if (type == EX_BLOCK_AGE) + __update_age_extent_cache(inode, tei, &ei, + fofs, len, prev_en, next_en, insert_p, insert_parent, leftmost); -out_read_extent_cache: + write_unlock(&et->lock); if (updated)
On 02/01, Chao Yu wrote: > On 2023/2/1 2:56, Jaegeuk Kim wrote: > > I wanted to avoid a deep if/else statement. > > So how about this: Nothing impressive. > > From 418b408420367ac5491c97a7c4d26e3d0e68ea57 Mon Sep 17 00:00:00 2001 > From: Chao Yu <chao@kernel.org> > Date: Tue, 31 Jan 2023 22:46:59 +0800 > Subject: [PATCH v2] f2fs: clean up __update_extent_tree_range() > > Introduce __update_read_extent_cache() and __update_age_extent_cache() > to clean up __update_extent_tree_range(), no logic change. > > Signed-off-by: Chao Yu <chao@kernel.org> > --- > v2 > - introduce __update_read_extent_cache() and __update_age_extent_cache() > to avoid a deep if/else statement in __update_extent_tree_range(). > fs/f2fs/extent_cache.c | 116 +++++++++++++++++++++++++++-------------- > 1 file changed, 77 insertions(+), 39 deletions(-) > > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c > index d70ad6a44cbf..887b0b2898b9 100644 > --- a/fs/f2fs/extent_cache.c > +++ b/fs/f2fs/extent_cache.c > @@ -666,6 +666,75 @@ static struct extent_node *__insert_extent_tree(struct f2fs_sb_info *sbi, > return en; > } > > +static inline bool __update_read_extent_cache(struct inode *inode, > + struct extent_info *tei, > + struct extent_info *ei, > + struct extent_info *dei, > + struct extent_info *prev, > + unsigned int fofs, unsigned int len, > + struct extent_node *prev_en, > + struct extent_node *next_en, > + struct rb_node **insert_p, > + struct rb_node *insert_parent, > + bool leftmost) > +{ > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > + struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_READ]; > + > + if (!tei->blk) > + return false; > + > + __set_extent_info(ei, fofs, len, tei->blk, false, > + 0, 0, EX_READ); > + if (!__try_merge_extent_node(sbi, et, ei, > + prev_en, next_en)) > + __insert_extent_tree(sbi, et, ei, > + insert_p, insert_parent, leftmost); > + > + /* give up read extent cache, if split and small updates happen */ > + if (dei->len >= 1 && > + prev->len < F2FS_MIN_EXTENT_LEN && > + et->largest.len < F2FS_MIN_EXTENT_LEN) { > + et->largest.len = 0; > + et->largest_updated = true; > + set_inode_flag(inode, FI_NO_EXTENT); > + } > + > + if (is_inode_flag_set(inode, FI_NO_EXTENT)) > + __free_extent_tree(sbi, et); > + > + if (et->largest_updated) { > + et->largest_updated = false; > + return true; > + } > + > + return false; > +} > + > +static inline void __update_age_extent_cache(struct inode *inode, > + struct extent_info *tei, > + struct extent_info *ei, > + unsigned int fofs, unsigned int len, > + struct extent_node *prev_en, > + struct extent_node *next_en, > + struct rb_node **insert_p, > + struct rb_node *insert_parent, > + bool leftmost) > +{ > + struct f2fs_sb_info *sbi = F2FS_I_SB(inode); > + struct extent_tree *et = F2FS_I(inode)->extent_tree[EX_BLOCK_AGE]; > + > + if (!tei->last_blocks) > + return; > + > + __set_extent_info(ei, fofs, len, 0, false, > + tei->age, tei->last_blocks, EX_BLOCK_AGE); > + if (!__try_merge_extent_node(sbi, et, ei, > + prev_en, next_en)) > + __insert_extent_tree(sbi, et, ei, > + insert_p, insert_parent, leftmost); > +} > + > static void __update_extent_tree_range(struct inode *inode, > struct extent_info *tei, enum extent_type type) > { > @@ -782,47 +851,16 @@ static void __update_extent_tree_range(struct inode *inode, > en = next_en; > } > > - if (type == EX_BLOCK_AGE) > - goto update_age_extent_cache; > - > - /* 3. update extent in read extent cache */ > - BUG_ON(type != EX_READ); > - > - if (tei->blk) { > - __set_extent_info(&ei, fofs, len, tei->blk, false, > - 0, 0, EX_READ); > - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) > - __insert_extent_tree(sbi, et, &ei, > + /* 3. update extent in extent cache */ > + if (type == EX_READ) > + updated = __update_read_extent_cache(inode, tei, &ei, &dei, > + &prev, fofs, len, prev_en, next_en, > insert_p, insert_parent, leftmost); > - > - /* give up extent_cache, if split and small updates happen */ > - if (dei.len >= 1 && > - prev.len < F2FS_MIN_EXTENT_LEN && > - et->largest.len < F2FS_MIN_EXTENT_LEN) { > - et->largest.len = 0; > - et->largest_updated = true; > - set_inode_flag(inode, FI_NO_EXTENT); > - } > - } > - > - if (is_inode_flag_set(inode, FI_NO_EXTENT)) > - __free_extent_tree(sbi, et); > - > - if (et->largest_updated) { > - et->largest_updated = false; > - updated = true; > - } > - goto out_read_extent_cache; > -update_age_extent_cache: > - if (!tei->last_blocks) > - goto out_read_extent_cache; > - > - __set_extent_info(&ei, fofs, len, 0, false, > - tei->age, tei->last_blocks, EX_BLOCK_AGE); > - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) > - __insert_extent_tree(sbi, et, &ei, > + else if (type == EX_BLOCK_AGE) > + __update_age_extent_cache(inode, tei, &ei, > + fofs, len, prev_en, next_en, > insert_p, insert_parent, leftmost); > -out_read_extent_cache: > + > write_unlock(&et->lock); > > if (updated) > -- > 2.25.1 > > Thanks,
On 2023/2/6 11:41, Jaegeuk Kim wrote: > On 02/01, Chao Yu wrote: >> On 2023/2/1 2:56, Jaegeuk Kim wrote: >>> I wanted to avoid a deep if/else statement. >> >> So how about this: > > Nothing impressive. Oops... Thanks,
diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c index d70ad6a44cbf..cf65a188d112 100644 --- a/fs/f2fs/extent_cache.c +++ b/fs/f2fs/extent_cache.c @@ -782,47 +782,43 @@ static void __update_extent_tree_range(struct inode *inode, en = next_en; } - if (type == EX_BLOCK_AGE) - goto update_age_extent_cache; - - /* 3. update extent in read extent cache */ - BUG_ON(type != EX_READ); - - if (tei->blk) { - __set_extent_info(&ei, fofs, len, tei->blk, false, - 0, 0, EX_READ); - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) - __insert_extent_tree(sbi, et, &ei, + /* 3. update extent in extent cache */ + if (type == EX_READ) { + if (tei->blk) { + __set_extent_info(&ei, fofs, len, tei->blk, false, + 0, 0, EX_READ); + if (!__try_merge_extent_node(sbi, et, &ei, + prev_en, next_en)) + __insert_extent_tree(sbi, et, &ei, insert_p, insert_parent, leftmost); - /* give up extent_cache, if split and small updates happen */ - if (dei.len >= 1 && - prev.len < F2FS_MIN_EXTENT_LEN && - et->largest.len < F2FS_MIN_EXTENT_LEN) { - et->largest.len = 0; - et->largest_updated = true; - set_inode_flag(inode, FI_NO_EXTENT); + /* give up read extent cache, if split and small updates happen */ + if (dei.len >= 1 && + prev.len < F2FS_MIN_EXTENT_LEN && + et->largest.len < F2FS_MIN_EXTENT_LEN) { + et->largest.len = 0; + et->largest_updated = true; + set_inode_flag(inode, FI_NO_EXTENT); + } } - } - - if (is_inode_flag_set(inode, FI_NO_EXTENT)) - __free_extent_tree(sbi, et); - if (et->largest_updated) { - et->largest_updated = false; - updated = true; - } - goto out_read_extent_cache; -update_age_extent_cache: - if (!tei->last_blocks) - goto out_read_extent_cache; + if (is_inode_flag_set(inode, FI_NO_EXTENT)) + __free_extent_tree(sbi, et); - __set_extent_info(&ei, fofs, len, 0, false, - tei->age, tei->last_blocks, EX_BLOCK_AGE); - if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en)) - __insert_extent_tree(sbi, et, &ei, + if (et->largest_updated) { + et->largest_updated = false; + updated = true; + } + } else if (type == EX_BLOCK_AGE) { + if (tei->last_blocks) { + __set_extent_info(&ei, fofs, len, 0, false, + tei->age, tei->last_blocks, EX_BLOCK_AGE); + if (!__try_merge_extent_node(sbi, et, &ei, + prev_en, next_en)) + __insert_extent_tree(sbi, et, &ei, insert_p, insert_parent, leftmost); -out_read_extent_cache: + } + } write_unlock(&et->lock); if (updated)
No logic change, just avoid goto statement. Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/extent_cache.c | 66 ++++++++++++++++++++---------------------- 1 file changed, 31 insertions(+), 35 deletions(-)