Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log

[David Sterba <dsterba@suse.cz>]

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:
> > > -			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;
 
---------------------