* [PATCH] btrfs: fixup error handling in fixup_inode_link_counts
@ 2021-05-19 17:13 Josef Bacik
2021-05-21 7:46 ` Johannes Thumshirn
2021-05-21 12:38 ` David Sterba
0 siblings, 2 replies; 3+ messages in thread
From: Josef Bacik @ 2021-05-19 17:13 UTC (permalink / raw)
To: linux-btrfs, kernel-team
This function has the following pattern
while (1) {
ret = whatever();
if (ret)
goto out;
}
ret = 0
out:
return ret;
However several places in this while loop we simply break; when there's
a problem, thus clearing the return value, and in one case we do a
return -EIO, and leak the memory for the path.
Fix this by re-arranging the loop to deal with ret == 1 coming from
btrfs_search_slot, and then simply delete the
ret = 0;
out:
bit so everybody can break if there is an error, which will allow for
proper error handling to occur.
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
fs/btrfs/tree-log.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 18009095908b..f8f708c02462 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -1778,7 +1778,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
struct btrfs_path *path)
{
- int ret;
+ int ret = 0;
struct btrfs_key key;
struct inode *inode;
@@ -1791,6 +1791,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
break;
if (ret == 1) {
+ ret = 0;
if (path->slots[0] == 0)
break;
path->slots[0]--;
@@ -1803,17 +1804,19 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
ret = btrfs_del_item(trans, root, path);
if (ret)
- goto out;
+ break;
btrfs_release_path(path);
inode = read_one_inode(root, key.offset);
- if (!inode)
- return -EIO;
+ if (!inode) {
+ ret = -EIO;
+ break;
+ }
ret = fixup_inode_link_count(trans, root, inode);
iput(inode);
if (ret)
- goto out;
+ break;
/*
* fixup on a directory may create new entries,
@@ -1822,8 +1825,6 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
*/
key.offset = (u64)-1;
}
- ret = 0;
-out:
btrfs_release_path(path);
return ret;
}
--
2.26.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs: fixup error handling in fixup_inode_link_counts
2021-05-19 17:13 [PATCH] btrfs: fixup error handling in fixup_inode_link_counts Josef Bacik
@ 2021-05-21 7:46 ` Johannes Thumshirn
2021-05-21 12:38 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: Johannes Thumshirn @ 2021-05-21 7:46 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] 3+ messages in thread
* Re: [PATCH] btrfs: fixup error handling in fixup_inode_link_counts
2021-05-19 17:13 [PATCH] btrfs: fixup error handling in fixup_inode_link_counts Josef Bacik
2021-05-21 7:46 ` Johannes Thumshirn
@ 2021-05-21 12:38 ` David Sterba
1 sibling, 0 replies; 3+ messages in thread
From: David Sterba @ 2021-05-21 12:38 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, kernel-team
On Wed, May 19, 2021 at 01:13:15PM -0400, Josef Bacik wrote:
> This function has the following pattern
>
> while (1) {
> ret = whatever();
> if (ret)
> goto out;
> }
> ret = 0
> out:
> return ret;
>
> However several places in this while loop we simply break; when there's
> a problem, thus clearing the return value, and in one case we do a
> return -EIO, and leak the memory for the path.
>
> Fix this by re-arranging the loop to deal with ret == 1 coming from
> btrfs_search_slot, and then simply delete the
>
> ret = 0;
> out:
>
> bit so everybody can break if there is an error, which will allow for
> proper error handling to occur.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> fs/btrfs/tree-log.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 18009095908b..f8f708c02462 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1778,7 +1778,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
> struct btrfs_root *root,
> struct btrfs_path *path)
> {
> - int ret;
> + int ret = 0;
> struct btrfs_key key;
> struct inode *inode;
>
> @@ -1791,6 +1791,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
> break;
>
> if (ret == 1) {
> + ret = 0;
> if (path->slots[0] == 0)
> break;
> path->slots[0]--;
> @@ -1803,17 +1804,19 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
>
> ret = btrfs_del_item(trans, root, path);
> if (ret)
> - goto out;
> + break;
>
> btrfs_release_path(path);
> inode = read_one_inode(root, key.offset);
> - if (!inode)
> - return -EIO;
> + if (!inode) {
> + ret = -EIO;
Not related to your patch, but forcing EIO does not seem to be
right. read_one_inode is a simple wrapper around read_one_inode that
returns ERR_PTR so we should perhaps return that
> + break;
> + }
>
> ret = fixup_inode_link_count(trans, root, inode);
> iput(inode);
> if (ret)
> - goto out;
> + break;
>
> /*
> * fixup on a directory may create new entries,
> @@ -1822,8 +1825,6 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
> */
> key.offset = (u64)-1;
> }
> - ret = 0;
> -out:
> btrfs_release_path(path);
> return ret;
> }
> --
> 2.26.3
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-21 12:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-19 17:13 [PATCH] btrfs: fixup error handling in fixup_inode_link_counts Josef Bacik
2021-05-21 7:46 ` Johannes Thumshirn
2021-05-21 12:38 ` 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.