[PATCH 0/2][v2] Fix init for device stats for seed devices

[PATCH 0/2][v2] Fix init for device stats for seed devices

[Josef Bacik]

v1->v2:
- Rework initial fix to work with the new seed_list.
- Add a follow up patch to address the fact that we were ignoring errors on
  reading the device tree.

Hello,

We see a lot of messages in the fleet where we use seed devices because we're
not init'ing the stats for seed devices properly.  I have an xfstest that shows
this (which I need to update), and this patch fixes the problem.  It's
relatively straightforward, simply init the stats on seed devices at the same
time we init them on our primary devices.  Thanks,

Josef

Josef Bacik (2):
  btrfs: init device stats for seed devices
  btrfs: return error if we're unable to read device stats

 fs/btrfs/volumes.c | 93 +++++++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 38 deletions(-)

-- 
2.26.2

[PATCH 1/2] btrfs: init device stats for seed devices

[Josef Bacik]

We recently started recording device stats across the fleet, and noticed
a large increase in messages such as this

BTRFS warning (device dm-0): get dev_stats failed, not yet valid

on our tiers that use seed devices for their root devices.  This is
because we do not initialize the device stats for any seed devices if we
have a sprout device and mount using that sprout device.  The basic
steps for reproducing are

mkfs seed device
mount seed device
fill seed device
umount seed device
btrfstune -S 1 seed device
mount seed device
btrfs device add -f sprout device /mnt/wherever
umount /mnt/wherever
mount sprout device /mnt/wherever
btrfs device stats /mnt/wherever

This will fail with the above message in dmesg.

Fix this by iterating over the fs_devices->seed if they exist in
btrfs_init_dev_stats.  This fixed the problem and properly reports the
stats for both devices.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6c7c8819cb31..c0cea9f5fdbc 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7223,61 +7223,69 @@ static void btrfs_set_dev_stats_value(struct extent_buffer *eb,
 			    sizeof(val));
 }
 
-int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
+static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
+				   struct btrfs_device *device,
+				   struct btrfs_path *path)
 {
-	struct btrfs_key key;
 	struct btrfs_root *dev_root = fs_info->dev_root;
-	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
+	struct btrfs_dev_stats_item *ptr;
 	struct extent_buffer *eb;
-	int slot;
-	int ret = 0;
+	struct btrfs_key key;
+	int item_size;
+	int i, ret, slot;
+
+	key.objectid = BTRFS_DEV_STATS_OBJECTID;
+	key.type = BTRFS_PERSISTENT_ITEM_KEY;
+	key.offset = device->devid;
+	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
+	if (ret) {
+		for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++)
+			btrfs_dev_stat_set(device, i, 0);
+		device->dev_stats_valid = 1;
+		btrfs_release_path(path);
+		return;
+	}
+	slot = path->slots[0];
+	eb = path->nodes[0];
+	item_size = btrfs_item_size_nr(eb, slot);
+
+	ptr = btrfs_item_ptr(eb, slot,
+			     struct btrfs_dev_stats_item);
+
+	for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
+		if (item_size >= (1 + i) * sizeof(__le64))
+			btrfs_dev_stat_set(device, i,
+					   btrfs_dev_stats_value(eb, ptr, i));
+		else
+			btrfs_dev_stat_set(device, i, 0);
+	}
+
+	device->dev_stats_valid = 1;
+	btrfs_dev_stat_print_on_load(device);
+	btrfs_release_path(path);
+}
+
+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 i;
 
 	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) {
-		int item_size;
-		struct btrfs_dev_stats_item *ptr;
-
-		key.objectid = BTRFS_DEV_STATS_OBJECTID;
-		key.type = BTRFS_PERSISTENT_ITEM_KEY;
-		key.offset = device->devid;
-		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
-		if (ret) {
-			for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++)
-				btrfs_dev_stat_set(device, i, 0);
-			device->dev_stats_valid = 1;
-			btrfs_release_path(path);
-			continue;
-		}
-		slot = path->slots[0];
-		eb = path->nodes[0];
-		item_size = btrfs_item_size_nr(eb, slot);
-
-		ptr = btrfs_item_ptr(eb, slot,
-				     struct btrfs_dev_stats_item);
-
-		for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
-			if (item_size >= (1 + i) * sizeof(__le64))
-				btrfs_dev_stat_set(device, i,
-					btrfs_dev_stats_value(eb, ptr, i));
-			else
-				btrfs_dev_stat_set(device, i, 0);
-		}
-
-		device->dev_stats_valid = 1;
-		btrfs_dev_stat_print_on_load(device);
-		btrfs_release_path(path);
+	list_for_each_entry(device, &fs_devices->devices, dev_list)
+		__btrfs_init_dev_stats(fs_info, device, path);
+	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);
 	}
 	mutex_unlock(&fs_devices->device_list_mutex);
 
 	btrfs_free_path(path);
-	return ret < 0 ? ret : 0;
+	return 0;
 }
 
 static int update_dev_stat_item(struct btrfs_trans_handle *trans,
-- 
2.26.2

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[Anand Jain]

Seed read errors should remain persistent.
A similar update to btrfs_run_dev_stats() is required?



On 19/9/20 4:44 am, Josef Bacik wrote:
> We recently started recording device stats across the fleet, and noticed
> a large increase in messages such as this
> 
> BTRFS warning (device dm-0): get dev_stats failed, not yet valid
> 
> on our tiers that use seed devices for their root devices.  This is
> because we do not initialize the device stats for any seed devices if we
> have a sprout device and mount using that sprout device.  The basic
> steps for reproducing are
> 
> mkfs seed device
> mount seed device
> fill seed device
> umount seed device
> btrfstune -S 1 seed device
> mount seed device
> btrfs device add -f sprout device /mnt/wherever
> umount /mnt/wherever
> mount sprout device /mnt/wherever
> btrfs device stats /mnt/wherever
> 
> This will fail with the above message in dmesg.
> 
> Fix this by iterating over the fs_devices->seed if they exist in
> btrfs_init_dev_stats.  This fixed the problem and properly reports the
> stats for both devices.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   fs/btrfs/volumes.c | 88 +++++++++++++++++++++++++---------------------
>   1 file changed, 48 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6c7c8819cb31..c0cea9f5fdbc 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7223,61 +7223,69 @@ static void btrfs_set_dev_stats_value(struct extent_buffer *eb,
>   			    sizeof(val));
>   }
>   
> -int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
> +static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
> +				   struct btrfs_device *device,
> +				   struct btrfs_path *path)
>   {
> -	struct btrfs_key key;
>   	struct btrfs_root *dev_root = fs_info->dev_root;
> -	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
> +	struct btrfs_dev_stats_item *ptr;
>   	struct extent_buffer *eb;
> -	int slot;
> -	int ret = 0;
> +	struct btrfs_key key;
> +	int item_size;
> +	int i, ret, slot;
> +
> +	key.objectid = BTRFS_DEV_STATS_OBJECTID;
> +	key.type = BTRFS_PERSISTENT_ITEM_KEY;
> +	key.offset = device->devid;
> +	ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
> +	if (ret) {
> +		for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++)
> +			btrfs_dev_stat_set(device, i, 0);
> +		device->dev_stats_valid = 1;
> +		btrfs_release_path(path);
> +		return;
> +	}
> +	slot = path->slots[0];
> +	eb = path->nodes[0];
> +	item_size = btrfs_item_size_nr(eb, slot);
> +
> +	ptr = btrfs_item_ptr(eb, slot,
> +			     struct btrfs_dev_stats_item);
> +
> +	for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
> +		if (item_size >= (1 + i) * sizeof(__le64))
> +			btrfs_dev_stat_set(device, i,
> +					   btrfs_dev_stats_value(eb, ptr, i));
> +		else
> +			btrfs_dev_stat_set(device, i, 0);
> +	}
> +
> +	device->dev_stats_valid = 1;
> +	btrfs_dev_stat_print_on_load(device);
> +	btrfs_release_path(path);
> +}
> +
> +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 i;
>   
>   	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) {
> -		int item_size;
> -		struct btrfs_dev_stats_item *ptr;
> -
> -		key.objectid = BTRFS_DEV_STATS_OBJECTID;
> -		key.type = BTRFS_PERSISTENT_ITEM_KEY;
> -		key.offset = device->devid;
> -		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
> -		if (ret) {
> -			for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++)
> -				btrfs_dev_stat_set(device, i, 0);
> -			device->dev_stats_valid = 1;
> -			btrfs_release_path(path);
> -			continue;
> -		}
> -		slot = path->slots[0];
> -		eb = path->nodes[0];
> -		item_size = btrfs_item_size_nr(eb, slot);
> -
> -		ptr = btrfs_item_ptr(eb, slot,
> -				     struct btrfs_dev_stats_item);
> -
> -		for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
> -			if (item_size >= (1 + i) * sizeof(__le64))
> -				btrfs_dev_stat_set(device, i,
> -					btrfs_dev_stats_value(eb, ptr, i));
> -			else
> -				btrfs_dev_stat_set(device, i, 0);
> -		}
> -
> -		device->dev_stats_valid = 1;
> -		btrfs_dev_stat_print_on_load(device);
> -		btrfs_release_path(path);
> +	list_for_each_entry(device, &fs_devices->devices, dev_list)
> +		__btrfs_init_dev_stats(fs_info, device, path);
> +	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);
>   	}
>   	mutex_unlock(&fs_devices->device_list_mutex);
>   
>   	btrfs_free_path(path);
> -	return ret < 0 ? ret : 0;
> +	return 0;
>   }
>   
>   static int update_dev_stat_item(struct btrfs_trans_handle *trans,
> 

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[David Sterba]

On Sat, Sep 19, 2020 at 05:47:18AM +0800, Anand Jain wrote:
> 
> Seed read errors should remain persistent.

How? The seed device is read-only, we can't record the stats and
recording them on the sprout device has unclear meaning.

> A similar update to btrfs_run_dev_stats() is required?

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[Anand Jain]

On 22/9/20 4:25 am, David Sterba wrote:
> On Sat, Sep 19, 2020 at 05:47:18AM +0800, Anand Jain wrote:
>>
>> Seed read errors should remain persistent.
> 
> How? The seed device is read-only, we can't record the stats and
> recording them on the sprout device

yeah on the sprout.

 > has unclear meaning.

Ok. Not required then.

> 
>> A similar update to btrfs_run_dev_stats() is required?

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[Anand Jain]

On 19/9/20 4:44 am, Josef Bacik wrote:
> We recently started recording device stats across the fleet, and noticed
> a large increase in messages such as this
> 
> BTRFS warning (device dm-0): get dev_stats failed, not yet valid
> 
> on our tiers that use seed devices for their root devices.  This is
> because we do not initialize the device stats for any seed devices if we
> have a sprout device and mount using that sprout device.  The basic
> steps for reproducing are
> 
> mkfs seed device
> mount seed device
> fill seed device
> umount seed device
> btrfstune -S 1 seed device
> mount seed device
> btrfs device add -f sprout device /mnt/wherever
> umount /mnt/wherever
> mount sprout device /mnt/wherever
> btrfs device stats /mnt/wherever
> 
> This will fail with the above message in dmesg.
> 
> Fix this by iterating over the fs_devices->seed if they exist in
> btrfs_init_dev_stats.  This fixed the problem and properly reports the
> stats for both devices.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

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

Thanks, Anand

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[David Sterba]

On Fri, Sep 18, 2020 at 04:44:32PM -0400, Josef Bacik wrote:
> We recently started recording device stats across the fleet, and noticed
> a large increase in messages such as this
> 
> BTRFS warning (device dm-0): get dev_stats failed, not yet valid
> 
> on our tiers that use seed devices for their root devices.  This is
> because we do not initialize the device stats for any seed devices if we
> have a sprout device and mount using that sprout device.  The basic
> steps for reproducing are
> 
> mkfs seed device
> mount seed device
> fill seed device
> umount seed device
> btrfstune -S 1 seed device
> mount seed device
> btrfs device add -f sprout device /mnt/wherever
> umount /mnt/wherever
> mount sprout device /mnt/wherever
> btrfs device stats /mnt/wherever

Please turn that into a fstest.

Re: [PATCH 1/2] btrfs: init device stats for seed devices

[David Sterba]

On Fri, Sep 18, 2020 at 04:44:32PM -0400, Josef Bacik wrote:
> We recently started recording device stats across the fleet, and noticed
> a large increase in messages such as this
> 
> BTRFS warning (device dm-0): get dev_stats failed, not yet valid
> 
> on our tiers that use seed devices for their root devices.  This is
> because we do not initialize the device stats for any seed devices if we
> have a sprout device and mount using that sprout device.  The basic
> steps for reproducing are
> 
> mkfs seed device
> mount seed device
> fill seed device
> umount seed device
> btrfstune -S 1 seed device
> mount seed device
> btrfs device add -f sprout device /mnt/wherever
> umount /mnt/wherever
> mount sprout device /mnt/wherever
> btrfs device stats /mnt/wherever
> 
> This will fail with the above message in dmesg.
> 
> Fix this by iterating over the fs_devices->seed if they exist in
> btrfs_init_dev_stats.  This fixed the problem and properly reports the
> stats for both devices.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/volumes.c | 88 +++++++++++++++++++++++++---------------------
>  1 file changed, 48 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6c7c8819cb31..c0cea9f5fdbc 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7223,61 +7223,69 @@ static void btrfs_set_dev_stats_value(struct extent_buffer *eb,
>  			    sizeof(val));
>  }
>  
> -int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
> +static void __btrfs_init_dev_stats(struct btrfs_fs_info *fs_info,
> +				   struct btrfs_device *device,
> +				   struct btrfs_path *path)

__ is not necessary and can be avoided by naming the function properly,
as it initializes the stats on a device

>  {
> -	struct btrfs_key key;
>  	struct btrfs_root *dev_root = fs_info->dev_root;

The fs_info parameter is used only once, just to read the dev_root, that
is also used just once. Both can be obtained from the @device.

The path is passed for convenience as it's reused and allocated out of
mutex, so that's fine for a helper.

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

[Josef Bacik]

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

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,
-- 
2.26.2

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

[Anand Jain]

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>

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

[David Sterba]

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;
>  }