btrfs: use (READ_/WRITE_)ONCE for fs_devices->read_policy

Commit Message

Naohiro Aota Feb. 2, 2024, 7:14 a.m. UTC

Since we can read/modify the value from the sysfs interface concurrently,
it would be better to protect it from compiler optimizations.

Currently, there is only one read policy BTRFS_READ_POLICY_PID available,
so no actual problem can happen now. This is a preparation for the future
expansion.

Signed-off-by: Naohiro Aota <[e-mail]>
---
 fs/btrfs/sysfs.c   |  7 ++++---
 fs/btrfs/volumes.c | 10 +++++-----
 2 files changed, 9 insertions(+), 8 deletions(-)

Comments

Anand Jain Feb. 2, 2024, 8:26 a.m. UTC | #1

On 2/2/24 12:44, Naohiro Aota wrote:
> Since we can read/modify the value from the sysfs interface concurrently,
> it would be better to protect it from compiler optimizations.
> 
> Currently, there is only one read policy BTRFS_READ_POLICY_PID available,
> so no actual problem can happen now. This is a preparation for the future
> expansion.
> 
> Signed-off-by: Naohiro Aota <[e-mail]>
Looks good.

Reviewed-by: Anand Jain <[e-mail]>

Thx.

David Sterba Feb. 2, 2024, 12:08 p.m. UTC | #2

On Fri, Feb 02, 2024 at 04:14:57PM +0900, Naohiro Aota wrote:
> Since we can read/modify the value from the sysfs interface concurrently,
> it would be better to protect it from compiler optimizations.
> 
> Currently, there is only one read policy BTRFS_READ_POLICY_PID available,
> so no actual problem can happen now. This is a preparation for the future
> expansion.
> 
> Signed-off-by: Naohiro Aota <[e-mail]>

Reviewed-by: David Sterba <[e-mail]>

Patch

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 84c05246ffd8..21586ecc35bf 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -1228,11 +1228,12 @@  static ssize_t btrfs_read_policy_show(struct kobject *kobj,
 				      struct kobj_attribute *a, char *buf)
 {
 	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
+	const enum btrfs_read_policy policy = READ_ONCE(fs_devices->read_policy);
 	ssize_t ret = 0;
 	int i;
 
 	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
-		if (fs_devices->read_policy == i)
+		if (policy == i)
 			ret += sysfs_emit_at(buf, ret, "%s[%s]",
 					 (ret == 0 ? "" : " "),
 					 btrfs_read_policy_name[i]);
@@ -1256,8 +1257,8 @@  static ssize_t btrfs_read_policy_store(struct kobject *kobj,
 
 	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
 		if (sysfs_streq(buf, btrfs_read_policy_name[i])) {
-			if (i != fs_devices->read_policy) {
-				fs_devices->read_policy = i;
+			if (i != READ_ONCE(fs_devices->read_policy)) {
+				WRITE_ONCE(fs_devices->read_policy, i);
 				btrfs_info(fs_devices->fs_info,
 					   "read policy set to '%s'",
 					   btrfs_read_policy_name[i]);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 474ab7ed65ea..224345658ea5 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5942,6 +5942,7 @@  static int find_live_mirror(struct btrfs_fs_info *fs_info,
 			    struct btrfs_chunk_map *map, int first,
 			    int dev_replace_is_ongoing)
 {
+	const enum btrfs_read_policy policy = READ_ONCE(fs_info->fs_devices->read_policy);
 	int i;
 	int num_stripes;
 	int preferred_mirror;
@@ -5956,13 +5957,12 @@  static int find_live_mirror(struct btrfs_fs_info *fs_info,
 	else
 		num_stripes = map->num_stripes;
 
-	switch (fs_info->fs_devices->read_policy) {
+	switch (policy) {
 	default:
 		/* Shouldn't happen, just warn and use pid instead of failing */
-		btrfs_warn_rl(fs_info,
-			      "unknown read_policy type %u, reset to pid",
-			      fs_info->fs_devices->read_policy);
-		fs_info->fs_devices->read_policy = BTRFS_READ_POLICY_PID;
+		btrfs_warn_rl(fs_info, "unknown read_policy type %u, reset to pid",
+			      policy);
+		WRITE_ONCE(fs_info->fs_devices->read_policy, BTRFS_READ_POLICY_PID);
 		fallthrough;
 	case BTRFS_READ_POLICY_PID:
 		preferred_mirror = first + (current->pid % num_stripes);