[PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters

[PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters

[Nikolay Borisov]

Commit addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
reworked the way device stats changes are tracked. A new atomic dev_stats_ccnt
counter was introduced which is incremented every time any of the device stats
counters are changed. This serves as a flag whether there are any pending stats
changes. However, this patch only partially implemented the correct memory
barriers necessary:

- It only ordered the stores to the counters but not the reads e.g. btrfs_run_dev_stats
- It completely omitted any comments documenting the intended design and how
the memory barriers pair with each-other

This patch provides the necessary comments as well as adds a missing smp_rmb in
btrfs_run_dev_stats. Furthermore since dev_stats_cnt is only a snapshot at best
there was no point in reading the counter twice - once in btrfs_dev_stats_dirty
and then again when assigning stats_cnt. Just collapse both reads into 1.

Fixes: addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.c | 16 ++++++++++++++--
 fs/btrfs/volumes.h | 12 ++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4de498817e8a..b6ab8c53477f 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7084,10 +7084,22 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
-		if (!device->dev_stats_valid || !btrfs_dev_stats_dirty(device))
+		stats_cnt = atomic_read(&device->dev_stats_ccnt);
+		if (!device->dev_stats_valid || stats_cnt == 0)
 			continue;
 
-		stats_cnt = atomic_read(&device->dev_stats_ccnt);
+
+		/*
+		 * There is a LOAD-LOAD control dependency between the value of
+		 * dev_stats_ccnt and updating the on-disk values which requires
+		 * reading the in-memory counters. Such control dependencies
+		 * require explicit read memory barriers.
+		 *
+		 * This memory barriers pairs with smp_mb__before_atomic in
+		 * btrfs_dev_stat_inc/btrfs_dev_stat_read_and_reset/btrfs_dev_stat_set
+		 */
+		smp_rmb();
+
 		ret = update_dev_stat_item(trans, fs_info, device);
 		if (!ret)
 			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6108fdfec67f..c5dd48eb7b3d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -498,6 +498,12 @@ static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
 				      int index)
 {
 	atomic_inc(dev->dev_stat_values + index);
+	/*
+	 * This memory barrier orders stores updating statistics before stores
+	 * updating dev_stats_ccnt.
+	 *
+	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
+	 */
 	smp_mb__before_atomic();
 	atomic_inc(&dev->dev_stats_ccnt);
 }
@@ -523,6 +529,12 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
 				      int index, unsigned long val)
 {
 	atomic_set(dev->dev_stat_values + index, val);
+	/*
+	 * This memory barrier orders stores updating statistics before stores
+	 * updating dev_stats_ccnt.
+	 *
+	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
+	 */
 	smp_mb__before_atomic();
 	atomic_inc(&dev->dev_stats_ccnt);
 }
-- 
2.7.4

[PATCH 2/3] btrfs: Remove redundant memory barrier

[Nikolay Borisov]

As per atomic_t.txt documentation :
 - RMW operations that have a return value are fully ordered;

atomic_xchg is one such operation so it already includes everything it needs
w.r.t memory ordering and add a comment ot be more explicit about that.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index c5dd48eb7b3d..0b8ddcbf8462 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -520,7 +520,13 @@ static inline int btrfs_dev_stat_read_and_reset(struct btrfs_device *dev,
 	int ret;
 
 	ret = atomic_xchg(dev->dev_stat_values + index, 0);
-	smp_mb__before_atomic();
+	/*
+	 * atomic_xchg implies a full memory barriers as per atomic_t.txt:
+	 * - RMW operations that have a return value are fully ordered;
+	 *
+	 * This implicit memory barriers is paired with the smp_rmb in
+	 * btrfs_run_dev_stats
+	 */
 	atomic_inc(&dev->dev_stats_ccnt);
 	return ret;
 }
-- 
2.7.4

[PATCH 3/3] btrfs: Remove unused function

[Nikolay Borisov]

It's sole callsite was removed in a previous patch so just nuke it for good.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/volumes.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 0b8ddcbf8462..3509a68d274f 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -489,11 +489,6 @@ int btrfs_finish_chunk_alloc(struct btrfs_trans_handle *trans,
 int btrfs_remove_chunk(struct btrfs_trans_handle *trans,
 		       struct btrfs_fs_info *fs_info, u64 chunk_offset);
 
-static inline int btrfs_dev_stats_dirty(struct btrfs_device *dev)
-{
-	return atomic_read(&dev->dev_stats_ccnt);
-}
-
 static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
 				      int index)
 {
-- 
2.7.4

Re: [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters

[Mathieu Desnoyers]

----- On Oct 20, 2017, at 11:10 AM, Nikolay Borisov nborisov@suse.com wrote:

> Commit addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is
> cleared")
> reworked the way device stats changes are tracked. A new atomic dev_stats_ccnt
> counter was introduced which is incremented every time any of the device stats
> counters are changed. This serves as a flag whether there are any pending stats
> changes. However, this patch only partially implemented the correct memory
> barriers necessary:
> 
> - It only ordered the stores to the counters but not the reads e.g.
> btrfs_run_dev_stats
> - It completely omitted any comments documenting the intended design and how
> the memory barriers pair with each-other
> 
> This patch provides the necessary comments as well as adds a missing smp_rmb in
> btrfs_run_dev_stats. Furthermore since dev_stats_cnt is only a snapshot at best
> there was no point in reading the counter twice - once in btrfs_dev_stats_dirty
> and then again when assigning stats_cnt. Just collapse both reads into 1.
> 
> Fixes: addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is
> cleared")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> fs/btrfs/volumes.c | 16 ++++++++++++++--
> fs/btrfs/volumes.h | 12 ++++++++++++
> 2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4de498817e8a..b6ab8c53477f 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7084,10 +7084,22 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle
> *trans,
> 
> 	mutex_lock(&fs_devices->device_list_mutex);
> 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> -		if (!device->dev_stats_valid || !btrfs_dev_stats_dirty(device))
> +		stats_cnt = atomic_read(&device->dev_stats_ccnt);
> +		if (!device->dev_stats_valid || stats_cnt == 0)
> 			continue;
> 
> -		stats_cnt = atomic_read(&device->dev_stats_ccnt);
> +
> +		/*
> +		 * There is a LOAD-LOAD control dependency between the value of
> +		 * dev_stats_ccnt and updating the on-disk values which requires
> +		 * reading the in-memory counters. Such control dependencies
> +		 * require explicit read memory barriers.
> +		 *
> +		 * This memory barriers pairs with smp_mb__before_atomic in
> +		 * btrfs_dev_stat_inc/btrfs_dev_stat_read_and_reset/btrfs_dev_stat_set

should be:

"This memory barriers pairs with smp_mb__before_atomic in
btrfs_dev_stat_inc/btrfs_dev_stat_set, and with the full
barrier implied by atomic_xchg in
btrfs_dev_stat_read_and_reset"

Thanks,

Mathieu


> +		 */
> +		smp_rmb();
> +
> 		ret = update_dev_stat_item(trans, fs_info, device);
> 		if (!ret)
> 			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6108fdfec67f..c5dd48eb7b3d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -498,6 +498,12 @@ static inline void btrfs_dev_stat_inc(struct btrfs_device
> *dev,
> 				      int index)
> {
> 	atomic_inc(dev->dev_stat_values + index);
> +	/*
> +	 * This memory barrier orders stores updating statistics before stores
> +	 * updating dev_stats_ccnt.
> +	 *
> +	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
> +	 */
> 	smp_mb__before_atomic();
> 	atomic_inc(&dev->dev_stats_ccnt);
> }
> @@ -523,6 +529,12 @@ static inline void btrfs_dev_stat_set(struct btrfs_device
> *dev,
> 				      int index, unsigned long val)
> {
> 	atomic_set(dev->dev_stat_values + index, val);
> +	/*
> +	 * This memory barrier orders stores updating statistics before stores
> +	 * updating dev_stats_ccnt.
> +	 *
> +	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
> +	 */
> 	smp_mb__before_atomic();
> 	atomic_inc(&dev->dev_stats_ccnt);
> }
> --
> 2.7.4

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

[PATCH v2] btrfs: Fix memory barriers usage with device stats counters

[Nikolay Borisov]

Commit addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
reworked the way device stats changes are tracked. A new atomic dev_stats_ccnt
counter was introduced which is incremented every time any of the device stats
counters are changed. This serves as a flag whether there are any pending stats
changes. However, this patch only partially implemented the correct memory
barriers necessary:

- It only ordered the stores to the counters but not the reads e.g. btrfs_run_dev_stats
- It completely omitted any comments documenting the intended design and how
the memory barriers pair with each-other

This patch provides the necessary comments as well as adds a missing smp_rmb in
btrfs_run_dev_stats. Furthermore since dev_stats_cnt is only a snapshot at best
there was no point in reading the counter twice - once in btrfs_dev_stats_dirty
and then again when assigning stats_cnt. Just collapse both reads into 1.

Fixes: addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

V2: 
 Reworded the comment in btrfs_run_dev_stats according to Mathieu Desnoyers'
 feedback 

 fs/btrfs/volumes.c | 18 ++++++++++++++++--
 fs/btrfs/volumes.h | 12 ++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4de498817e8a..7f8ed3e08ece 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7084,10 +7084,24 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
 
 	mutex_lock(&fs_devices->device_list_mutex);
 	list_for_each_entry(device, &fs_devices->devices, dev_list) {
-		if (!device->dev_stats_valid || !btrfs_dev_stats_dirty(device))
+		stats_cnt = atomic_read(&device->dev_stats_ccnt);
+		if (!device->dev_stats_valid || stats_cnt == 0)
 			continue;
 
-		stats_cnt = atomic_read(&device->dev_stats_ccnt);
+
+		/*
+		 * There is a LOAD-LOAD control dependency between the value of
+		 * dev_stats_ccnt and updating the on-disk values which requires
+		 * reading the in-memory counters. Such control dependencies
+		 * require explicit read memory barriers.
+		 *
+		 * This memory barriers pairs with smp_mb__before_atomic in
+		 * btrfs_dev_stat_inc/btrfs_dev_stat_set and with the full
+		 * barrier implied by atomic_xchg in
+		 * btrfs_dev_stats_read_and_reset
+		 */
+		smp_rmb();
+
 		ret = update_dev_stat_item(trans, fs_info, device);
 		if (!ret)
 			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 6108fdfec67f..c5dd48eb7b3d 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -498,6 +498,12 @@ static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
 				      int index)
 {
 	atomic_inc(dev->dev_stat_values + index);
+	/*
+	 * This memory barrier orders stores updating statistics before stores
+	 * updating dev_stats_ccnt.
+	 *
+	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
+	 */
 	smp_mb__before_atomic();
 	atomic_inc(&dev->dev_stats_ccnt);
 }
@@ -523,6 +529,12 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
 				      int index, unsigned long val)
 {
 	atomic_set(dev->dev_stat_values + index, val);
+	/*
+	 * This memory barrier orders stores updating statistics before stores
+	 * updating dev_stats_ccnt.
+	 *
+	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
+	 */
 	smp_mb__before_atomic();
 	atomic_inc(&dev->dev_stats_ccnt);
 }
-- 
2.7.4

Re: [PATCH v2] btrfs: Fix memory barriers usage with device stats counters

[Mathieu Desnoyers]

----- Nikolay Borisov <nborisov@suse.com> wrote:
> Commit addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
> reworked the way device stats changes are tracked. A new atomic dev_stats_ccnt
> counter was introduced which is incremented every time any of the device stats
> counters are changed. This serves as a flag whether there are any pending stats
> changes. However, this patch only partially implemented the correct memory
> barriers necessary:
> 
> - It only ordered the stores to the counters but not the reads e.g. btrfs_run_dev_stats
> - It completely omitted any comments documenting the intended design and how
> the memory barriers pair with each-other
> 
> This patch provides the necessary comments as well as adds a missing smp_rmb in
> btrfs_run_dev_stats. Furthermore since dev_stats_cnt is only a snapshot at best
> there was no point in reading the counter twice - once in btrfs_dev_stats_dirty
> and then again when assigning stats_cnt. Just collapse both reads into 1.
> 
> Fixes: addc3fa74e5b ("Btrfs: Fix the problem that the dirty flag of dev stats is cleared")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

> ---
> 
> V2: 
>  Reworded the comment in btrfs_run_dev_stats according to Mathieu Desnoyers'
>  feedback 
> 
>  fs/btrfs/volumes.c | 18 ++++++++++++++++--
>  fs/btrfs/volumes.h | 12 ++++++++++++
>  2 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4de498817e8a..7f8ed3e08ece 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7084,10 +7084,24 @@ int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
>  
>  	mutex_lock(&fs_devices->device_list_mutex);
>  	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> -		if (!device->dev_stats_valid || !btrfs_dev_stats_dirty(device))
> +		stats_cnt = atomic_read(&device->dev_stats_ccnt);
> +		if (!device->dev_stats_valid || stats_cnt == 0)
>  			continue;
>  
> -		stats_cnt = atomic_read(&device->dev_stats_ccnt);
> +
> +		/*
> +		 * There is a LOAD-LOAD control dependency between the value of
> +		 * dev_stats_ccnt and updating the on-disk values which requires
> +		 * reading the in-memory counters. Such control dependencies
> +		 * require explicit read memory barriers.
> +		 *
> +		 * This memory barriers pairs with smp_mb__before_atomic in
> +		 * btrfs_dev_stat_inc/btrfs_dev_stat_set and with the full
> +		 * barrier implied by atomic_xchg in
> +		 * btrfs_dev_stats_read_and_reset
> +		 */
> +		smp_rmb();
> +
>  		ret = update_dev_stat_item(trans, fs_info, device);
>  		if (!ret)
>  			atomic_sub(stats_cnt, &device->dev_stats_ccnt);
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 6108fdfec67f..c5dd48eb7b3d 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -498,6 +498,12 @@ static inline void btrfs_dev_stat_inc(struct btrfs_device *dev,
>  				      int index)
>  {
>  	atomic_inc(dev->dev_stat_values + index);
> +	/*
> +	 * This memory barrier orders stores updating statistics before stores
> +	 * updating dev_stats_ccnt.
> +	 *
> +	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
> +	 */
>  	smp_mb__before_atomic();
>  	atomic_inc(&dev->dev_stats_ccnt);
>  }
> @@ -523,6 +529,12 @@ static inline void btrfs_dev_stat_set(struct btrfs_device *dev,
>  				      int index, unsigned long val)
>  {
>  	atomic_set(dev->dev_stat_values + index, val);
> +	/*
> +	 * This memory barrier orders stores updating statistics before stores
> +	 * updating dev_stats_ccnt.
> +	 *
> +	 * It pairs with smp_rmb() in btrfs_run_dev_stats().
> +	 */
>  	smp_mb__before_atomic();
>  	atomic_inc(&dev->dev_stats_ccnt);
>  }
> -- 
> 2.7.4
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 2/3] btrfs: Remove redundant memory barrier

[David Sterba]

On Fri, Oct 20, 2017 at 06:10:58PM +0300, Nikolay Borisov wrote:
> As per atomic_t.txt documentation :
>  - RMW operations that have a return value are fully ordered;
> 
> atomic_xchg is one such operation so it already includes everything it needs
> w.r.t memory ordering and add a comment ot be more explicit about that.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>