Re: [PATCH 06/10] Btrfs: Fix the problem that the dirty flag of dev stats is cleared

[David Sterba <dsterba@suse.cz>]

On Thu, Jul 24, 2014 at 11:37:11AM +0800, Miao Xie wrote:
> The io error might happen during writing out the device stats, and the
> device stats information and dirty flag would be update at that time,
> but the current code didn't consider this case, just clear the dirty
> flag, it would cause that we forgot to write out the new device stats
> information. Fix it.
> 
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
>  fs/btrfs/volumes.c |  7 +++++--
>  fs/btrfs/volumes.h | 19 +++++++++++++++----
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 19188df..0d37746 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -159,6 +159,7 @@ static struct btrfs_device *__alloc_device(void)
>  
>  	spin_lock_init(&dev->reada_lock);
>  	atomic_set(&dev->reada_in_flight, 0);
> +	atomic_set(&dev->dev_stats_ccnt, 0);
>  	INIT_RADIX_TREE(&dev->reada_zones, GFP_NOFS & ~__GFP_WAIT);
>  	INIT_RADIX_TREE(&dev->reada_extents, GFP_NOFS & ~__GFP_WAIT);
>  
> @@ -6398,16 +6399,18 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
>  	struct btrfs_root *dev_root = fs_info->dev_root;
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>  	struct btrfs_device *device;
> +	int stats_cnt;
>  	int ret = 0;
>  
>  	mutex_lock(&fs_devices->device_list_mutex);
>  	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> -		if (!device->dev_stats_valid || !device->dev_stats_dirty)
> +		if (!device->dev_stats_valid || !btrfs_dev_stats_dirty(device))

The helper btrfs_dev_stats_dirty is used only once and IMHO not
necessary.
>  			continue;
>  
> +		stats_cnt = atomic_read(&device->dev_stats_ccnt);

Here it is opencoded anyway.

>  		ret = update_dev_stat_item(trans, dev_root, device);
>  		if (!ret)
> -			device->dev_stats_dirty = 0;
> +			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
>  	}
>  	mutex_unlock(&fs_devices->device_list_mutex);
>  
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6fcc8ea..0defd23 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -110,7 +110,9 @@ struct btrfs_device {
>  	/* disk I/O failure stats. For detailed description refer to
>  	 * enum btrfs_dev_stat_values in ioctl.h */
>  	int dev_stats_valid;
> -	int dev_stats_dirty; /* counters need to be written to disk */
> +
> +	/* Counter to record the change of device stats */
> +	atomic_t dev_stats_ccnt;

dev_stats_dirty is more descriptive, please keep it. The counter
semantics can be documented here.

>  	atomic_t dev_stat_values[BTRFS_DEV_STAT_VALUES_MAX];
>  };
>  
> @@ -359,11 +361,18 @@ unsigned long btrfs_full_stripe_len(struct btrfs_root *root,
>  int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
>  				struct btrfs_root *extent_root,
>  				u64 chunk_offset, u64 chunk_size);
> +
> +static inline int btrfs_dev_stats_dirty(struct btrfs_device *dev)
> +{
> +	return atomic_read(&dev->dev_stats_ccnt);

IMHO too trivial, not necessary.

> +}
> +
>  static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
>  				      int index)
>  {
>  	atomic_inc(dev->dev_stat_values + index);
> -	dev->dev_stats_dirty = 1;

> +	smp_mb__before_atomic();
> +	atomic_inc(&dev->dev_stats_ccnt);

Please put the two lines into a wrapper, 3 times repeating the same is
worth it.

> @@ -378,7 +387,8 @@ static inline int btrfs_dev_stat_read_and_reset(struct btrfs_device *dev,
>  	int ret;
>  
>  	ret = atomic_xchg(dev->dev_stat_values + index, 0);
> -	dev->dev_stats_dirty = 1;
> +	smp_mb__before_atomic();
> +	atomic_inc(&dev->dev_stats_ccnt);

> @@ -386,7 +396,8 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
>  				      int index, unsigned long val)
>  {
>  	atomic_set(dev->dev_stat_values + index, val);
> -	dev->dev_stats_dirty = 1;
> +	smp_mb__before_atomic();
> +	atomic_inc(&dev->dev_stats_ccnt);