Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
From: David Sterba <hidden>
Date: 2021-05-21 12:51:00
On Thu, May 20, 2021 at 03:24:10PM +0200, David Sterba wrote:
On Thu, May 20, 2021 at 09:07:26AM +0800, Qu Wenruo wrote:quoted
quoted
- btrfs_update_inode(trans, root, BTRFS_I(inode));I did a quick grep and found that we have other locations where we call btrfs_uppdate_inode() without catching the return value: $ grep -IRe "^\s\+btrfs_update_inode(" fs/btrfs/ fs/btrfs/free-space-cache.c: btrfs_update_inode(trans, root, BTRFS_I(inode)); fs/btrfs/free-space-cache.c: btrfs_update_inode(trans, root, BTRFS_I(inode)); fs/btrfs/inode.c: btrfs_update_inode(trans, root, inode); fs/btrfs/inode.c: btrfs_update_inode(trans, root, BTRFS_I(inode)); Maybe it's better to make btrfs_update_inode() to have __must_check prefix?We should handle errors everywhere by default, with rare exceptions that might get a comment why it's ok to ignore the errors. So that would mean that basically all functions get __must_check attribute if we really want to catch that.
As an alternative I'm thinking about a set of coccinelle rules to find such cases, and not only that. Eg. lack of error handling of btrfs_update_inode is as simple as --- @@ @@ * btrfs_update_inode(...); --- With following output. The advantage of separate rules is that it can be run outside of compilation and the semantic language offers much wider options than the few compiler attributes. diff -u -p ./free-space-cache.c /tmp/nothing/free-space-cache.c
--- ./free-space-cache.c
+++ /tmp/nothing/free-space-cache.c@@ -1270,7 +1270,6 @@ out: "failed to write free space cache for block group %llu error %d", block_group->start, ret); } - btrfs_update_inode(trans, root, BTRFS_I(inode)); if (block_group) { /* the dirty list is protected by the dirty_bgs_lock */
@@ -1455,7 +1454,6 @@ out: invalidate_inode_pages2(inode->i_mapping); BTRFS_I(inode)->generation = 0; } - btrfs_update_inode(trans, root, BTRFS_I(inode)); if (must_iput) iput(inode); return ret;
diff -u -p ./inode.c /tmp/nothing/inode.c
--- ./inode.c
+++ /tmp/nothing/inode.c@@ -4997,7 +4997,6 @@ static int maybe_insert_hole(struct btrf btrfs_abort_transaction(trans, ret); } else { btrfs_update_inode_bytes(inode, 0, drop_args.bytes_found); - btrfs_update_inode(trans, root, inode); } btrfs_end_transaction(trans); return ret;
@@ -6564,7 +6563,6 @@ static int btrfs_mknod(struct user_names if (err) goto out_unlock; - btrfs_update_inode(trans, root, BTRFS_I(inode)); d_instantiate_new(dentry, inode); out_unlock:
diff -u -p ./tree-log.c /tmp/nothing/tree-log.c
--- ./tree-log.c
+++ /tmp/nothing/tree-log.c@@ -1574,7 +1574,6 @@ static noinline int add_inode_ref(struct if (ret) goto out; - btrfs_update_inode(trans, root, BTRFS_I(inode)); } ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
@@ -1749,7 +1748,6 @@ static noinline int fixup_inode_link_cou if (nlink != inode->i_nlink) { set_nlink(inode, nlink); - btrfs_update_inode(trans, root, BTRFS_I(inode)); } BTRFS_I(inode)->index_cnt = (u64)-1; ---------------------