All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] btrfs: check error value from btrfs_update_inode in tree log
@ 2021-05-19 15:26 Josef Bacik
  2021-05-19 16:17 ` Johannes Thumshirn
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Josef Bacik @ 2021-05-19 15:26 UTC (permalink / raw)
  To: linux-btrfs, kernel-team

Error injection testing uncovered a case where we ended up with invalid
link counts on an inode.  This happened because we failed to notice an
error when updating the inode while replaying the tree log, and
committed the transaction with an invalid file system.  Fix this by
checking the return value of btrfs_update_inode.  This resolved the link
count errors I was seeing, and we already properly handle passing up the
error values in these paths.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/tree-log.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 326be57f2828..4dc74949040d 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1574,7 +1574,9 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
 			if (ret)
 				goto out;
 
-			btrfs_update_inode(trans, root, BTRFS_I(inode));
+			ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
+			if (ret)
+				goto out;
 		}
 
 		ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
@@ -1749,7 +1751,9 @@ static noinline int fixup_inode_link_count(struct btrfs_trans_handle *trans,
 
 	if (nlink != inode->i_nlink) {
 		set_nlink(inode, nlink);
-		btrfs_update_inode(trans, root, BTRFS_I(inode));
+		ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
+		if (ret)
+			goto out;
 	}
 	BTRFS_I(inode)->index_cnt = (u64)-1;
 
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
  2021-05-19 15:26 [PATCH] btrfs: check error value from btrfs_update_inode in tree log Josef Bacik
@ 2021-05-19 16:17 ` Johannes Thumshirn
  2021-05-20  1:07 ` Qu Wenruo
  2021-05-21 12:55 ` David Sterba
  2 siblings, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2021-05-19 16:17 UTC (permalink / raw)
  To: Josef Bacik, linux-btrfs, kernel-team

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
  2021-05-19 15:26 [PATCH] btrfs: check error value from btrfs_update_inode in tree log Josef Bacik
  2021-05-19 16:17 ` Johannes Thumshirn
@ 2021-05-20  1:07 ` Qu Wenruo
  2021-05-20 13:24   ` David Sterba
  2021-05-21 12:55 ` David Sterba
  2 siblings, 1 reply; 6+ messages in thread
From: Qu Wenruo @ 2021-05-20  1:07 UTC (permalink / raw)
  To: Josef Bacik, linux-btrfs, kernel-team



On 2021/5/19 下午11:26, Josef Bacik wrote:
> Error injection testing uncovered a case where we ended up with invalid
> link counts on an inode.  This happened because we failed to notice an
> error when updating the inode while replaying the tree log, and
> committed the transaction with an invalid file system.  Fix this by
> checking the return value of btrfs_update_inode.  This resolved the link
> count errors I was seeing, and we already properly handle passing up the
> error values in these paths.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

But one thing unrelated to notice, inlined below.

> ---
>   fs/btrfs/tree-log.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 326be57f2828..4dc74949040d 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1574,7 +1574,9 @@ static noinline int add_inode_ref(struct btrfs_trans_handle *trans,
>   			if (ret)
>   				goto out;
>
> -			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?

Thanks,
Qu


> +			ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> +			if (ret)
> +				goto out;
>   		}
>
>   		ref_ptr = (unsigned long)(ref_ptr + ref_struct_size) + namelen;
> @@ -1749,7 +1751,9 @@ static noinline int fixup_inode_link_count(struct btrfs_trans_handle *trans,
>
>   	if (nlink != inode->i_nlink) {
>   		set_nlink(inode, nlink);
> -		btrfs_update_inode(trans, root, BTRFS_I(inode));
> +		ret = btrfs_update_inode(trans, root, BTRFS_I(inode));
> +		if (ret)
> +			goto out;
>   	}
>   	BTRFS_I(inode)->index_cnt = (u64)-1;
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
  2021-05-20  1:07 ` Qu Wenruo
@ 2021-05-20 13:24   ` David Sterba
  2021-05-21 12:47     ` David Sterba
  0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2021-05-20 13:24 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: Josef Bacik, linux-btrfs, kernel-team

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
  2021-05-20 13:24   ` David Sterba
@ 2021-05-21 12:47     ` David Sterba
  0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2021-05-21 12:47 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, Josef Bacik, linux-btrfs, kernel-team

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: check error value from btrfs_update_inode in tree log
  2021-05-19 15:26 [PATCH] btrfs: check error value from btrfs_update_inode in tree log Josef Bacik
  2021-05-19 16:17 ` Johannes Thumshirn
  2021-05-20  1:07 ` Qu Wenruo
@ 2021-05-21 12:55 ` David Sterba
  2 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2021-05-21 12:55 UTC (permalink / raw)
  To: Josef Bacik; +Cc: linux-btrfs, kernel-team

On Wed, May 19, 2021 at 11:26:25AM -0400, Josef Bacik wrote:
> Error injection testing uncovered a case where we ended up with invalid
> link counts on an inode.  This happened because we failed to notice an
> error when updating the inode while replaying the tree log, and
> committed the transaction with an invalid file system.  Fix this by
> checking the return value of btrfs_update_inode.  This resolved the link
> count errors I was seeing, and we already properly handle passing up the
> error values in these paths.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-21 12:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 15:26 [PATCH] btrfs: check error value from btrfs_update_inode in tree log Josef Bacik
2021-05-19 16:17 ` Johannes Thumshirn
2021-05-20  1:07 ` Qu Wenruo
2021-05-20 13:24   ` David Sterba
2021-05-21 12:47     ` David Sterba
2021-05-21 12:55 ` David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.