On 1/30/20 2:49 AM, David Sterba wrote:

On Wed, Jan 08, 2020 at 12:16:47PM +0800, Anand Jain wrote:

quoted

Add

  /sys/fs/btrfs/UUID/read_policy

attribute so that the read policy for the raid1 and raid10 chunks can be
tuned.

When this attribute is read, it shall show all available policies, and
the active policy is with in [ ], read_policy attribute can be written
using one of the items showed in the read.

For example:
cat /sys/fs/btrfs/UUID/read_policy
[by_pid]
echo by_pid > /sys/fs/btrfs/UUID/read_policy
echo -n by_pid > /sys/fs/btrfs/UUID/read_policy

Dropping "by_" is a good thing, but it should be removed everywhere.
Also '-n' to echo should not be necessary and the store handler of sysfs
should deal with that.

My reference was Block device's scheduler [1],

[1]
cat /sys/block/sda/queue/scheduler
[mq-deadline] kyber none

/sys/block/sda/queue$ echo mq-deadline > ./scheduler
/sys/block/sda/queue$ echo "mq-deadline " > ./scheduler
/sys/block/sda/queue$ echo " mq-deadline " > ./scheduler
/sys/block/sda/queue$ echo -n mq-deadline > ./scheduler
/sys/block/sda/queue$ echo -n " mq-deadline " > ./scheduler
/sys/block/sda/queue$ echo -n " mq-deadline test" > ./scheduler
echo: write error: Invalid argument
/sys/block/sda/queue$ echo -n "mq-deadline kyber" > ./scheduler
echo: write error: Invalid argument

We could allow both echo and echo -n.

quoted

Signed-off-by: Anand Jain <redacted>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
---
v3: rename [by_pid] to [pid]
v2: v2: check input len before strip and kstrdup

  fs/btrfs/sysfs.c   | 66 ++++++++++++++++++++++++++++++++++++++++++++++
  fs/btrfs/volumes.h |  1 +
  2 files changed, 67 insertions(+)

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 104a97586744..cc4a642878a1 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c

@@ -809,6 +809,71 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
  
  BTRFS_ATTR(, checksum, btrfs_checksum_show);
  
+static const inline char *btrfs_read_policy_name(enum btrfs_read_policy_type type)
+{
+	switch (type) {
+	case BTRFS_READ_BY_PID:
+		return "pid";
+	default:
+		return "null";
+	}
+}

A simple table should do, similar what we have for the compression
number -> string mapping.

  Yes. Much better thanks.

quoted

+
+static ssize_t btrfs_read_policy_show(struct kobject *kobj,
+				      struct kobj_attribute *a, char *buf)
+{
+	int i;
+	ssize_t len = 0;

As this is used as return value, plese rename it to 'ret'

  Ok.

quoted

+	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
+
+	for (i = 0; i < BTRFS_NR_READ_POLICY_TYPE; i++) {
+		if (len)
+			len += snprintf(buf + len, PAGE_SIZE, " ");

You can use the same thning for the separator as is in
supported_checksums_show, ie. (i == 0 ? "" : " ") and add one more %s to
the format.

  Ok.

quoted

+		if (fs_devices->read_policy == i)
+			len += snprintf(buf + len, PAGE_SIZE, "[%s]",
+					btrfs_read_policy_name(i));
+		else
+			len += snprintf(buf + len, PAGE_SIZE, "%s",
+					btrfs_read_policy_name(i));

Keeping the default and the rest as separte calls to snprintf is
probably better so with the separator it would be

		if (fs_devices->read_policy == i)
			len += snprintf(buf + len, PAGE_SIZE, "%s[%s]",
					(i == 0 ? "" : " "),
					btrfs_read_policy_name(i));
		else
			len += snprintf(buf + len, PAGE_SIZE, "%s%s",
					(i == 0 ? "" : " "),
					btrfs_read_policy_name(i));

  Yes. fixed.

quoted

+	}
+
+	len += snprintf(buf + len, PAGE_SIZE, "\n");
+
+	return len;
+}
+
+static ssize_t btrfs_read_policy_store(struct kobject *kobj,
+				       struct kobj_attribute *a,
+				       const char *buf, size_t len)
+{
+	int i;
+	char *stripped;
+	char *policy_name;
+	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
+
+	if (len > BTRFS_READ_POLICY_NAME_MAX)
+		return -EINVAL;
+
+	policy_name = kstrdup(buf, GFP_KERNEL);

Can you avoid the allocation? None of the sysfs handlers should need it.

quoted

+	if (!policy_name)
+		return -ENOMEM;
+
+	stripped = strstrip(policy_name);

So the allocation is to make a copy of a string to get rid of leading
and trailing whitespace. There shouldn't be any leading space that we
should care about, but anyway skip_spaces() can be used on a read-only
string just fine.

  Ah. Yes.

The trailing whitespace is for the potential '\n' that we want to
handle. But doing an allocation here is an overkill, you can add a
helper that will verify that there's no garbage at the end of the
string, once the policy string matches one of ours.

  ok. Good idea.

Thanks, Anand

quoted

+
+	for (i = 0; i < BTRFS_NR_READ_POLICY_TYPE; i++) {
+		if (strncmp(stripped, btrfs_read_policy_name(i),
+			    strlen(stripped)) == 0) {
+			fs_devices->read_policy = i;
+			kfree(policy_name);
+			return len;
+		}
+	}
+
+	kfree(policy_name);
+	return -EINVAL;
+}
+BTRFS_ATTR_RW(, read_policy, btrfs_read_policy_show, btrfs_read_policy_store);
+
  static const struct attribute *btrfs_attrs[] = {
  	BTRFS_ATTR_PTR(, label),
  	BTRFS_ATTR_PTR(, nodesize),

@@ -817,6 +882,7 @@ static const struct attribute *btrfs_attrs[] = {
  	BTRFS_ATTR_PTR(, quota_override),
  	BTRFS_ATTR_PTR(, metadata_uuid),
  	BTRFS_ATTR_PTR(, checksum),
+	BTRFS_ATTR_PTR(, read_policy),
  	NULL,
  };
  

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 46f4e2258203..fe1494d95893 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h

@@ -209,6 +209,7 @@ BTRFS_DEVICE_GETSET_FUNCS(disk_total_bytes);
  BTRFS_DEVICE_GETSET_FUNCS(bytes_used);
  
  /* read_policy types */
+#define BTRFS_READ_POLICY_NAME_MAX	12

And this can be dropped afterwards

quoted

  #define BTRFS_READ_POLICY_DEFAULT	BTRFS_READ_BY_PID
  enum btrfs_read_policy_type {
  	BTRFS_READ_BY_PID,
-- 
2.23.0