[1/8] btrfs: Make get_state_failrec return failrec directly
diff mbox series

Message ID 20200702122335.9117-2-nborisov@suse.com
State New
Headers show
Series
  • Corrupt counter improvement
Related show

Commit Message

Nikolay Borisov July 2, 2020, 12:23 p.m. UTC
Only failure that get_state_failrec can get is if there is no failurec
for the given address. There is no reason why the function should return
a status code and use a separate parameter for returning the actual
failure rec (if one is found). Simplify it by making the return type
a pointer and return ERR_PTR value in case of errors.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-io-tree.h |  4 ++--
 fs/btrfs/extent_io.c      | 23 ++++++++++++-----------
 2 files changed, 14 insertions(+), 13 deletions(-)

Comments

Josef Bacik July 2, 2020, 1:07 p.m. UTC | #1
On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> Only failure that get_state_failrec can get is if there is no failurec
> for the given address. There is no reason why the function should return
> a status code and use a separate parameter for returning the actual
> failure rec (if one is found). Simplify it by making the return type
> a pointer and return ERR_PTR value in case of errors.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/extent-io-tree.h |  4 ++--
>   fs/btrfs/extent_io.c      | 23 ++++++++++++-----------
>   2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
> index b6561455b3c4..62eef5b1dfc6 100644
> --- a/fs/btrfs/extent-io-tree.h
> +++ b/fs/btrfs/extent-io-tree.h
> @@ -233,8 +233,8 @@ bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
>   			       struct extent_state **cached_state);
>   
>   /* This should be reworked in the future and put elsewhere. */
> -int get_state_failrec(struct extent_io_tree *tree, u64 start,
> -		      struct io_failure_record **failrec);
> +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> +					    u64 start);
>   int set_state_failrec(struct extent_io_tree *tree, u64 start,
>   		      struct io_failure_record *failrec);
>   void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start,
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 8a7e9da74b87..6f0891ad353b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2121,12 +2121,12 @@ int set_state_failrec(struct extent_io_tree *tree, u64 start,
>   	return ret;
>   }
>   
> -int get_state_failrec(struct extent_io_tree *tree, u64 start,
> -		      struct io_failure_record **failrec)
> +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> +					    u64 start)
>   {
>   	struct rb_node *node;
>   	struct extent_state *state;
> -	int ret = 0;
> +	struct io_failure_record *failrec;

Seems we can just do

struct io_failure_record *failrec = ERR_PTR(-ENOENT);

here and avoid the extra stuff below, as we only ever return -ENOENT on failure. 
  Thanks,

Josef
David Sterba July 2, 2020, 1:25 p.m. UTC | #2
On Thu, Jul 02, 2020 at 09:07:51AM -0400, Josef Bacik wrote:
> On 7/2/20 8:23 AM, Nikolay Borisov wrote:
> > +struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
> > +					    u64 start)
> >   {
> >   	struct rb_node *node;
> >   	struct extent_state *state;
> > -	int ret = 0;
> > +	struct io_failure_record *failrec;
> 
> Seems we can just do
> 
> struct io_failure_record *failrec = ERR_PTR(-ENOENT);
> 
> here and avoid the extra stuff below, as we only ever return -ENOENT on failure. 

I'm not a fan of this pattern, setting the error code just before the
label is IMHO more clear and one does not have to look up the initial
value.

Patch
diff mbox series

diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
index b6561455b3c4..62eef5b1dfc6 100644
--- a/fs/btrfs/extent-io-tree.h
+++ b/fs/btrfs/extent-io-tree.h
@@ -233,8 +233,8 @@  bool btrfs_find_delalloc_range(struct extent_io_tree *tree, u64 *start,
 			       struct extent_state **cached_state);
 
 /* This should be reworked in the future and put elsewhere. */
-int get_state_failrec(struct extent_io_tree *tree, u64 start,
-		      struct io_failure_record **failrec);
+struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
+					    u64 start);
 int set_state_failrec(struct extent_io_tree *tree, u64 start,
 		      struct io_failure_record *failrec);
 void btrfs_free_io_failure_record(struct btrfs_inode *inode, u64 start,
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 8a7e9da74b87..6f0891ad353b 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2121,12 +2121,12 @@  int set_state_failrec(struct extent_io_tree *tree, u64 start,
 	return ret;
 }
 
-int get_state_failrec(struct extent_io_tree *tree, u64 start,
-		      struct io_failure_record **failrec)
+struct io_failure_record *get_state_failrec(struct extent_io_tree *tree,
+					    u64 start)
 {
 	struct rb_node *node;
 	struct extent_state *state;
-	int ret = 0;
+	struct io_failure_record *failrec;
 
 	spin_lock(&tree->lock);
 	/*
@@ -2135,18 +2135,19 @@  int get_state_failrec(struct extent_io_tree *tree, u64 start,
 	 */
 	node = tree_search(tree, start);
 	if (!node) {
-		ret = -ENOENT;
+		failrec = ERR_PTR(-ENOENT);
 		goto out;
 	}
 	state = rb_entry(node, struct extent_state, rb_node);
 	if (state->start != start) {
-		ret = -ENOENT;
+		failrec = ERR_PTR(-ENOENT);
 		goto out;
 	}
-	*failrec = state->failrec;
+
+	failrec = state->failrec;
 out:
 	spin_unlock(&tree->lock);
-	return ret;
+	return failrec;
 }
 
 /*
@@ -2376,8 +2377,8 @@  int clean_io_failure(struct btrfs_fs_info *fs_info,
 	if (!ret)
 		return 0;
 
-	ret = get_state_failrec(failure_tree, start, &failrec);
-	if (ret)
+	failrec = get_state_failrec(failure_tree, start);
+	if (IS_ERR(failrec))
 		return 0;
 
 	BUG_ON(!failrec->this_mirror);
@@ -2461,8 +2462,8 @@  int btrfs_get_io_failure_record(struct inode *inode, u64 start, u64 end,
 	int ret;
 	u64 logical;
 
-	ret = get_state_failrec(failure_tree, start, &failrec);
-	if (ret) {
+	failrec = get_state_failrec(failure_tree, start);
+	if (IS_ERR(failrec)) {
 		failrec = kzalloc(sizeof(*failrec), GFP_NOFS);
 		if (!failrec)
 			return -ENOMEM;