diff mbox series

[2/2] btrfs: Use reada_control pointer instead of void pointer

Message ID 20210406162404.11746-2-rgoldwyn@suse.de (mailing list archive)
State New, archived
Headers show
Series [1/2] btrfs: Remove unused function btrfs_reada_detach() | expand

Commit Message

Goldwyn Rodrigues April 6, 2021, 4:24 p.m. UTC 
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Since struct reada_control is defined in ctree.h,
Use struct reada_control pointer as a function argument for
btrfs_reada_wait() instead of a void pointer in order
to avoid type-casting within the function.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ctree.h | 2 +-
 fs/btrfs/reada.c | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Anand Jain April 7, 2021, 4:40 a.m. UTC | #1 
On 07/04/2021 00:24, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Since struct reada_control is defined in ctree.h,
> Use struct reada_control pointer as a function argument for
> btrfs_reada_wait() instead of a void pointer in order > to avoid type-casting within the function.

yep.

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>


Reviewed-by: Anand Jain <anand.jain@oralce.com>


> ---
>   fs/btrfs/ctree.h | 2 +-
>   fs/btrfs/reada.c | 6 ++----
>   2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2acbd8919611..8bf434a4f014 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3699,7 +3699,7 @@ struct reada_control {
>   };
>   struct reada_control *btrfs_reada_add(struct btrfs_root *root,
>   			      struct btrfs_key *start, struct btrfs_key *end);
> -int btrfs_reada_wait(void *handle);
> +int btrfs_reada_wait(struct reada_control *rc);
>   int btree_readahead_hook(struct extent_buffer *eb, int err);
>   void btrfs_reada_remove_dev(struct btrfs_device *dev);
>   void btrfs_reada_undo_remove_dev(struct btrfs_device *dev);
> diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
> index 0d357f8b65bc..9bfa47cd3920 100644
> --- a/fs/btrfs/reada.c
> +++ b/fs/btrfs/reada.c
> @@ -998,9 +998,8 @@ struct reada_control *btrfs_reada_add(struct btrfs_root *root,
>   }
>   
>   #ifdef DEBUG
> -int btrfs_reada_wait(void *handle)
> +int btrfs_reada_wait(struct reada_control *rc)
>   {
> -	struct reada_control *rc = handle;
>   	struct btrfs_fs_info *fs_info = rc->fs_info;
>   
>   	while (atomic_read(&rc->elems)) {
> @@ -1018,9 +1017,8 @@ int btrfs_reada_wait(void *handle)
>   	return 0;
>   }
>   #else
> -int btrfs_reada_wait(void *handle)
> +int btrfs_reada_wait(struct reada_control *rc)
>   {
> -	struct reada_control *rc = handle;
>   	struct btrfs_fs_info *fs_info = rc->fs_info;
>   
>   	while (atomic_read(&rc->elems)) {
>
Ritesh Harjani April 7, 2021, 3:49 p.m. UTC | #2 
On 21/04/06 11:24AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Since struct reada_control is defined in ctree.h,
> Use struct reada_control pointer as a function argument for
> btrfs_reada_wait() instead of a void pointer in order
> to avoid type-casting within the function.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h | 2 +-
>  fs/btrfs/reada.c | 6 ++----
>  2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2acbd8919611..8bf434a4f014 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3699,7 +3699,7 @@ struct reada_control {
>  };
>  struct reada_control *btrfs_reada_add(struct btrfs_root *root,
>  			      struct btrfs_key *start, struct btrfs_key *end);
> -int btrfs_reada_wait(void *handle);
> +int btrfs_reada_wait(struct reada_control *rc);

While we are at it we may as well make this function return void.
Since we anyways always return 0 and no one seems to be handling the return
value anyways.


>  int btree_readahead_hook(struct extent_buffer *eb, int err);

I guess, you may want to look into return value from this function too.
I don't think that too is being checked by any of it's callers.

-ritesh
David Sterba April 23, 2021, 11:28 a.m. UTC | #3 
On Wed, Apr 07, 2021 at 09:19:46PM +0530, riteshh wrote:
> On 21/04/06 11:24AM, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Since struct reada_control is defined in ctree.h,
> > Use struct reada_control pointer as a function argument for
> > btrfs_reada_wait() instead of a void pointer in order
> > to avoid type-casting within the function.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  fs/btrfs/ctree.h | 2 +-
> >  fs/btrfs/reada.c | 6 ++----
> >  2 files changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 2acbd8919611..8bf434a4f014 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -3699,7 +3699,7 @@ struct reada_control {
> >  };
> >  struct reada_control *btrfs_reada_add(struct btrfs_root *root,
> >  			      struct btrfs_key *start, struct btrfs_key *end);
> > -int btrfs_reada_wait(void *handle);
> > +int btrfs_reada_wait(struct reada_control *rc);
> 
> While we are at it we may as well make this function return void.
> Since we anyways always return 0 and no one seems to be handling the return
> value anyways.

That nothing is handling the error is not a sufficient reason, it needs
to be at least verified that this is expected. Also if there are called
functions that could return error or just do the bug-on error handling
instead of proper error handling, the call path needs to be updated.
In this case it's reada_start_machine that needs to be fixed.

As readahead is only a hint, many errors are not considered fatal so the
actual fix could be to just return the error and let the callers decide.
btrfs_reada_wait could try to start it, if it fails, just bail out. Then
it would be ok to return void value.


> >  int btree_readahead_hook(struct extent_buffer *eb, int err);
> 
> I guess, you may want to look into return value from this function too.
> I don't think that too is being checked by any of it's callers.

That looks like it can follow the same pattern as described above.
diff mbox series

Patch

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2acbd8919611..8bf434a4f014 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3699,7 +3699,7 @@  struct reada_control {
 };
 struct reada_control *btrfs_reada_add(struct btrfs_root *root,
 			      struct btrfs_key *start, struct btrfs_key *end);
-int btrfs_reada_wait(void *handle);
+int btrfs_reada_wait(struct reada_control *rc);
 int btree_readahead_hook(struct extent_buffer *eb, int err);
 void btrfs_reada_remove_dev(struct btrfs_device *dev);
 void btrfs_reada_undo_remove_dev(struct btrfs_device *dev);
diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index 0d357f8b65bc..9bfa47cd3920 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -998,9 +998,8 @@  struct reada_control *btrfs_reada_add(struct btrfs_root *root,
 }
 
 #ifdef DEBUG
-int btrfs_reada_wait(void *handle)
+int btrfs_reada_wait(struct reada_control *rc)
 {
-	struct reada_control *rc = handle;
 	struct btrfs_fs_info *fs_info = rc->fs_info;
 
 	while (atomic_read(&rc->elems)) {
@@ -1018,9 +1017,8 @@  int btrfs_reada_wait(void *handle)
 	return 0;
 }
 #else
-int btrfs_reada_wait(void *handle)
+int btrfs_reada_wait(struct reada_control *rc)
 {
-	struct reada_control *rc = handle;
 	struct btrfs_fs_info *fs_info = rc->fs_info;
 
 	while (atomic_read(&rc->elems)) {