[PATCH v2] btrfs: make btrfs_destroy_delayed_refs() return void

[PATCH v2] btrfs: make btrfs_destroy_delayed_refs() return void

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

btrfs_destroy_delayed_refs() always returns 0 and its single caller does
not check its return value, as it also returns void, and so does the
callers' caller and so on. This is because we are in the transaction abort
path, where we have no way to deal with errors (we are in a critical
situation) and all cleanup of resources works in a best effort fashion.
So make btrfs_destroy_delayed_refs() return void.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Make it explicit in the changelog that we are in the transaction
    abort path and therefore have no way to deal with errors.

    V1 was part of a patchset that was merged except for this patch:
    https://lore.kernel.org/linux-btrfs/cover.1685363099.git.fdmanana@suse.com/

 fs/btrfs/disk-io.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 90f998ac68f0..0bd437fbe07d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4555,13 +4555,12 @@ static void btrfs_destroy_all_ordered_extents(struct btrfs_fs_info *fs_info)
 	btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
 }
 
-static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
-				      struct btrfs_fs_info *fs_info)
+static void btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
+				       struct btrfs_fs_info *fs_info)
 {
 	struct rb_node *node;
 	struct btrfs_delayed_ref_root *delayed_refs;
 	struct btrfs_delayed_ref_node *ref;
-	int ret = 0;
 
 	delayed_refs = &trans->delayed_refs;
 
@@ -4569,7 +4568,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 	if (atomic_read(&delayed_refs->num_entries) == 0) {
 		spin_unlock(&delayed_refs->lock);
 		btrfs_debug(fs_info, "delayed_refs has NO entry");
-		return ret;
+		return;
 	}
 
 	while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) {
@@ -4630,8 +4629,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 	btrfs_qgroup_destroy_extent_records(trans);
 
 	spin_unlock(&delayed_refs->lock);
-
-	return ret;
 }
 
 static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root)
-- 
2.34.1

Re: [PATCH v2] btrfs: make btrfs_destroy_delayed_refs() return void

[Qu Wenruo]

On 2023/6/2 19:19, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> btrfs_destroy_delayed_refs() always returns 0 and its single caller does
> not check its return value, as it also returns void, and so does the
> callers' caller and so on. This is because we are in the transaction abort
> path, where we have no way to deal with errors (we are in a critical
> situation) and all cleanup of resources works in a best effort fashion.
> So make btrfs_destroy_delayed_refs() return void.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>
> V2: Make it explicit in the changelog that we are in the transaction
>      abort path and therefore have no way to deal with errors.
>
>      V1 was part of a patchset that was merged except for this patch:
>      https://lore.kernel.org/linux-btrfs/cover.1685363099.git.fdmanana@suse.com/
>
>   fs/btrfs/disk-io.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 90f998ac68f0..0bd437fbe07d 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4555,13 +4555,12 @@ static void btrfs_destroy_all_ordered_extents(struct btrfs_fs_info *fs_info)
>   	btrfs_wait_ordered_roots(fs_info, U64_MAX, 0, (u64)-1);
>   }
>
> -static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
> -				      struct btrfs_fs_info *fs_info)
> +static void btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
> +				       struct btrfs_fs_info *fs_info)
>   {
>   	struct rb_node *node;
>   	struct btrfs_delayed_ref_root *delayed_refs;
>   	struct btrfs_delayed_ref_node *ref;
> -	int ret = 0;
>
>   	delayed_refs = &trans->delayed_refs;
>
> @@ -4569,7 +4568,7 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
>   	if (atomic_read(&delayed_refs->num_entries) == 0) {
>   		spin_unlock(&delayed_refs->lock);
>   		btrfs_debug(fs_info, "delayed_refs has NO entry");
> -		return ret;
> +		return;
>   	}
>
>   	while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) {
> @@ -4630,8 +4629,6 @@ static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
>   	btrfs_qgroup_destroy_extent_records(trans);
>
>   	spin_unlock(&delayed_refs->lock);
> -
> -	return ret;
>   }
>
>   static void btrfs_destroy_delalloc_inodes(struct btrfs_root *root)

Re: [PATCH v2] btrfs: make btrfs_destroy_delayed_refs() return void

[David Sterba]

On Fri, Jun 02, 2023 at 12:19:42PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> btrfs_destroy_delayed_refs() always returns 0 and its single caller does
> not check its return value, as it also returns void, and so does the
> callers' caller and so on. This is because we are in the transaction abort
> path, where we have no way to deal with errors (we are in a critical
> situation) and all cleanup of resources works in a best effort fashion.
> So make btrfs_destroy_delayed_refs() return void.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> 
> V2: Make it explicit in the changelog that we are in the transaction
>     abort path and therefore have no way to deal with errors.
> 
>     V1 was part of a patchset that was merged except for this patch:
>     https://lore.kernel.org/linux-btrfs/cover.1685363099.git.fdmanana@suse.com/

Added to misc-next, my previous objections were meant for clarity and
the core of the error handling and return value passing lies in
unpin_extent_range(), not so significant for transaction cleanup.