| Message ID | 565b63e6e34c122ca9bbe1e0272f43d6327a6316.1694126893.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanups and struct packing | expand |
On 2023/9/8 07:09, David Sterba wrote: > Drop all __must_check annotations because they're used in random > functions and not consistently. All errors should be handled. Is there any compiler warning option to warn about unchecked return value? In fact recently when working on qgroup GFP_ATOMIC, I found a call site that we didn't handle error at all (qgroup_update_counters()). I'm pretty sure that is not the last one. > > Signed-off-by: David Sterba <dsterba@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/extent-tree.h | 2 +- > fs/btrfs/relocation.c | 2 +- > fs/btrfs/root-tree.h | 6 ++---- > 3 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h > index ab2016db17e8..2109c72aea2a 100644 > --- a/fs/btrfs/extent-tree.h > +++ b/fs/btrfs/extent-tree.h > @@ -142,7 +142,7 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info, > int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len); > int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans); > int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref); > -int __must_check btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, > +int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, > int for_reloc); > int btrfs_drop_subtree(struct btrfs_trans_handle *trans, > struct btrfs_root *root, > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index 9951a0caf5bb..ad67a88f2bbf 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -631,7 +631,7 @@ static int clone_backref_node(struct btrfs_trans_handle *trans, > /* > * helper to add 'address of tree root -> reloc tree' mapping > */ > -static int __must_check __add_reloc_root(struct btrfs_root *root) > +static int __add_reloc_root(struct btrfs_root *root) > { > struct btrfs_fs_info *fs_info = root->fs_info; > struct rb_node *rb_node; > diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h > index eb15768b9170..8b2c3859e464 100644 > --- a/fs/btrfs/root-tree.h > +++ b/fs/btrfs/root-tree.h > @@ -20,10 +20,8 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, const struct btrfs_key *key > int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root, > const struct btrfs_key *key, > struct btrfs_root_item *item); > -int __must_check btrfs_update_root(struct btrfs_trans_handle *trans, > - struct btrfs_root *root, > - struct btrfs_key *key, > - struct btrfs_root_item *item); > +int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root *root, > + struct btrfs_key *key, struct btrfs_root_item *item); > int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key, > struct btrfs_path *path, struct btrfs_root_item *root_item, > struct btrfs_key *root_key);
On Fri, Sep 08, 2023 at 07:56:24AM +0800, Qu Wenruo wrote: > > > On 2023/9/8 07:09, David Sterba wrote: > > Drop all __must_check annotations because they're used in random > > functions and not consistently. All errors should be handled. > > Is there any compiler warning option to warn about unchecked return value? By default this is checked for functions annotated by warn_unused_result attribute, which is exactly what __must_check does. The check can be disabled but that's not what we want. > In fact recently when working on qgroup GFP_ATOMIC, I found a call site > that we didn't handle error at all (qgroup_update_counters()). A quick way how to check if a given function is properly handled is to add the __must_check attribute and run build. For qgroup_update_counters this is found right away: CC [M] fs/btrfs/qgroup.o fs/btrfs/qgroup.c: In function ‘btrfs_qgroup_account_extent’: fs/btrfs/qgroup.c:2736:9: warning: ignoring return value of ‘qgroup_update_counters’ declared with attribute ‘warn_unused_result’ [-Wunused-result] 2736 | qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2737 | num_bytes, seq); | ~~~~~~~~~~~~~~~ > I'm pretty sure that is not the last one. Yeah but we'd have to add the attribute to most if not all functions. It could be possibly automated using coccinnelle scripts, list functions, apply a semantic patch to add the attribute, compile it and get the results. A quick test shows it's doable. I think in general the error handling is good but if we want to make sure we haven't missed anything important we can't do it manually efficiently.
On Fri, Sep 08, 2023 at 02:34:15AM +0200, David Sterba wrote: > On Fri, Sep 08, 2023 at 07:56:24AM +0800, Qu Wenruo wrote: > > > > > > On 2023/9/8 07:09, David Sterba wrote: > > > Drop all __must_check annotations because they're used in random > > > functions and not consistently. All errors should be handled. > > > > Is there any compiler warning option to warn about unchecked return value? > > By default this is checked for functions annotated by warn_unused_result > attribute, which is exactly what __must_check does. The check can be > disabled but that's not what we want. > > > In fact recently when working on qgroup GFP_ATOMIC, I found a call site > > that we didn't handle error at all (qgroup_update_counters()). > > A quick way how to check if a given function is properly handled is to > add the __must_check attribute and run build. For qgroup_update_counters > this is found right away: > > CC [M] fs/btrfs/qgroup.o > fs/btrfs/qgroup.c: In function ‘btrfs_qgroup_account_extent’: > fs/btrfs/qgroup.c:2736:9: warning: ignoring return value of ‘qgroup_update_counters’ declared with attribute ‘warn_unused_result’ [-Wunused-result] > 2736 | qgroup_update_counters(fs_info, qgroups, nr_old_roots, nr_new_roots, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 2737 | num_bytes, seq); > | ~~~~~~~~~~~~~~~ > > > I'm pretty sure that is not the last one. > > Yeah but we'd have to add the attribute to most if not all functions. It > could be possibly automated using coccinnelle scripts, list functions, > apply a semantic patch to add the attribute, compile it and get the > results. A quick test shows it's doable. We have 3179 functions, lots of them return void (but they might ignore return values from called functions so it does not mean they're safe). There are 65 Functions that don't have all their return values checked, which 2%. There are several that do the extent bit handling and we knowingly don't handle the errors because all fatal erros lead to panic. The remaining are a mixed bag, I tried to check a few randomly and it seems that the error handling could lead to bad situations. Ideally we should audit the whole list, and also maybe identify some key functions that should have the __must_check attribute, e.g. btrfs_commit_transaction (which is not in the list). add_async_extent add_delayed_ref_head add_extent_mapping add_ra_bio_pages addrm_unknown_feature_attrs btree_release_folio __btrfs_add_free_space btrfs_block_rsv_release btrfs_cache_block_group btrfs_cleanup_transaction __btrfs_commit_inode_delayed_items btrfs_del_item btrfs_do_readpage btrfs_end_transaction btrfs_finish_ordered_io btrfs_forget_devices btrfs_free_reserved_extent btrfs_free_stale_devices btrfs_grab_root btrfs_inode_lock btrfs_log_inode_parent btrfs_orphan_del btrfs_pin_extent btrfs_ref_tree_mod __btrfs_release_folio btrfs_release_folio btrfs_repair_eb_io_failure btrfs_reset_sb_log_zones __btrfs_run_defrag_inode btrfs_wait_block_group_cache_done btrfs_wq_run_delayed_node btrfs_zone_activate cache_save_setup clear_extent_bit clear_extent_bits clear_extent_dirty clear_extent_uptodate clear_state_bit del_qgroup_rb del_qgroup_relation_item del_relation_rb dev_extent_hole_check do_zone_finish fill_writer_pointer_gap find_next_key get_raid56_logic_offset inc_block_group_ro insert_root_entry invalidate_extent_cache link_free_space load_block_group_size_class maybe_fail_all_tickets pin_down_extent process_page_range push_for_double_split qgroup_unreserve_range qgroup_update_counters release_extent_buffer remove_from_discard_list tree_insert_offset try_merge_free_space unlock_extent unpin_extent_range update_backref_cache __update_reloc_root
diff --git a/fs/btrfs/extent-tree.h b/fs/btrfs/extent-tree.h index ab2016db17e8..2109c72aea2a 100644 --- a/fs/btrfs/extent-tree.h +++ b/fs/btrfs/extent-tree.h @@ -142,7 +142,7 @@ int btrfs_free_reserved_extent(struct btrfs_fs_info *fs_info, int btrfs_pin_reserved_extent(struct btrfs_trans_handle *trans, u64 start, u64 len); int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans); int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans, struct btrfs_ref *generic_ref); -int __must_check btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, +int btrfs_drop_snapshot(struct btrfs_root *root, int update_ref, int for_reloc); int btrfs_drop_subtree(struct btrfs_trans_handle *trans, struct btrfs_root *root, diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 9951a0caf5bb..ad67a88f2bbf 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -631,7 +631,7 @@ static int clone_backref_node(struct btrfs_trans_handle *trans, /* * helper to add 'address of tree root -> reloc tree' mapping */ -static int __must_check __add_reloc_root(struct btrfs_root *root) +static int __add_reloc_root(struct btrfs_root *root) { struct btrfs_fs_info *fs_info = root->fs_info; struct rb_node *rb_node; diff --git a/fs/btrfs/root-tree.h b/fs/btrfs/root-tree.h index eb15768b9170..8b2c3859e464 100644 --- a/fs/btrfs/root-tree.h +++ b/fs/btrfs/root-tree.h @@ -20,10 +20,8 @@ int btrfs_del_root(struct btrfs_trans_handle *trans, const struct btrfs_key *key int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root, const struct btrfs_key *key, struct btrfs_root_item *item); -int __must_check btrfs_update_root(struct btrfs_trans_handle *trans, - struct btrfs_root *root, - struct btrfs_key *key, - struct btrfs_root_item *item); +int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root *root, + struct btrfs_key *key, struct btrfs_root_item *item); int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key, struct btrfs_path *path, struct btrfs_root_item *root_item, struct btrfs_key *root_key);
Drop all __must_check annotations because they're used in random functions and not consistently. All errors should be handled. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/extent-tree.h | 2 +- fs/btrfs/relocation.c | 2 +- fs/btrfs/root-tree.h | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-)