[PATCH 1/2] btrfs: Remove unused function btrfs_reada_detach()

[PATCH 1/2] btrfs: Remove unused function btrfs_reada_detach()

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

btrfs_reada_detach() is not called by any function. Remove.

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

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index f2fd73e58ee6..2acbd8919611 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3700,7 +3700,6 @@ 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);
-void btrfs_reada_detach(void *handle);
 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 06713a8fe26b..0d357f8b65bc 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -24,7 +24,7 @@
  * To trigger a readahead, btrfs_reada_add must be called. It will start
  * a read ahead for the given range [start, end) on tree root. The returned
  * handle can either be used to wait on the readahead to finish
- * (btrfs_reada_wait), or to send it to the background (btrfs_reada_detach).
+ * (btrfs_reada_wait).
  *
  * The read ahead works as follows:
  * On btrfs_reada_add, the root of the tree is inserted into a radix_tree.
@@ -1036,13 +1036,6 @@ int btrfs_reada_wait(void *handle)
 }
 #endif
 
-void btrfs_reada_detach(void *handle)
-{
-	struct reada_control *rc = handle;
-
-	kref_put(&rc->refcnt, reada_control_release);
-}
-
 /*
  * Before removing a device (device replace or device remove ioctls), call this
  * function to wait for all existing readahead requests on the device and to
-- 
2.30.2

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

[Goldwyn Rodrigues]

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);
 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)) {
-- 
2.30.2

Re: [PATCH 1/2] btrfs: Remove unused function btrfs_reada_detach()

[Anand Jain]

On 07/04/2021 00:24, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs_reada_detach() is not called by any function. Remove.
> 

  btrfs_reada_detach() was never used.

  commit 48a3b6366f69 (btrfs: make static code static & remove dead code)
  spared it.

  IMO ok to remove btrfs_reada_detach().

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

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>   fs/btrfs/ctree.h | 1 -
>   fs/btrfs/reada.c | 9 +--------
>   2 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f2fd73e58ee6..2acbd8919611 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3700,7 +3700,6 @@ 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);
> -void btrfs_reada_detach(void *handle);
>   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 06713a8fe26b..0d357f8b65bc 100644
> --- a/fs/btrfs/reada.c
> +++ b/fs/btrfs/reada.c
> @@ -24,7 +24,7 @@
>    * To trigger a readahead, btrfs_reada_add must be called. It will start
>    * a read ahead for the given range [start, end) on tree root. The returned
>    * handle can either be used to wait on the readahead to finish
> - * (btrfs_reada_wait), or to send it to the background (btrfs_reada_detach).
> + * (btrfs_reada_wait).
>    *
>    * The read ahead works as follows:
>    * On btrfs_reada_add, the root of the tree is inserted into a radix_tree.
> @@ -1036,13 +1036,6 @@ int btrfs_reada_wait(void *handle)
>   }
>   #endif
>   
> -void btrfs_reada_detach(void *handle)
> -{
> -	struct reada_control *rc = handle;
> -
> -	kref_put(&rc->refcnt, reada_control_release);
> -}
> -
>   /*
>    * Before removing a device (device replace or device remove ioctls), call this
>    * function to wait for all existing readahead requests on the device and to
> 

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

[Anand Jain]

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)) {
> 

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

[riteshh]

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

Re: [PATCH 1/2] btrfs: Remove unused function btrfs_reada_detach()

[riteshh]

On 21/04/06 11:24AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> btrfs_reada_detach() is not called by any function. Remove.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h | 1 -
>  fs/btrfs/reada.c | 9 +--------
>  2 files changed, 1 insertion(+), 9 deletions(-)
>

Although not an expert in this area, but I would hopefully start doing more
reviews in btrfs and hence will be spending more time with it's code.

For this patch seems to be easy and trivial and looks fine to me.
Please feel free to add:-

Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Re: [PATCH 1/2] btrfs: Remove unused function btrfs_reada_detach()

[David Sterba]

On Tue, Apr 06, 2021 at 11:24:03AM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> btrfs_reada_detach() is not called by any function. Remove.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ctree.h | 1 -
>  fs/btrfs/reada.c | 9 +--------
>  2 files changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index f2fd73e58ee6..2acbd8919611 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3700,7 +3700,6 @@ 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);
> -void btrfs_reada_detach(void *handle);
>  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 06713a8fe26b..0d357f8b65bc 100644
> --- a/fs/btrfs/reada.c
> +++ b/fs/btrfs/reada.c
> @@ -24,7 +24,7 @@
>   * To trigger a readahead, btrfs_reada_add must be called. It will start
>   * a read ahead for the given range [start, end) on tree root. The returned
>   * handle can either be used to wait on the readahead to finish
> - * (btrfs_reada_wait), or to send it to the background (btrfs_reada_detach).
> + * (btrfs_reada_wait).

btrfs_reada_detach is part of public readahead API, providing the
background readahead.

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

[David Sterba]

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.