diff mbox series

[2/2] btrfs: return error if we're unable to read device stats

Message ID 6f50f5be859468da38bd504c0f78a97dbcd0306d.1600461724.git.josef@toxicpanda.com
State New, archived
Headers show
Series Fix init for device stats for seed devices | expand

Commit Message

Josef Bacik Sept. 18, 2020, 8:44 p.m. UTC
I noticed when fixing device stats for seed devices that we simply threw
away the return value from btrfs_search_slot().  This is because we may
not have stat items, but we could very well get an error, and thus miss
reporting the error up the chain.  Fix this by returning ret if it's an
actual error, and then stop trying to init the rest of the devices stats
and return the error up the chain.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/volumes.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

Comments

Anand Jain Sept. 21, 2020, 11:54 p.m. UTC | #1
On 19/9/20 4:44 am, Josef Bacik wrote:
> I noticed when fixing device stats for seed devices that we simply threw
> away the return value from btrfs_search_slot().  This is because we may
> not have stat items, but we could very well get an error, and thus miss
> reporting the error up the chain.  Fix this by returning ret if it's an
> actual error, and then stop trying to init the rest of the devices stats
> and return the error up the chain.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>
David Sterba Sept. 22, 2020, 5:28 p.m. UTC | #2
On Fri, Sep 18, 2020 at 04:44:33PM -0400, Josef Bacik wrote:
> @@ -7270,22 +7271,30 @@ int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
>  	struct btrfs_device *device;
>  	struct btrfs_path *path = NULL;
> +	int ret = 0;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		return -ENOMEM;
>  
>  	mutex_lock(&fs_devices->device_list_mutex);
> -	list_for_each_entry(device, &fs_devices->devices, dev_list)
> -		__btrfs_init_dev_stats(fs_info, device, path);
> +	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> +		ret = __btrfs_init_dev_stats(fs_info, device, path);
> +		if (ret)
> +			goto out;
> +	}
>  	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
> -		list_for_each_entry(device, &seed_devs->devices, dev_list)
> -			__btrfs_init_dev_stats(fs_info, device, path);
> +		list_for_each_entry(device, &seed_devs->devices, dev_list) {
> +			ret = __btrfs_init_dev_stats(fs_info, device, path);
> +			if (ret)
> +				break;

This jumps out of the inner list_for_each and continues traversing the
seed_list, is that intentional? Or goto out should be here as well.

> +		}
>  	}
> +out:
>  	mutex_unlock(&fs_devices->device_list_mutex);
>  
>  	btrfs_free_path(path);
> -	return 0;
> +	return ret;
>  }
diff mbox series

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c0cea9f5fdbc..7cc677a7e544 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7223,9 +7223,9 @@  static void btrfs_set_dev_stats_value(struct extent_buffer *eb,
 			    sizeof(val));
 }
 
-static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
-				   struct btrfs_device *device,
-				   struct btrfs_path *path)
+static int __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
+				  struct btrfs_device *device,
+				  struct btrfs_path *path)
 {
 	struct btrfs_root *dev_root = fs_info->dev_root;
 	struct btrfs_dev_stats_item *ptr;
@@ -7243,7 +7243,7 @@  static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
 			btrfs_dev_stat_set(device, i, 0);
 		device->dev_stats_valid = 1;
 		btrfs_release_path(path);
-		return;
+		return ret < 0 ? ret : 0;
 	}
 	slot = path->slots[0];
 	eb = path->nodes[0];
@@ -7263,6 +7263,7 @@  static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
 	device->dev_stats_valid = 1;
 	btrfs_dev_stat_print_on_load(device);
 	btrfs_release_path(path);
+	return 0;
 }
 
 int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
@@ -7270,22 +7271,30 @@  int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs;
 	struct btrfs_device *device;
 	struct btrfs_path *path = NULL;
+	int ret = 0;
 
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
 
 	mutex_lock(&fs_devices->device_list_mutex);
-	list_for_each_entry(device, &fs_devices->devices, dev_list)
-		__btrfs_init_dev_stats(fs_info, device, path);
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+		ret = __btrfs_init_dev_stats(fs_info, device, path);
+		if (ret)
+			goto out;
+	}
 	list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) {
-		list_for_each_entry(device, &seed_devs->devices, dev_list)
-			__btrfs_init_dev_stats(fs_info, device, path);
+		list_for_each_entry(device, &seed_devs->devices, dev_list) {
+			ret = __btrfs_init_dev_stats(fs_info, device, path);
+			if (ret)
+				break;
+		}
 	}
+out:
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	btrfs_free_path(path);
-	return 0;
+	return ret;
 }
 
 static int update_dev_stat_item(struct btrfs_trans_handle *trans,