On 01/21/2019 07:56 PM, Steven Davies wrote:
On 2018-05-16 11:03, Anand Jain wrote:
Going back to an old patchset I was testing this weekend:
Adds the mount option:
mount -o read_mirror_policy=<devid>
To set the devid of the device which should be used for read. That
means all the normal reads will go to that particular device only.
This also helps testing and gives a better control for the test
scripts including mount context reads.
Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
Tested-By: Steven Davies <btrfs-list@xxxxxxxxxxx>
Thanks for testing.
---
fs/btrfs/super.c | 21 +++++++++++++++++++++
fs/btrfs/volumes.c | 10 ++++++++++
fs/btrfs/volumes.h | 2 ++
3 files changed, 33 insertions(+)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 21eff0ac1e4f..7ddecf4178a6 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -852,6 +852,27 @@ int btrfs_parse_options(struct btrfs_fs_info
*info, char *options,
BTRFS_READ_MIRROR_BY_PID;
break;
}
+
+ intarg = 0;
+ if (match_int(&args[0], &intarg) == 0) {
+ struct btrfs_device *device;
+
+ device = btrfs_find_device(info, intarg,
+ NULL, NULL);
+ if (!device) {
+ btrfs_err(info,
+ "read_mirror_policy: invalid devid %d",
+ intarg);
+ ret = -EINVAL;
+ goto out;
+ }
+ info->read_mirror_policy =
+ BTRFS_READ_MIRROR_BY_DEV;
+ set_bit(BTRFS_DEV_STATE_READ_MIRROR,
+ &device->dev_state);
Not an expert, but might this be overwritten with another state? e.g.
BTRFS_DEV_STATE_REPLACE_TGT. The device would then lose its READ_MIRROR
flag and the fs would always use the first device for reading.
It won't it is defined as bitmap.
+ break;
+ }
I noticed that it's possible to pass this option multiple times at
mount, which sets multiple devices as read mirrors. While that doesn't
do anything harmful, the code below will only use the first device. It
may be worth at least mentioning this in documentation.
There were few feedback if read_mirror_policy should be a mount
option or a sysfs or a property. IMO property is better as it would
be persistent. In sysfs and mount-option, the user or a config file
has to remember. Will fix.
+
ret = -EINVAL;
goto out;
case Opt_err:
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 64dba5c4cf33..3a9c3804ff17 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5220,6 +5220,16 @@ static int find_live_mirror(struct
btrfs_fs_info *fs_info,
num_stripes = map->num_stripes;
switch(fs_info->read_mirror_policy) {
+ case BTRFS_READ_MIRROR_BY_DEV:
+ preferred_mirror = first;
+ if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
+ &map->stripes[preferred_mirror].dev->dev_state))
+ break;
+ if (test_bit(BTRFS_DEV_STATE_READ_MIRROR,
+ &map->stripes[++preferred_mirror].dev->dev_state)) <--[*]
+ break;
+ preferred_mirror = first;
Why set preferred_mirror again? The only effect of re-setting it will be
to use the lowest devid (1) rather than the highest (2). Is there any
difference? Either way it should never happen, because the above code
traps it (except when the BTRFS_DEV_STATE_READ_MIRROR flag has been
changed as mentioned above).
Code at [*] above does ++preferred_mirror. So the following
preferred_mirror = first;
is not redundant.
From Goffredo's comment[1] on Timofey's similar effect patch, if it
becomes possible to have more mirrors in a RAID1/RAID10 fs then this
code will need to be updated to test dev_state for more than two
devices. Would it be sensible to implement this as a for loop straight
away?
Right. A loop is better, will add.
+ break;
case BTRFS_READ_MIRROR_DEFAULT:
case BTRFS_READ_MIRROR_BY_PID:
default:
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 953df9925832..55b2eebbf183 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -37,6 +37,7 @@ struct btrfs_pending_bios {
enum btrfs_read_mirror_type {
BTRFS_READ_MIRROR_DEFAULT,
BTRFS_READ_MIRROR_BY_PID,
+ BTRFS_READ_MIRROR_BY_DEV,
};
#define BTRFS_DEV_STATE_WRITEABLE (0)
@@ -44,6 +45,7 @@ enum btrfs_read_mirror_type {
#define BTRFS_DEV_STATE_MISSING (2)
#define BTRFS_DEV_STATE_REPLACE_TGT (3)
#define BTRFS_DEV_STATE_FLUSH_SENT (4)
+#define BTRFS_DEV_STATE_READ_MIRROR (5)
struct btrfs_device {
struct list_head dev_list;
Happy to add Tested-By on patch 1/3 too, but I haven't looked at 3/3 yet.
[1]: https://patchwork.kernel.org/patch/10678531/#22315779
Thanks,
Anand.
