Re: [PATCH 3/9] btrfs: move the tree mod log code into its own file
On 11/3/21 10:31 pm, fdman...@kernel.org wrote: From: Filipe Manana The tree modification log, which records modifications done to btrees, is quite large and currently spread all over ctree.c, which is a huge file already. To make things better organized, move all that code into its own separate source and header files. Functions and definitions that are used outside of the module (mostly by ctree.c) are renamed so that they start with a "btrfs_" prefix. Everything else remains unchanged. This makes it easier to go over the tree modification log code everytime I need to go read it to fix a bug. Signed-off-by: Filipe Manana Reviewed-by: Anand Jain
Re: [PATCH 3/9] btrfs: move the tree mod log code into its own file
On Thu, Mar 11, 2021 at 5:27 PM kernel test robot wrote: > > Hi, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on v5.12-rc2] > [also build test WARNING on next-20210311] > [cannot apply to kdave/for-next] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-bug-fixes-for-the-tree-mod-log-and-small-refactorings/20210311-223429 > base:a38fd8748464831584a19438cbb3082b5a2dab15 > config: x86_64-randconfig-m001-20210311 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > smatch warnings: > fs/btrfs/tree-mod-log.c:286 btrfs_tree_mod_log_insert_move() warn: missing > error code 'ret' > fs/btrfs/tree-mod-log.c:519 btrfs_tree_mod_log_eb_copy() warn: missing error > code 'ret' > fs/btrfs/tree-mod-log.c:577 btrfs_tree_mod_log_free_eb() warn: missing error > code 'ret' No, those are all ok. For all cases, 'ret' was initialized to 0 when declared, and that's what we want to return when tree_mod_dont_log() returns true. > > vim +/ret +286 fs/btrfs/tree-mod-log.c > >246 >247 int btrfs_tree_mod_log_insert_move(struct extent_buffer *eb, >248 int dst_slot, int src_slot, >249 int nr_items) >250 { >251 struct tree_mod_elem *tm = NULL; >252 struct tree_mod_elem **tm_list = NULL; >253 int ret = 0; >254 int i; >255 int locked = 0; >256 >257 if (!tree_mod_need_log(eb->fs_info, eb)) >258 return 0; >259 >260 tm_list = kcalloc(nr_items, sizeof(struct tree_mod_elem *), > GFP_NOFS); >261 if (!tm_list) >262 return -ENOMEM; >263 >264 tm = kzalloc(sizeof(*tm), GFP_NOFS); >265 if (!tm) { >266 ret = -ENOMEM; >267 goto free_tms; >268 } >269 >270 tm->logical = eb->start; >271 tm->slot = src_slot; >272 tm->move.dst_slot = dst_slot; >273 tm->move.nr_items = nr_items; >274 tm->op = BTRFS_MOD_LOG_MOVE_KEYS; >275 >276 for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) { >277 tm_list[i] = alloc_tree_mod_elem(eb, i + dst_slot, >278 BTRFS_MOD_LOG_KEY_REMOVE_WHILE_MOVING, GFP_NOFS); >279 if (!tm_list[i]) { >280 ret = -ENOMEM; >281 goto free_tms; >282 } >283 } >284 >285 if (tree_mod_dont_log(eb->fs_info, eb)) > > 286 goto free_tms; >287 locked = 1; >288 >289 /* >290 * When we override something during the move, we log these > removals. >291 * This can only happen when we move towards the beginning of > the >292 * buffer, i.e. dst_slot < src_slot. >293 */ >294 for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) { >295 ret = tree_mod_log_insert(eb->fs_info, tm_list[i]); >296 if (ret) >297 goto free_tms; >298 } >299 >300 ret = tree_mod_log_insert(eb->fs_info, tm); >301 if (ret) >302 goto free_tms; >303 write_unlock(&eb->fs_info->tree_mod_log_lock); >304 kfree(tm_list); >305 >306 return 0; >307 free_tms: >308 for (i = 0; i < nr_items; i++) { >309 if (tm_list[i] && !RB_EMPTY_NODE(&tm_list[i]->node)) >310 rb_erase(&tm_list[i]->node, > &eb->fs_info->tree_mod_log); >311 kfree(tm_list[i]); >312 } >313 if (locked) >314 write_unlock(&eb->fs_info->tree_mod_log_lock); >315 kfree(tm_list); >316 kfree(tm); >317 >318 return ret; >319 } >320 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
Re: [PATCH 3/9] btrfs: move the tree mod log code into its own file
Hi, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on v5.12-rc2] [also build test WARNING on next-20210311] [cannot apply to kdave/for-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-bug-fixes-for-the-tree-mod-log-and-small-refactorings/20210311-223429 base:a38fd8748464831584a19438cbb3082b5a2dab15 config: x86_64-randconfig-m001-20210311 (attached as .config) compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot smatch warnings: fs/btrfs/tree-mod-log.c:286 btrfs_tree_mod_log_insert_move() warn: missing error code 'ret' fs/btrfs/tree-mod-log.c:519 btrfs_tree_mod_log_eb_copy() warn: missing error code 'ret' fs/btrfs/tree-mod-log.c:577 btrfs_tree_mod_log_free_eb() warn: missing error code 'ret' vim +/ret +286 fs/btrfs/tree-mod-log.c 246 247 int btrfs_tree_mod_log_insert_move(struct extent_buffer *eb, 248 int dst_slot, int src_slot, 249 int nr_items) 250 { 251 struct tree_mod_elem *tm = NULL; 252 struct tree_mod_elem **tm_list = NULL; 253 int ret = 0; 254 int i; 255 int locked = 0; 256 257 if (!tree_mod_need_log(eb->fs_info, eb)) 258 return 0; 259 260 tm_list = kcalloc(nr_items, sizeof(struct tree_mod_elem *), GFP_NOFS); 261 if (!tm_list) 262 return -ENOMEM; 263 264 tm = kzalloc(sizeof(*tm), GFP_NOFS); 265 if (!tm) { 266 ret = -ENOMEM; 267 goto free_tms; 268 } 269 270 tm->logical = eb->start; 271 tm->slot = src_slot; 272 tm->move.dst_slot = dst_slot; 273 tm->move.nr_items = nr_items; 274 tm->op = BTRFS_MOD_LOG_MOVE_KEYS; 275 276 for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) { 277 tm_list[i] = alloc_tree_mod_elem(eb, i + dst_slot, 278 BTRFS_MOD_LOG_KEY_REMOVE_WHILE_MOVING, GFP_NOFS); 279 if (!tm_list[i]) { 280 ret = -ENOMEM; 281 goto free_tms; 282 } 283 } 284 285 if (tree_mod_dont_log(eb->fs_info, eb)) > 286 goto free_tms; 287 locked = 1; 288 289 /* 290 * When we override something during the move, we log these removals. 291 * This can only happen when we move towards the beginning of the 292 * buffer, i.e. dst_slot < src_slot. 293 */ 294 for (i = 0; i + dst_slot < src_slot && i < nr_items; i++) { 295 ret = tree_mod_log_insert(eb->fs_info, tm_list[i]); 296 if (ret) 297 goto free_tms; 298 } 299 300 ret = tree_mod_log_insert(eb->fs_info, tm); 301 if (ret) 302 goto free_tms; 303 write_unlock(&eb->fs_info->tree_mod_log_lock); 304 kfree(tm_list); 305 306 return 0; 307 free_tms: 308 for (i = 0; i < nr_items; i++) { 309 if (tm_list[i] && !RB_EMPTY_NODE(&tm_list[i]->node)) 310 rb_erase(&tm_list[i]->node, &eb->fs_info->tree_mod_log); 311 kfree(tm_list[i]); 312 } 313 if (locked) 314 write_unlock(&eb->fs_info->tree_mod_log_lock); 315 kfree(tm_list); 316 kfree(tm); 317 318 return ret; 319 } 320 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
[PATCH 3/9] btrfs: move the tree mod log code into its own file
From: Filipe Manana The tree modification log, which records modifications done to btrees, is quite large and currently spread all over ctree.c, which is a huge file already. To make things better organized, move all that code into its own separate source and header files. Functions and definitions that are used outside of the module (mostly by ctree.c) are renamed so that they start with a "btrfs_" prefix. Everything else remains unchanged. This makes it easier to go over the tree modification log code everytime I need to go read it to fix a bug. Signed-off-by: Filipe Manana --- fs/btrfs/Makefile | 2 +- fs/btrfs/backref.c | 33 +- fs/btrfs/ctree.c| 956 ++-- fs/btrfs/ctree.h| 17 - fs/btrfs/delayed-ref.c | 9 +- fs/btrfs/qgroup.c | 9 +- fs/btrfs/tree-mod-log.c | 889 + fs/btrfs/tree-mod-log.h | 52 +++ 8 files changed, 1006 insertions(+), 961 deletions(-) create mode 100644 fs/btrfs/tree-mod-log.c create mode 100644 fs/btrfs/tree-mod-log.h diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile index b634c42115ea..fdba34b10a98 100644 --- a/fs/btrfs/Makefile +++ b/fs/btrfs/Makefile @@ -28,7 +28,7 @@ btrfs-y += super.o ctree.o extent-tree.o print-tree.o root-tree.o dir-item.o \ reada.o backref.o ulist.o qgroup.o send.o dev-replace.o raid56.o \ uuid-tree.o props.o free-space-tree.o tree-checker.o space-info.o \ block-rsv.o delalloc-space.o block-group.o discard.o reflink.o \ - subpage.o + subpage.o tree-mod-log.o btrfs-$(CONFIG_BTRFS_FS_POSIX_ACL) += acl.o btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c index f47c1528eb9a..117d423fdb93 100644 --- a/fs/btrfs/backref.c +++ b/fs/btrfs/backref.c @@ -14,6 +14,7 @@ #include "delayed-ref.h" #include "locking.h" #include "misc.h" +#include "tree-mod-log.h" /* Just an arbitrary number so we can be sure this happened */ #define BACKREF_FOUND_SHARED 6 @@ -452,7 +453,7 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path, if (path->slots[0] >= btrfs_header_nritems(eb) || is_shared_data_backref(preftrees, eb->start) || ref->root_id != btrfs_header_owner(eb)) { - if (time_seq == SEQ_LAST) + if (time_seq == BTRFS_SEQ_LAST) ret = btrfs_next_leaf(root, path); else ret = btrfs_next_old_leaf(root, path, time_seq); @@ -476,7 +477,7 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path, if (slot == 0 && (is_shared_data_backref(preftrees, eb->start) || ref->root_id != btrfs_header_owner(eb))) { - if (time_seq == SEQ_LAST) + if (time_seq == BTRFS_SEQ_LAST) ret = btrfs_next_leaf(root, path); else ret = btrfs_next_old_leaf(root, path, time_seq); @@ -514,7 +515,7 @@ static int add_all_parents(struct btrfs_root *root, struct btrfs_path *path, eie = NULL; } next: - if (time_seq == SEQ_LAST) + if (time_seq == BTRFS_SEQ_LAST) ret = btrfs_next_item(root, path); else ret = btrfs_next_old_item(root, path, time_seq); @@ -574,7 +575,7 @@ static int resolve_indirect_ref(struct btrfs_fs_info *fs_info, if (path->search_commit_root) root_level = btrfs_header_level(root->commit_root); - else if (time_seq == SEQ_LAST) + else if (time_seq == BTRFS_SEQ_LAST) root_level = btrfs_header_level(root->node); else root_level = btrfs_old_root_level(root, time_seq); @@ -605,7 +606,7 @@ static int resolve_indirect_ref(struct btrfs_fs_info *fs_info, search_key.offset >= LLONG_MAX) search_key.offset = 0; path->lowest_level = level; - if (time_seq == SEQ_LAST) + if (time_seq == BTRFS_SEQ_LAST) ret = btrfs_search_slot(NULL, root, &search_key, path, 0, 0); else ret = btrfs_search_old_slot(root, &search_key, path, time_seq); @@ -1147,8 +1148,8 @@ static int add_keyed_refs(struct btrfs_fs_info *fs_info, * indirect refs to their parent bytenr. * When roots are found, they're added to the roots list * - * If time_seq is set to SEQ_LAST, it will not search delayed_refs, and behave - * much like trans == NULL case, the difference only lies in it will not + * If time_seq is set to BTRFS_SEQ_LAST, it will not search delayed_refs, and + * behave much like trans == NULL case, the difference only lies in it will not * commit root. * The special case is for qgroup to search roots in commit_transaction(). * @@ -11