| Message ID | 20200702122335.9117-2-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Corrupt counter improvement | expand |
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
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.
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;
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(-)