[PATCH 0/3 RESEND Rebased] readmirror feature

[PATCH 0/3 RESEND Rebased] readmirror feature

[Anand Jain]

These patches are tested to be working fine.

Function call chain  __btrfs_map_block()->find_live_mirror() uses
thread pid to determine the %mirror_num when the mirror_num=0.

This patch introduces a framework so that we can add policies to determine
the %mirror_num. And adds the devid as the readmirror policy.

The property is stored as an extented attributes of root inode
(BTRFS_FS_TREE_OBJECTID).
User provided devid list is validated against the fs_devices::dev_list.

 For example:
   Usage:
     btrfs property set <mnt> readmirror devid<n>[,<m>...]
     btrfs property set <mnt> readmirror ""

   mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
   btrfs prop set /btrfs readmirror devid1,2
   btrfs prop get /btrfs readmirror
    readmirror=devid1,2
   getfattr -n btrfs.readmirror --absolute-names /btrfs
    btrfs.readmirror="devid1,2"
   btrfs prop set /btrfs readmirror ""
   getfattr -n btrfs.readmirror --absolute-names /btrfs
    /btrfs: btrfs.readmirror: No such attribute
   btrfs prop get /btrfs readmirror

RFC->v1:
  Drops pid as one of the readmirror policy choices and as usual remains
  as default. And when the devid is reset the readmirror policy falls back
  to pid.
  Drops the mount -o readmirror idea, it can be added at a later point of
  time.
  Property now accepts more than 1 devid as readmirror device. As shown
  in the example above.

Anand Jain (3):
  btrfs: add inode pointer to prop_handler::validate()
  btrfs: add readmirror property framework
  btrfs: add readmirror devid property

 fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
 fs/btrfs/props.h   |   4 +-
 fs/btrfs/volumes.c |  25 +++++++++-
 fs/btrfs/volumes.h |   8 +++
 fs/btrfs/xattr.c   |   2 +-
 5 files changed, 150 insertions(+), 9 deletions(-)

-- 
2.20.1 (Apple Git-117)

[PATCH 1/3 RESEND Rebased] btrfs: add inode pointer to prop_handler::validate()

[Anand Jain]

In preparation to add the readmirror property, pass inode in the
prop_handler::validate() function vector, so that it can fetch
corresponding fs_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c | 12 +++++++-----
 fs/btrfs/props.h |  4 ++--
 fs/btrfs/xattr.c |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index e0469816c678..f9143f7c006d 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -17,7 +17,7 @@ static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
 struct prop_handler {
 	struct hlist_node node;
 	const char *xattr_name;
-	int (*validate)(const char *value, size_t len);
+	int (*validate)(struct inode *inode, const char *value, size_t len);
 	int (*apply)(struct inode *inode, const char *value, size_t len);
 	const char *(*extract)(struct inode *inode);
 	int inheritable;
@@ -55,7 +55,8 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
+int btrfs_validate_prop(struct inode *inode, const char *name,
+			const char *value, size_t value_len)
 {
 	const struct prop_handler *handler;
 
@@ -69,7 +70,7 @@ int btrfs_validate_prop(const char *name, const char *value, size_t value_len)
 	if (value_len == 0)
 		return 0;
 
-	return handler->validate(value, value_len);
+	return handler->validate(inode, value, value_len);
 }
 
 int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
@@ -252,7 +253,8 @@ int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path)
 	return ret;
 }
 
-static int prop_compression_validate(const char *value, size_t len)
+static int prop_compression_validate(struct inode *inode, const char *value,
+				     size_t len)
 {
 	if (!value)
 		return 0;
@@ -350,7 +352,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 		 * This is not strictly necessary as the property should be
 		 * valid, but in case it isn't, don't propagate it futher.
 		 */
-		ret = h->validate(value, strlen(value));
+		ret = h->validate(inode, value, strlen(value));
 		if (ret)
 			continue;
 
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 40b2c65b518c..5b1e39136f7f 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -13,8 +13,8 @@ void __init btrfs_props_init(void);
 int btrfs_set_prop(struct btrfs_trans_handle *trans, struct inode *inode,
 		   const char *name, const char *value, size_t value_len,
 		   int flags);
-int btrfs_validate_prop(const char *name, const char *value, size_t value_len);
-
+int btrfs_validate_prop(struct inode *inode, const char *name,
+			const char *value, size_t value_len);
 int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
 
 int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/xattr.c b/fs/btrfs/xattr.c
index 95d9aebff2c4..9bfe179717d3 100644
--- a/fs/btrfs/xattr.c
+++ b/fs/btrfs/xattr.c
@@ -378,7 +378,7 @@ static int btrfs_xattr_handler_set_prop(const struct xattr_handler *handler,
 	struct btrfs_root *root = BTRFS_I(inode)->root;
 
 	name = xattr_full_name(handler, name);
-	ret = btrfs_validate_prop(name, value, size);
+	ret = btrfs_validate_prop(inode, name, value, size);
 	if (ret)
 		return ret;
 
-- 
2.20.1 (Apple Git-117)

[PATCH 2/3 RESEND Rebased] btrfs: add readmirror property framework

[Anand Jain]

Function call chain  __btrfs_map_block()->find_live_mirror() uses
thread %pid to determine the %mirror_num for the read when mirror_num=0
in the argument.

This patch introduces a framework so that readmirror is a configurable
parameter, with default set to pid.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 fs/btrfs/volumes.c |  9 ++++++++-
 fs/btrfs/volumes.h |  6 ++++++
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index f9143f7c006d..0dc26a154a98 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -10,6 +10,7 @@
 #include "ctree.h"
 #include "xattr.h"
 #include "compression.h"
+#include "volumes.h"
 
 #define BTRFS_PROP_HANDLERS_HT_BITS 8
 static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
@@ -312,6 +313,39 @@ static const char *prop_compression_extract(struct inode *inode)
 	return NULL;
 }
 
+static int prop_readmirror_validate(struct inode *inode, const char *value,
+				    size_t len)
+{
+	struct btrfs_root *root = BTRFS_I(inode)->root;
+
+	if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
+		return -EINVAL;
+
+	if (!len)
+		return 0;
+
+	return -EINVAL;
+}
+
+static int prop_readmirror_apply(struct inode *inode, const char *value,
+				 size_t len)
+{
+	struct btrfs_fs_devices *fs_devices = btrfs_sb(inode->i_sb)->fs_devices;
+
+	fs_devices->readmirror_policy = BTRFS_READMIRROR_DEFAULT;
+
+	return 0;
+}
+
+static const char *prop_readmirror_extract(struct inode *inode)
+{
+	/*
+	 * readmirror policy is applied for the whole FS, inheritance is not
+	 * applicable.
+	 */
+	return NULL;
+}
+
 static struct prop_handler prop_handlers[] = {
 	{
 		.xattr_name = XATTR_BTRFS_PREFIX "compression",
@@ -320,6 +354,13 @@ static struct prop_handler prop_handlers[] = {
 		.extract = prop_compression_extract,
 		.inheritable = 1
 	},
+	{
+		.xattr_name = XATTR_BTRFS_PREFIX "readmirror",
+		.validate = prop_readmirror_validate,
+		.apply = prop_readmirror_apply,
+		.extract = prop_readmirror_extract,
+		.inheritable = 0
+	},
 };
 
 static int inherit_props(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index a13ddba1ebc3..d72850ed4f88 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5490,7 +5490,14 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
 	else
 		num_stripes = map->num_stripes;
 
-	preferred_mirror = first + current->pid % num_stripes;
+	switch(fs_info->fs_devices->readmirror_policy) {
+	case BTRFS_READMIRROR_DEFAULT:
+		/* fall through */
+	default:
+		/* readmirror as per thread pid */
+		preferred_mirror = first + current->pid % num_stripes;
+		break;
+	}
 
 	if (dev_replace_is_ongoing &&
 	    fs_info->dev_replace.cont_reading_from_srcdev_mode ==
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 7f6aa1816409..e985d2133c0a 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -219,6 +219,10 @@ BTRFS_DEVICE_GETSET_FUNCS(total_bytes);
 BTRFS_DEVICE_GETSET_FUNCS(disk_total_bytes);
 BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
 
+enum btrfs_readmirror_policy {
+	BTRFS_READMIRROR_DEFAULT,
+};
+
 struct btrfs_fs_devices {
 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
 	u8 metadata_uuid[BTRFS_FSID_SIZE];
@@ -269,6 +273,8 @@ struct btrfs_fs_devices {
 	struct kobject fsid_kobj;
 	struct kobject *device_dir_kobj;
 	struct completion kobj_unregister;
+
+	int readmirror_policy;
 };
 
 #define BTRFS_BIO_INLINE_CSUM_SIZE	64
-- 
2.20.1 (Apple Git-117)

[PATCH 3/3 RESEND Rebased] btrfs: add readmirror devid property

[Anand Jain]

Introduces devid readmirror property, to direct read IO to the specified
device(s).

The readmirror property is stored as extended attribute on the root
inode. The readmirror input format is devid1,2,3.. etc. And for the
each devid provided, a new flag BTRFS_DEV_STATE_READ_OPTIMISED is set.

As of now readmirror by devid supports only raid1s. Raid10 support has
to leverage device grouping feature, which is yet to be implemented.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/props.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/volumes.c | 16 +++++++++++
 fs/btrfs/volumes.h |  2 ++
 3 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 0dc26a154a98..6a789e1c150c 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -316,7 +316,10 @@ static const char *prop_compression_extract(struct inode *inode)
 static int prop_readmirror_validate(struct inode *inode, const char *value,
 				    size_t len)
 {
+	char *value_dup;
+	char *devid_str;
 	struct btrfs_root *root = BTRFS_I(inode)->root;
+	struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
 
 	if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
 		return -EINVAL;
@@ -324,16 +327,80 @@ static int prop_readmirror_validate(struct inode *inode, const char *value,
 	if (!len)
 		return 0;
 
-	return -EINVAL;
+	if (len <= 5 || strncmp("devid", value, 5))
+		return -EINVAL;
+
+	value_dup = kstrndup(value + 5, len - 5, GFP_KERNEL);
+	if (!value_dup)
+		return -ENOMEM;
+
+	while ((devid_str = strsep(&value_dup, ",")) != NULL) {
+		u64 devid;
+		struct btrfs_device *device;
+
+		if (kstrtoull(devid_str, 10, &devid)) {
+			kfree(value_dup);
+			return -EINVAL;
+		}
+
+		device = btrfs_find_device(fs_devices, devid, NULL, NULL, false);
+		if (!device) {
+			kfree(value_dup);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
 }
 
 static int prop_readmirror_apply(struct inode *inode, const char *value,
 				 size_t len)
 {
+	char *value_dup;
+	char *devid_str;
+	struct btrfs_device *device;
 	struct btrfs_fs_devices *fs_devices = btrfs_sb(inode->i_sb)->fs_devices;
 
+	if (value) {
+		value_dup = kstrndup(value + 5, len - 5, GFP_KERNEL);
+		if (!value_dup)
+			return -ENOMEM;
+	}
+
+	/* Both set and reset has to clear the exisiting values */
+	list_for_each_entry(device, &fs_devices->devices, dev_list) {
+		if (test_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
+			     &device->dev_state)) {
+			clear_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
+				  &device->dev_state);
+		}
+	}
 	fs_devices->readmirror_policy = BTRFS_READMIRROR_DEFAULT;
 
+	/* Its only reset so just return */
+	if (!value)
+		return 0;
+
+	while ((devid_str = strsep(&value_dup, ",")) != NULL) {
+		u64 devid;
+
+		/* Has been verified in validate() this will not fail */
+		if (kstrtoull(devid_str, 10, &devid)) {
+			kfree(value_dup);
+			return -EINVAL;
+		}
+
+		device = btrfs_find_device(fs_devices, devid, NULL, NULL, false);
+		if (!device) {
+			kfree(value_dup);
+			return -EINVAL;
+		}
+
+		set_bit(BTRFS_DEV_STATE_READ_OPTIMISED, &device->dev_state);
+		fs_devices->readmirror_policy = BTRFS_READMIRROR_DEVID;
+	}
+
+	kfree(value_dup);
 	return 0;
 }
 
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index d72850ed4f88..993645f4b5f6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5481,6 +5481,7 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
 	int preferred_mirror;
 	int tolerance;
 	struct btrfs_device *srcdev;
+	bool found = false;
 
 	ASSERT((map->type &
 		 (BTRFS_BLOCK_GROUP_RAID1_MASK | BTRFS_BLOCK_GROUP_RAID10)));
@@ -5491,6 +5492,21 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
 		num_stripes = map->num_stripes;
 
 	switch(fs_info->fs_devices->readmirror_policy) {
+	case BTRFS_READMIRROR_DEVID:
+		/* skip raid10 for now */
+		if (map->type & BTRFS_BLOCK_GROUP_RAID1) {
+			for (i = first; i < first + num_stripes; i++) {
+				if (test_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
+					     &map->stripes[i].dev->dev_state)) {
+					preferred_mirror = i;
+					found = true;
+					break;
+				}
+			}
+			if (found)
+				break;
+		}
+		/* fall through */
 	case BTRFS_READMIRROR_DEFAULT:
 		/* fall through */
 	default:
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index e985d2133c0a..55b0f3b28595 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -56,6 +56,7 @@ struct btrfs_io_geometry {
 #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_OPTIMISED	(5)
 
 struct btrfs_device {
 	struct list_head dev_list; /* device_list_mutex */
@@ -221,6 +222,7 @@ BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
 
 enum btrfs_readmirror_policy {
 	BTRFS_READMIRROR_DEFAULT,
+	BTRFS_READMIRROR_DEVID,
 };
 
 struct btrfs_fs_devices {
-- 
2.20.1 (Apple Git-117)

[PATCH 1/2 RESEND Rebased] btrfs-progs: add helper to create xattr name

[Anand Jain]

This is a preparatory patch to add more xattr attributes, care a helper
function to alloc xattr name.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 props.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/props.c b/props.c
index efa11180d4c5..3a498bd9e904 100644
--- a/props.c
+++ b/props.c
@@ -94,6 +94,21 @@ static int prop_label(enum prop_object_type type,
 	return ret;
 }
 
+static char *alloc_xattr_name(const char *name)
+{
+	char *xattr_name;
+
+	xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name) + 1);
+	if (!xattr_name)
+		ERR_PTR(-ENOMEM);
+
+	memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
+	memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
+	xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
+
+	return xattr_name;
+}
+
 static int prop_compression(enum prop_object_type type,
 			    const char *object,
 			    const char *name,
@@ -114,14 +129,11 @@ static int prop_compression(enum prop_object_type type,
 		goto out;
 	}
 
-	xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name) + 1);
-	if (!xattr_name) {
-		ret = -ENOMEM;
-		goto out;
+	xattr_name = alloc_xattr_name(name);
+	if (IS_ERR(xattr_name)) {
+		error("failed to alloc xattr_name %s: %m", object);
+		return PTR_ERR(xattr_name);
 	}
-	memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
-	memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
-	xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
 
 	if (value) {
 		if (strcmp(value, "no") == 0 || strcmp(value, "none") == 0)
-- 
1.8.3.1

[PATCH 2/2 RESEND Rebased] btrfs-progs: add readmirror policy

[Anand Jain]

This sets the readmirror=<parm> as a btrfs.<attr> extentded attribute.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 props.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/props.c b/props.c
index 3a498bd9e904..1d1a2c7f9d14 100644
--- a/props.c
+++ b/props.c
@@ -178,6 +178,53 @@ out:
 	return ret;
 }
 
+static int prop_readmirror(enum prop_object_type type, const char *object,
+			   const char *name, const char *value)
+{
+	int fd;
+	int ret;
+	char buf[256] = {0};
+	char *xattr_name;
+	DIR *dirstream = NULL;
+
+	fd = open_file_or_dir3(object, &dirstream, value ? O_RDWR : O_RDONLY);
+	if (fd < 0) {
+		ret = -errno;
+		error("failed to open %s: %m", object);
+		return ret;
+	}
+
+	xattr_name = alloc_xattr_name(name);
+	if (IS_ERR(xattr_name)) {
+		error("failed to alloc xattr_name %s: %m", object);
+		return PTR_ERR(xattr_name);
+	}
+
+	ret = 0;
+	if (value) {
+		if (fsetxattr(fd, xattr_name, value, strlen(value), 0) < 0) {
+			ret = -errno;
+			error("failed to set readmirror for %s: %m", object);
+		}
+	} else {
+		if (fgetxattr(fd, xattr_name, buf, 256) < 0) {
+			if (errno != ENOATTR) {
+				ret = -errno;
+				error("failed to get readmirror for %s: %m",
+				      object);
+			}
+		} else {
+			fprintf(stdout, "readmirror=%.*s\n", (int) strlen(buf),
+				buf);
+		}
+	}
+
+	free(xattr_name);
+	close_file_or_dir(fd, dirstream);
+
+	return ret;
+}
+
 const struct prop_handler prop_handlers[] = {
 	{"ro", "Set/get read-only flag of subvolume.", 0, prop_object_subvol,
 	 prop_read_only},
@@ -185,5 +232,7 @@ const struct prop_handler prop_handlers[] = {
 	 prop_object_dev | prop_object_root, prop_label},
 	{"compression", "Set/get compression for a file or directory", 0,
 	 prop_object_inode, prop_compression},
+	{"readmirror", "set/get readmirror policy for filesystem", 0,
+	 prop_object_root, prop_readmirror},
 	{NULL, NULL, 0, 0, NULL}
 };
-- 
1.8.3.1

Re: [PATCH 0/3 RESEND Rebased] readmirror feature

[Anand Jain]

Ping?


On 26/6/19 4:33 PM, Anand Jain wrote:
> These patches are tested to be working fine.
> 
> Function call chain  __btrfs_map_block()->find_live_mirror() uses
> thread pid to determine the %mirror_num when the mirror_num=0.
> 
> This patch introduces a framework so that we can add policies to determine
> the %mirror_num. And adds the devid as the readmirror policy.
> 
> The property is stored as an extented attributes of root inode
> (BTRFS_FS_TREE_OBJECTID).
> User provided devid list is validated against the fs_devices::dev_list.
> 
>   For example:
>     Usage:
>       btrfs property set <mnt> readmirror devid<n>[,<m>...]
>       btrfs property set <mnt> readmirror ""
> 
>     mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
>     btrfs prop set /btrfs readmirror devid1,2
>     btrfs prop get /btrfs readmirror
>      readmirror=devid1,2
>     getfattr -n btrfs.readmirror --absolute-names /btrfs
>      btrfs.readmirror="devid1,2"
>     btrfs prop set /btrfs readmirror ""
>     getfattr -n btrfs.readmirror --absolute-names /btrfs
>      /btrfs: btrfs.readmirror: No such attribute
>     btrfs prop get /btrfs readmirror
> 
> RFC->v1:
>    Drops pid as one of the readmirror policy choices and as usual remains
>    as default. And when the devid is reset the readmirror policy falls back
>    to pid.
>    Drops the mount -o readmirror idea, it can be added at a later point of
>    time.
>    Property now accepts more than 1 devid as readmirror device. As shown
>    in the example above.
> 
> Anand Jain (3):
>    btrfs: add inode pointer to prop_handler::validate()
>    btrfs: add readmirror property framework
>    btrfs: add readmirror devid property
> 
>   fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
>   fs/btrfs/props.h   |   4 +-
>   fs/btrfs/volumes.c |  25 +++++++++-
>   fs/btrfs/volumes.h |   8 +++
>   fs/btrfs/xattr.c   |   2 +-
>   5 files changed, 150 insertions(+), 9 deletions(-)
> 

Re: [PATCH 2/2 RESEND Rebased] btrfs-progs: add readmirror policy

[David Sterba]

On Wed, Jun 26, 2019 at 04:37:23PM +0800, Anand Jain wrote:
> This sets the readmirror=<parm> as a btrfs.<attr> extentded attribute.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  props.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/props.c b/props.c
> index 3a498bd9e904..1d1a2c7f9d14 100644
> --- a/props.c
> +++ b/props.c
> @@ -178,6 +178,53 @@ out:
>  	return ret;
>  }
>  
> +static int prop_readmirror(enum prop_object_type type, const char *object,
> +			   const char *name, const char *value)
> +{
> +	int fd;
> +	int ret;
> +	char buf[256] = {0};
> +	char *xattr_name;
> +	DIR *dirstream = NULL;
> +
> +	fd = open_file_or_dir3(object, &dirstream, value ? O_RDWR : O_RDONLY);
> +	if (fd < 0) {
> +		ret = -errno;
> +		error("failed to open %s: %m", object);
> +		return ret;
> +	}
> +
> +	xattr_name = alloc_xattr_name(name);
> +	if (IS_ERR(xattr_name)) {
> +		error("failed to alloc xattr_name %s: %m", object);
> +		return PTR_ERR(xattr_name);
> +	}
> +
> +	ret = 0;
> +	if (value) {
> +		if (fsetxattr(fd, xattr_name, value, strlen(value), 0) < 0) {
> +			ret = -errno;
> +			error("failed to set readmirror for %s: %m", object);
> +		}
> +	} else {
> +		if (fgetxattr(fd, xattr_name, buf, 256) < 0) {
> +			if (errno != ENOATTR) {
> +				ret = -errno;
> +				error("failed to get readmirror for %s: %m",
> +				      object);
> +			}
> +		} else {
> +			fprintf(stdout, "readmirror=%.*s\n", (int) strlen(buf),
> +				buf);
> +		}
> +	}
> +
> +	free(xattr_name);
> +	close_file_or_dir(fd, dirstream);
> +
> +	return ret;
> +}
> +
>  const struct prop_handler prop_handlers[] = {
>  	{"ro", "Set/get read-only flag of subvolume.", 0, prop_object_subvol,
>  	 prop_read_only},
> @@ -185,5 +232,7 @@ const struct prop_handler prop_handlers[] = {
>  	 prop_object_dev | prop_object_root, prop_label},
>  	{"compression", "Set/get compression for a file or directory", 0,
>  	 prop_object_inode, prop_compression},
> +	{"readmirror", "set/get readmirror policy for filesystem", 0,
> +	 prop_object_root, prop_readmirror},

For some unknown reason the object type for filesystem-wide props is
called prop_object_root, which is correct, but it got me confused first.

So the most reliable way to set it is

  $ btrfs prop set -t filesystem /path readmirror <VALUE>

and

  $ btrfs prop set /path readmirror <VALUE>

will auto-detect the object type by /path, but I'm not sure what exactly
does it do in case it's a mount point but not the toplevel subvolume.

Re: [PATCH 2/2 RESEND Rebased] btrfs-progs: add readmirror policy

[David Sterba]

On Wed, Jun 26, 2019 at 04:37:23PM +0800, Anand Jain wrote:
>  	 prop_object_inode, prop_compression},
> +	{"readmirror", "set/get readmirror policy for filesystem", 0,

The help text for the property is useless.

Re: [PATCH 3/3 RESEND Rebased] btrfs: add readmirror devid property

[David Sterba]

On Wed, Jun 26, 2019 at 04:34:02PM +0800, Anand Jain wrote:
> Introduces devid readmirror property, to direct read IO to the specified
> device(s).
> 
> The readmirror property is stored as extended attribute on the root
> inode.

So this is permanently stored on the filesystem, is the policy applied
at mount time?

> The readmirror input format is devid1,2,3.. etc. And for the
> each devid provided, a new flag BTRFS_DEV_STATE_READ_OPTIMISED is set.

I'd say it should be called PREFERRED, and the format specifier could
add ":" between devid and the numbers, like "devid:1,2,3", we'll
probably want more types in the future.

This is the first whole-filesystem property so we can't follow a naming
scheme, so we'll have to decide if we go with flat or hierarchical
naming with more generic categories like policy.mirror.read and similar.

> As of now readmirror by devid supports only raid1s. Raid10 support has
> to leverage device grouping feature, which is yet to be implemented.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/props.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/volumes.c | 16 +++++++++++
>  fs/btrfs/volumes.h |  2 ++
>  3 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 0dc26a154a98..6a789e1c150c 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -316,7 +316,10 @@ static const char *prop_compression_extract(struct inode *inode)
>  static int prop_readmirror_validate(struct inode *inode, const char *value,
>  				    size_t len)
>  {
> +	char *value_dup;
> +	char *devid_str;
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> +	struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
>  
>  	if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
>  		return -EINVAL;
> @@ -324,16 +327,80 @@ static int prop_readmirror_validate(struct inode *inode, const char *value,
>  	if (!len)
>  		return 0;
>  
> -	return -EINVAL;
> +	if (len <= 5 || strncmp("devid", value, 5))
> +		return -EINVAL;
> +
> +	value_dup = kstrndup(value + 5, len - 5, GFP_KERNEL);
> +	if (!value_dup)
> +		return -ENOMEM;
> +
> +	while ((devid_str = strsep(&value_dup, ",")) != NULL) {
> +		u64 devid;
> +		struct btrfs_device *device;
> +
> +		if (kstrtoull(devid_str, 10, &devid)) {
> +			kfree(value_dup);
> +			return -EINVAL;
> +		}
> +
> +		device = btrfs_find_device(fs_devices, devid, NULL, NULL, false);

Doesn't this need locking?

> +		if (!device) {
> +			kfree(value_dup);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  static int prop_readmirror_apply(struct inode *inode, const char *value,
>  				 size_t len)
>  {
> +	char *value_dup;
> +	char *devid_str;
> +	struct btrfs_device *device;
>  	struct btrfs_fs_devices *fs_devices = btrfs_sb(inode->i_sb)->fs_devices;
>  
> +	if (value) {
> +		value_dup = kstrndup(value + 5, len - 5, GFP_KERNEL);
> +		if (!value_dup)
> +			return -ENOMEM;
> +	}
> +
> +	/* Both set and reset has to clear the exisiting values */
> +	list_for_each_entry(device, &fs_devices->devices, dev_list) {
> +		if (test_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
> +			     &device->dev_state)) {
> +			clear_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
> +				  &device->dev_state);
> +		}
> +	}

And this one too.

>  	fs_devices->readmirror_policy = BTRFS_READMIRROR_DEFAULT;
>  
> +	/* Its only reset so just return */
> +	if (!value)
> +		return 0;
> +
> +	while ((devid_str = strsep(&value_dup, ",")) != NULL) {
> +		u64 devid;
> +
> +		/* Has been verified in validate() this will not fail */
> +		if (kstrtoull(devid_str, 10, &devid)) {
> +			kfree(value_dup);
> +			return -EINVAL;
> +		}
> +
> +		device = btrfs_find_device(fs_devices, devid, NULL, NULL, false);
> +		if (!device) {
> +			kfree(value_dup);
> +			return -EINVAL;
> +		}
> +
> +		set_bit(BTRFS_DEV_STATE_READ_OPTIMISED, &device->dev_state);
> +		fs_devices->readmirror_policy = BTRFS_READMIRROR_DEVID;
> +	}
> +
> +	kfree(value_dup);
>  	return 0;
>  }
>  
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d72850ed4f88..993645f4b5f6 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5481,6 +5481,7 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
>  	int preferred_mirror;
>  	int tolerance;
>  	struct btrfs_device *srcdev;
> +	bool found = false;
>  
>  	ASSERT((map->type &
>  		 (BTRFS_BLOCK_GROUP_RAID1_MASK | BTRFS_BLOCK_GROUP_RAID10)));
> @@ -5491,6 +5492,21 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
>  		num_stripes = map->num_stripes;
>  
>  	switch(fs_info->fs_devices->readmirror_policy) {
> +	case BTRFS_READMIRROR_DEVID:
> +		/* skip raid10 for now */
> +		if (map->type & BTRFS_BLOCK_GROUP_RAID1) {
> +			for (i = first; i < first + num_stripes; i++) {
> +				if (test_bit(BTRFS_DEV_STATE_READ_OPTIMISED,
> +					     &map->stripes[i].dev->dev_state)) {
> +					preferred_mirror = i;
> +					found = true;
> +					break;
> +				}
> +			}
> +			if (found)
> +				break;
> +		}
> +		/* fall through */
>  	case BTRFS_READMIRROR_DEFAULT:
>  		/* fall through */
>  	default:
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index e985d2133c0a..55b0f3b28595 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -56,6 +56,7 @@ struct btrfs_io_geometry {
>  #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_OPTIMISED	(5)
>  
>  struct btrfs_device {
>  	struct list_head dev_list; /* device_list_mutex */
> @@ -221,6 +222,7 @@ BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
>  
>  enum btrfs_readmirror_policy {
>  	BTRFS_READMIRROR_DEFAULT,
> +	BTRFS_READMIRROR_DEVID,
>  };
>  
>  struct btrfs_fs_devices {
> -- 
> 2.20.1 (Apple Git-117)

Re: [PATCH 2/3 RESEND Rebased] btrfs: add readmirror property framework

[David Sterba]

On Wed, Jun 26, 2019 at 04:34:01PM +0800, Anand Jain wrote:
> Function call chain  __btrfs_map_block()->find_live_mirror() uses
> thread %pid to determine the %mirror_num for the read when mirror_num=0
> in the argument.
> 
> This patch introduces a framework so that readmirror is a configurable
> parameter, with default set to pid.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/props.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/volumes.c |  9 ++++++++-
>  fs/btrfs/volumes.h |  6 ++++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index f9143f7c006d..0dc26a154a98 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -10,6 +10,7 @@
>  #include "ctree.h"
>  #include "xattr.h"
>  #include "compression.h"
> +#include "volumes.h"
>  
>  #define BTRFS_PROP_HANDLERS_HT_BITS 8
>  static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
> @@ -312,6 +313,39 @@ static const char *prop_compression_extract(struct inode *inode)
>  	return NULL;
>  }
>  
> +static int prop_readmirror_validate(struct inode *inode, const char *value,
> +				    size_t len)
> +{
> +	struct btrfs_root *root = BTRFS_I(inode)->root;
> +
> +	if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
> +		return -EINVAL;
> +
> +	if (!len)
> +		return 0;
> +
> +	return -EINVAL;
> +}
> +
> +static int prop_readmirror_apply(struct inode *inode, const char *value,
> +				 size_t len)
> +{
> +	struct btrfs_fs_devices *fs_devices = btrfs_sb(inode->i_sb)->fs_devices;
> +
> +	fs_devices->readmirror_policy = BTRFS_READMIRROR_DEFAULT;
> +
> +	return 0;
> +}
> +
> +static const char *prop_readmirror_extract(struct inode *inode)
> +{
> +	/*
> +	 * readmirror policy is applied for the whole FS, inheritance is not
> +	 * applicable.
> +	 */

Extract is the 'get' implementation of the property, not inheritance, or
I don't understand what does the comment refer to.

> +	return NULL;

The return value should reflect the status of the property, ie.
basically the same value that would set the current state.

> +}
> +
>  static struct prop_handler prop_handlers[] = {
>  	{
>  		.xattr_name = XATTR_BTRFS_PREFIX "compression",
> @@ -320,6 +354,13 @@ static struct prop_handler prop_handlers[] = {
>  		.extract = prop_compression_extract,
>  		.inheritable = 1
>  	},
> +	{
> +		.xattr_name = XATTR_BTRFS_PREFIX "readmirror",
> +		.validate = prop_readmirror_validate,
> +		.apply = prop_readmirror_apply,
> +		.extract = prop_readmirror_extract,
> +		.inheritable = 0
> +	},
>  };
>  
>  static int inherit_props(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index a13ddba1ebc3..d72850ed4f88 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5490,7 +5490,14 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
>  	else
>  		num_stripes = map->num_stripes;
>  
> -	preferred_mirror = first + current->pid % num_stripes;
> +	switch(fs_info->fs_devices->readmirror_policy) {
> +	case BTRFS_READMIRROR_DEFAULT:
> +		/* fall through */
> +	default:
> +		/* readmirror as per thread pid */
> +		preferred_mirror = first + current->pid % num_stripes;
> +		break;
> +	}
>  
>  	if (dev_replace_is_ongoing &&
>  	    fs_info->dev_replace.cont_reading_from_srcdev_mode ==
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 7f6aa1816409..e985d2133c0a 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -219,6 +219,10 @@ BTRFS_DEVICE_GETSET_FUNCS(total_bytes);
>  BTRFS_DEVICE_GETSET_FUNCS(disk_total_bytes);
>  BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
>  
> +enum btrfs_readmirror_policy {
> +	BTRFS_READMIRROR_DEFAULT,
> +};
> +
>  struct btrfs_fs_devices {
>  	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
>  	u8 metadata_uuid[BTRFS_FSID_SIZE];
> @@ -269,6 +273,8 @@ struct btrfs_fs_devices {
>  	struct kobject fsid_kobj;
>  	struct kobject *device_dir_kobj;
>  	struct completion kobj_unregister;
> +
> +	int readmirror_policy;
>  };
>  
>  #define BTRFS_BIO_INLINE_CSUM_SIZE	64
> -- 
> 2.20.1 (Apple Git-117)

Re: [PATCH 3/3 RESEND Rebased] btrfs: add readmirror devid property

[David Sterba]

On Tue, Jul 23, 2019 at 04:55:53PM +0200, David Sterba wrote:
> On Wed, Jun 26, 2019 at 04:34:02PM +0800, Anand Jain wrote:
> > Introduces devid readmirror property, to direct read IO to the specified
> > device(s).
> > 
> > The readmirror property is stored as extended attribute on the root
> > inode.
> 
> So this is permanently stored on the filesystem, is the policy applied
> at mount time?

Found in some previous version that it's not permanently stored, so
scratch that.

> > The readmirror input format is devid1,2,3.. etc. And for the
> > each devid provided, a new flag BTRFS_DEV_STATE_READ_OPTIMISED is set.
> 
> I'd say it should be called PREFERRED, and the format specifier could
> add ":" between devid and the numbers, like "devid:1,2,3", we'll
> probably want more types in the future.
> 
> This is the first whole-filesystem property so we can't follow a naming
> scheme, so we'll have to decide if we go with flat or hierarchical
> naming with more generic categories like policy.mirror.read and similar.

I'll have to go through the past discussions again, there was some
feedback regarding this I did not take into account.

Re: [PATCH 0/3 RESEND Rebased] readmirror feature

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2358 bytes --]



On 2019/6/26 下午4:33, Anand Jain wrote:
> These patches are tested to be working fine.
> 
> Function call chain  __btrfs_map_block()->find_live_mirror() uses
> thread pid to determine the %mirror_num when the mirror_num=0.
> 
> This patch introduces a framework so that we can add policies to determine
> the %mirror_num. And adds the devid as the readmirror policy.
> 
> The property is stored as an extented attributes of root inode
> (BTRFS_FS_TREE_OBJECTID).

This doesn't look right to me.

As readmirror should work at chunk layer, putting it into root tree
doesn't follow the layer separation of btrfs.

And furthermore, this breaks the XATTR schema. Normally we only have
XATTR item after an INODE item, not a ROOT_ITEM.

Is the on-disk format already accepted or still under design stage?

Thanks,
Qu

> User provided devid list is validated against the fs_devices::dev_list.
> 
>  For example:
>    Usage:
>      btrfs property set <mnt> readmirror devid<n>[,<m>...]
>      btrfs property set <mnt> readmirror ""
> 
>    mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
>    btrfs prop set /btrfs readmirror devid1,2
>    btrfs prop get /btrfs readmirror
>     readmirror=devid1,2
>    getfattr -n btrfs.readmirror --absolute-names /btrfs
>     btrfs.readmirror="devid1,2"
>    btrfs prop set /btrfs readmirror ""
>    getfattr -n btrfs.readmirror --absolute-names /btrfs
>     /btrfs: btrfs.readmirror: No such attribute
>    btrfs prop get /btrfs readmirror
> 
> RFC->v1:
>   Drops pid as one of the readmirror policy choices and as usual remains
>   as default. And when the devid is reset the readmirror policy falls back
>   to pid.
>   Drops the mount -o readmirror idea, it can be added at a later point of
>   time.
>   Property now accepts more than 1 devid as readmirror device. As shown
>   in the example above.
> 
> Anand Jain (3):
>   btrfs: add inode pointer to prop_handler::validate()
>   btrfs: add readmirror property framework
>   btrfs: add readmirror devid property
> 
>  fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
>  fs/btrfs/props.h   |   4 +-
>  fs/btrfs/volumes.c |  25 +++++++++-
>  fs/btrfs/volumes.h |   8 +++
>  fs/btrfs/xattr.c   |   2 +-
>  5 files changed, 150 insertions(+), 9 deletions(-)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3 RESEND Rebased] readmirror feature

[Anand Jain]

> On 24 Jul 2019, at 8:20 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> 
> 
> 
> On 2019/6/26 下午4:33, Anand Jain wrote:
>> These patches are tested to be working fine.
>> 
>> Function call chain  __btrfs_map_block()->find_live_mirror() uses
>> thread pid to determine the %mirror_num when the mirror_num=0.
>> 
>> This patch introduces a framework so that we can add policies to determine
>> the %mirror_num. And adds the devid as the readmirror policy.
>> 
>> The property is stored as an extented attributes of root inode
>> (BTRFS_FS_TREE_OBJECTID).
> 
> This doesn't look right to me.
> 
> As readmirror should work at chunk layer, putting it into root tree
> doesn't follow the layer separation of btrfs.
> 
> And furthermore, this breaks the XATTR schema. Normally we only have
> XATTR item after an INODE item, not a ROOT_ITEM.
> 
> Is the on-disk format already accepted or still under design stage?
> 

 I mentioned about the storage for this new property in the RFC patch, as I knew there will be some surprises.

 The advantage of using the XATTR on the ROOT_ITEM is there is no on-disk format update nor there is any new KEY, albeit it deviates from the traditional way of using the xattr. Also, this approach don’t need an ioctl, as things work using the existing get/set xattr interface.

 The other way I had in mind was to introduce a new Key in the dev-tree such as

    (BTRFS_READMIRROR_OBJECTID, BTRFS_PERSISTENT_ITEM_KEY, devid)

 Again the interface can be ioctl or the get/set xattr. If we have to use the xattr then irrespective which inode is used we would anyway store it in the dev-tree using the above key.

 This is still open for changes, the idea is to get a long lasting flexible design, so comments are welcome.

Thanks, Anand


> Thanks,
> Qu
> 
>> User provided devid list is validated against the fs_devices::dev_list.
>> 
>> For example:
>>   Usage:
>>     btrfs property set <mnt> readmirror devid<n>[,<m>...]
>>     btrfs property set <mnt> readmirror ""
>> 
>>   mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
>>   btrfs prop set /btrfs readmirror devid1,2
>>   btrfs prop get /btrfs readmirror
>>    readmirror=devid1,2
>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>    btrfs.readmirror="devid1,2"
>>   btrfs prop set /btrfs readmirror ""
>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>    /btrfs: btrfs.readmirror: No such attribute
>>   btrfs prop get /btrfs readmirror
>> 
>> RFC->v1:
>>  Drops pid as one of the readmirror policy choices and as usual remains
>>  as default. And when the devid is reset the readmirror policy falls back
>>  to pid.
>>  Drops the mount -o readmirror idea, it can be added at a later point of
>>  time.
>>  Property now accepts more than 1 devid as readmirror device. As shown
>>  in the example above.
>> 
>> Anand Jain (3):
>>  btrfs: add inode pointer to prop_handler::validate()
>>  btrfs: add readmirror property framework
>>  btrfs: add readmirror devid property
>> 
>> fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
>> fs/btrfs/props.h   |   4 +-
>> fs/btrfs/volumes.c |  25 +++++++++-
>> fs/btrfs/volumes.h |   8 +++
>> fs/btrfs/xattr.c   |   2 +-
>> 5 files changed, 150 insertions(+), 9 deletions(-)
>> 
> 

Re: [PATCH 2/3 RESEND Rebased] btrfs: add readmirror property framework

[Anand Jain]

> On 23 Jul 2019, at 10:57 PM, David Sterba <dsterba@suse.cz> wrote:
> 
> On Wed, Jun 26, 2019 at 04:34:01PM +0800, Anand Jain wrote:
>> Function call chain  __btrfs_map_block()->find_live_mirror() uses
>> thread %pid to determine the %mirror_num for the read when mirror_num=0
>> in the argument.
>> 
>> This patch introduces a framework so that readmirror is a configurable
>> parameter, with default set to pid.
>> 
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/props.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>> fs/btrfs/volumes.c |  9 ++++++++-
>> fs/btrfs/volumes.h |  6 ++++++
>> 3 files changed, 55 insertions(+), 1 deletion(-)
>> 
>> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
>> index f9143f7c006d..0dc26a154a98 100644
>> --- a/fs/btrfs/props.c
>> +++ b/fs/btrfs/props.c
>> @@ -10,6 +10,7 @@
>> #include "ctree.h"
>> #include "xattr.h"
>> #include "compression.h"
>> +#include "volumes.h"
>> 
>> #define BTRFS_PROP_HANDLERS_HT_BITS 8
>> static DEFINE_HASHTABLE(prop_handlers_ht, BTRFS_PROP_HANDLERS_HT_BITS);
>> @@ -312,6 +313,39 @@ static const char *prop_compression_extract(struct inode *inode)
>> 	return NULL;
>> }
>> 
>> +static int prop_readmirror_validate(struct inode *inode, const char *value,
>> +				    size_t len)
>> +{
>> +	struct btrfs_root *root = BTRFS_I(inode)->root;
>> +
>> +	if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
>> +		return -EINVAL;
>> +
>> +	if (!len)
>> +		return 0;
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int prop_readmirror_apply(struct inode *inode, const char *value,
>> +				 size_t len)
>> +{
>> +	struct btrfs_fs_devices *fs_devices = btrfs_sb(inode->i_sb)->fs_devices;
>> +
>> +	fs_devices->readmirror_policy = BTRFS_READMIRROR_DEFAULT;
>> +
>> +	return 0;
>> +}
>> +
>> +static const char *prop_readmirror_extract(struct inode *inode)
>> +{
>> +	/*
>> +	 * readmirror policy is applied for the whole FS, inheritance is not
>> +	 * applicable.
>> +	 */
> 
> Extract is the 'get' implementation of the property, not inheritance, or
> I don't understand what does the comment refer to.


prop_handler::extract() is only used by inherit_props(). Readmirror property is for the volume/fsid so prop_handler::inheritable is set to 0. So inherit_props() doesn’t call extract() for readmirror.
The getxattr still work using the xattr interface and will have to mount the root which is I think is ok which is similar to the admin only operations, otherwise we have to introduce a new ioctl.

Thanks, Anand


>> +	return NULL;
> 
> The return value should reflect the status of the property, ie.
> basically the same value that would set the current state.
> 
>> +}
>> +
>> static struct prop_handler prop_handlers[] = {
>> 	{
>> 		.xattr_name = XATTR_BTRFS_PREFIX "compression",
>> @@ -320,6 +354,13 @@ static struct prop_handler prop_handlers[] = {
>> 		.extract = prop_compression_extract,
>> 		.inheritable = 1
>> 	},
>> +	{
>> +		.xattr_name = XATTR_BTRFS_PREFIX "readmirror",
>> +		.validate = prop_readmirror_validate,
>> +		.apply = prop_readmirror_apply,
>> +		.extract = prop_readmirror_extract,
>> +		.inheritable = 0
>> +	},
>> };
>> 
>> static int inherit_props(struct btrfs_trans_handle *trans,
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index a13ddba1ebc3..d72850ed4f88 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -5490,7 +5490,14 @@ static int find_live_mirror(struct btrfs_fs_info *fs_info,
>> 	else
>> 		num_stripes = map->num_stripes;
>> 
>> -	preferred_mirror = first + current->pid % num_stripes;
>> +	switch(fs_info->fs_devices->readmirror_policy) {
>> +	case BTRFS_READMIRROR_DEFAULT:
>> +		/* fall through */
>> +	default:
>> +		/* readmirror as per thread pid */
>> +		preferred_mirror = first + current->pid % num_stripes;
>> +		break;
>> +	}
>> 
>> 	if (dev_replace_is_ongoing &&
>> 	    fs_info->dev_replace.cont_reading_from_srcdev_mode ==
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index 7f6aa1816409..e985d2133c0a 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -219,6 +219,10 @@ BTRFS_DEVICE_GETSET_FUNCS(total_bytes);
>> BTRFS_DEVICE_GETSET_FUNCS(disk_total_bytes);
>> BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
>> 
>> +enum btrfs_readmirror_policy {
>> +	BTRFS_READMIRROR_DEFAULT,
>> +};
>> +
>> struct btrfs_fs_devices {
>> 	u8 fsid[BTRFS_FSID_SIZE]; /* FS specific uuid */
>> 	u8 metadata_uuid[BTRFS_FSID_SIZE];
>> @@ -269,6 +273,8 @@ struct btrfs_fs_devices {
>> 	struct kobject fsid_kobj;
>> 	struct kobject *device_dir_kobj;
>> 	struct completion kobj_unregister;
>> +
>> +	int readmirror_policy;
>> };
>> 
>> #define BTRFS_BIO_INLINE_CSUM_SIZE	64
>> -- 
>> 2.20.1 (Apple Git-117)

Re: [PATCH 0/3 RESEND Rebased] readmirror feature

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4301 bytes --]



On 2019/7/24 上午10:26, Anand Jain wrote:
> 
> 
>> On 24 Jul 2019, at 8:20 AM, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2019/6/26 下午4:33, Anand Jain wrote:
>>> These patches are tested to be working fine.
>>>
>>> Function call chain  __btrfs_map_block()->find_live_mirror() uses
>>> thread pid to determine the %mirror_num when the mirror_num=0.
>>>
>>> This patch introduces a framework so that we can add policies to determine
>>> the %mirror_num. And adds the devid as the readmirror policy.
>>>
>>> The property is stored as an extented attributes of root inode
>>> (BTRFS_FS_TREE_OBJECTID).
>>
>> This doesn't look right to me.
>>
>> As readmirror should work at chunk layer, putting it into root tree
>> doesn't follow the layer separation of btrfs.
>>
>> And furthermore, this breaks the XATTR schema. Normally we only have
>> XATTR item after an INODE item, not a ROOT_ITEM.
>>
>> Is the on-disk format already accepted or still under design stage?
>>
> 
>  I mentioned about the storage for this new property in the RFC patch, as I knew there will be some surprises.
> 
>  The advantage of using the XATTR on the ROOT_ITEM is there is no on-disk format update nor there is any new KEY, albeit it deviates from the traditional way of using the xattr. Also, this approach don’t need an ioctl, as things work using the existing get/set xattr interface.
> 
>  The other way I had in mind was to introduce a new Key in the dev-tree such as
> 
>     (BTRFS_READMIRROR_OBJECTID, BTRFS_PERSISTENT_ITEM_KEY, devid)

I'd say, this is more generic.
If one day we have some more features, we can just add new objectid for it.
And to my point of view, it's easier to validate than some floating
XATTR in root tree, especially for tree checker.

The only minor comment for this key is the offset and where the tree it
belongs to.

If the readmirror policy is global, I'd prefer to put it into root tree
and set the key offset to 0.

If the policy is per-chunk, it would be more easier to understand by
putting it into chunk tree, and use chunk bytenr as key offset.

If the policy is per-device, then your current key looks pretty good to me.
> 
>  Again the interface can be ioctl or the get/set xattr. If we have to use the xattr then irrespective which inode is used we would anyway store it in the dev-tree using the above key.

The prop interface can be enhanced, as we have filesystem level prop
already, I see no reason why it can't handle things in other trees.

Thanks,
Qu

> 
>  This is still open for changes, the idea is to get a long lasting flexible design, so comments are welcome.
> 
> Thanks, Anand
> 
> 
>> Thanks,
>> Qu
>>
>>> User provided devid list is validated against the fs_devices::dev_list.
>>>
>>> For example:
>>>   Usage:
>>>     btrfs property set <mnt> readmirror devid<n>[,<m>...]
>>>     btrfs property set <mnt> readmirror ""
>>>
>>>   mkfs.btrfs -fq -draid1 -mraid1 /dev/sd[b-d] && mount /dev/sdb /btrfs
>>>   btrfs prop set /btrfs readmirror devid1,2
>>>   btrfs prop get /btrfs readmirror
>>>    readmirror=devid1,2
>>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>>    btrfs.readmirror="devid1,2"
>>>   btrfs prop set /btrfs readmirror ""
>>>   getfattr -n btrfs.readmirror --absolute-names /btrfs
>>>    /btrfs: btrfs.readmirror: No such attribute
>>>   btrfs prop get /btrfs readmirror
>>>
>>> RFC->v1:
>>>  Drops pid as one of the readmirror policy choices and as usual remains
>>>  as default. And when the devid is reset the readmirror policy falls back
>>>  to pid.
>>>  Drops the mount -o readmirror idea, it can be added at a later point of
>>>  time.
>>>  Property now accepts more than 1 devid as readmirror device. As shown
>>>  in the example above.
>>>
>>> Anand Jain (3):
>>>  btrfs: add inode pointer to prop_handler::validate()
>>>  btrfs: add readmirror property framework
>>>  btrfs: add readmirror devid property
>>>
>>> fs/btrfs/props.c   | 120 +++++++++++++++++++++++++++++++++++++++++++--
>>> fs/btrfs/props.h   |   4 +-
>>> fs/btrfs/volumes.c |  25 +++++++++-
>>> fs/btrfs/volumes.h |   8 +++
>>> fs/btrfs/xattr.c   |   2 +-
>>> 5 files changed, 150 insertions(+), 9 deletions(-)
>>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2 RESEND Rebased] btrfs-progs: add readmirror policy

[Anand Jain]

On 23/7/19 9:53 PM, David Sterba wrote:
> On Wed, Jun 26, 2019 at 04:37:23PM +0800, Anand Jain wrote:
>> This sets the readmirror=<parm> as a btrfs.<attr> extentded attribute.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   props.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/props.c b/props.c
>> index 3a498bd9e904..1d1a2c7f9d14 100644
>> --- a/props.c
>> +++ b/props.c
>> @@ -178,6 +178,53 @@ out:
>>   	return ret;
>>   }
>>   
>> +static int prop_readmirror(enum prop_object_type type, const char *object,
>> +			   const char *name, const char *value)
>> +{
>> +	int fd;
>> +	int ret;
>> +	char buf[256] = {0};
>> +	char *xattr_name;
>> +	DIR *dirstream = NULL;
>> +
>> +	fd = open_file_or_dir3(object, &dirstream, value ? O_RDWR : O_RDONLY);
>> +	if (fd < 0) {
>> +		ret = -errno;
>> +		error("failed to open %s: %m", object);
>> +		return ret;
>> +	}
>> +
>> +	xattr_name = alloc_xattr_name(name);
>> +	if (IS_ERR(xattr_name)) {
>> +		error("failed to alloc xattr_name %s: %m", object);
>> +		return PTR_ERR(xattr_name);
>> +	}
>> +
>> +	ret = 0;
>> +	if (value) {
>> +		if (fsetxattr(fd, xattr_name, value, strlen(value), 0) < 0) {
>> +			ret = -errno;
>> +			error("failed to set readmirror for %s: %m", object);
>> +		}
>> +	} else {
>> +		if (fgetxattr(fd, xattr_name, buf, 256) < 0) {
>> +			if (errno != ENOATTR) {
>> +				ret = -errno;
>> +				error("failed to get readmirror for %s: %m",
>> +				      object);
>> +			}
>> +		} else {
>> +			fprintf(stdout, "readmirror=%.*s\n", (int) strlen(buf),
>> +				buf);
>> +		}
>> +	}
>> +
>> +	free(xattr_name);
>> +	close_file_or_dir(fd, dirstream);
>> +
>> +	return ret;
>> +}
>> +
>>   const struct prop_handler prop_handlers[] = {
>>   	{"ro", "Set/get read-only flag of subvolume.", 0, prop_object_subvol,
>>   	 prop_read_only},
>> @@ -185,5 +232,7 @@ const struct prop_handler prop_handlers[] = {
>>   	 prop_object_dev | prop_object_root, prop_label},
>>   	{"compression", "Set/get compression for a file or directory", 0,
>>   	 prop_object_inode, prop_compression},
>> +	{"readmirror", "set/get readmirror policy for filesystem", 0,
>> +	 prop_object_root, prop_readmirror},
> 
> For some unknown reason the object type for filesystem-wide props is
> called prop_object_root, which is correct, but it got me confused first.
> 
> So the most reliable way to set it is
> 
>    $ btrfs prop set -t filesystem /path readmirror <VALUE>
> 
> and
> 
>    $ btrfs prop set /path readmirror <VALUE>
> 
> will auto-detect the object type by /path, but I'm not sure what exactly
> does it do in case it's a mount point but not the toplevel subvolume.
> 

If its not toplevel subvolume it fails in kernel with -EINVAL.

----
+static int prop_readmirror_validate(struct inode *inode, const char *value,
+                                   size_t len)
+{
+       struct btrfs_root *root = BTRFS_I(inode)->root;
+
+       if (root->root_key.objectid != BTRFS_FS_TREE_OBJECTID)
+               return -EINVAL;
-----