* [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.