* [PATCH 2/3] btrfs: Remove redundant memory barrier
2017-10-20 15:10 [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Nikolay Borisov
@ 2017-10-20 15:10 ` Nikolay Borisov
2017-10-30 12:55 ` David Sterba
2017-10-20 15:10 ` [PATCH 3/3] btrfs: Remove unused function Nikolay Borisov
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2017-10-20 15:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, mathieu.desnoyers, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/3] btrfs: Remove unused function
2017-10-20 15:10 [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Nikolay Borisov
2017-10-20 15:10 ` [PATCH 2/3] btrfs: Remove redundant memory barrier Nikolay Borisov
@ 2017-10-20 15:10 ` Nikolay Borisov
2017-10-20 15:26 ` [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Mathieu Desnoyers
2017-10-24 10:47 ` [PATCH v2] " Nikolay Borisov
3 siblings, 0 replies; 7+ messages in thread
From: Nikolay Borisov @ 2017-10-20 15:10 UTC (permalink / raw)
To: linux-btrfs; +Cc: dsterba, mathieu.desnoyers, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters
2017-10-20 15:10 [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Nikolay Borisov
2017-10-20 15:10 ` [PATCH 2/3] btrfs: Remove redundant memory barrier Nikolay Borisov
2017-10-20 15:10 ` [PATCH 3/3] btrfs: Remove unused function Nikolay Borisov
@ 2017-10-20 15:26 ` Mathieu Desnoyers
2017-10-24 10:47 ` [PATCH v2] " Nikolay Borisov
3 siblings, 0 replies; 7+ messages in thread
From: Mathieu Desnoyers @ 2017-10-20 15:26 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs, dsterba
----- 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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2] btrfs: Fix memory barriers usage with device stats counters
2017-10-20 15:10 [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Nikolay Borisov
` (2 preceding siblings ...)
2017-10-20 15:26 ` [PATCH 1/3] btrfs: Fix memory barriers usage with device stats counters Mathieu Desnoyers
@ 2017-10-24 10:47 ` Nikolay Borisov
2017-10-24 13:24 ` Mathieu Desnoyers
3 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2017-10-24 10:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: mathieu.desnoyers, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2] btrfs: Fix memory barriers usage with device stats counters
2017-10-24 10:47 ` [PATCH v2] " Nikolay Borisov
@ 2017-10-24 13:24 ` Mathieu Desnoyers
0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Desnoyers @ 2017-10-24 13:24 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: linux-btrfs
----- 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
^ permalink raw reply [flat|nested] 7+ messages in thread