Re: [PATCH] btrfs: init device stats for new devices

[Stefan Behrens <sbehrens@giantdisaster.de>]

On Mon, 30 Sep 2013 14:58:02 -0700, Zach Brown wrote:
>> I discovered one minor bug in BTRFS filesystem.
> 
> You sure did.
> 
>> ERROR: ioctl(BTRFS_IOC_GET_DEV_STATS) on /dev/sde failed: No such device
>>
>> But this is not true ... all specified devices exist and are members
>> of btrfs filesystem. In dmesg I see this:
>> ...
>> [973077.099118] btrfs: get dev_stats failed, not yet valid
>> ....
>>
>> What makes device statistics valid ? I tried doing full filesystem
>> scrub ... but it did not fix that issue.
> 
> The stats are only initialized (considered valid) for devices that are
> known at mount.  You could unmount and mount after adding (or replacing)
> new devices and they'd start returning stats.
> 
> The following (bad) patch illustrates the problem, but the code should
> be restructured so stats are reliably read as devices are added.
> 
> - z
> 
> From: Zach Brown <zab@redhat.com>
> Date: Mon, 30 Sep 2013 17:48:05 -0400
> Subject: [PATCH] btrfs: init device stats for new devices
> 
> Device stats are only initialized (read from tree items) on mount.
> Trying to read device stats after adding or replacing new devices will
> return errors.
> 
> This cheesy patch demonstrates the problem, but this should really be a
> natural side-effect of adding devices to the fs_devices list.  We have
> evidence that trying to do it by hand doesn't work.
> 
> Any preferences for how to restructure this?

btrfs_init_new_device() and btrfs_init_dev_replace_tgtdev() are the two
functions that allocate and initialize new btrfs_device structures after
a filesystem is mounted. The device->dev_stats_valid = 1 should be done
there IMO. Before, kzalloc() has set the statistic values to the correct
value zero for new devices.


> ---
>  fs/btrfs/dev-replace.c | 4 +++-
>  fs/btrfs/volumes.c     | 6 ++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 5d84443..7309096 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -556,7 +556,9 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
>  
>  	mutex_unlock(&dev_replace->lock_finishing_cancel_unmount);
>  
> -	return 0;
> +	ret = btrfs_init_dev_stats(root->fs_info);
> +
> +	return ret;
>  }
>  
>  static void btrfs_dev_replace_update_device_in_mapping_tree(
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 0431147..e4ccc9b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2126,6 +2126,9 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>  		ret = btrfs_commit_transaction(trans, root);
>  	}
>  
> +	if (!ret)
> +		ret = btrfs_init_dev_stats(root->fs_info);
> +
>  	return ret;
>  
>  error_trans:
> @@ -6060,6 +6063,9 @@ int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
>  		int item_size;
>  		struct btrfs_dev_stats_item *ptr;
>  
> +		if (device->dev_stats_valid)
> +			continue;
> +
>  		key.objectid = 0;
>  		key.type = BTRFS_DEV_STATS_KEY;
>  		key.offset = device->devid;
>