[v3,2/2] btrfs: sysfs, add read_policy attribute
diff mbox series

Message ID 20200108041647.2330-1-anand.jain@oracle.com
State New
Headers show
Series
  • Untitled series #225099
Related show

Commit Message

Anand Jain Jan. 8, 2020, 4:16 a.m. UTC
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

Signed-off-by: Anand Jain <anand.jain@oracle.com>
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(+)

Comments

David Sterba Jan. 29, 2020, 6:49 p.m. UTC | #1
On Wed, Jan 08, 2020 at 12:16:47PM +0800, Anand Jain wrote:
> 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.

> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 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.

> +
> +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'

> +	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.

> +		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));

> +	}
> +
> +	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.

> +	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.

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.

> +
> +	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

>  #define BTRFS_READ_POLICY_DEFAULT	BTRFS_READ_BY_PID
>  enum btrfs_read_policy_type {
>  	BTRFS_READ_BY_PID,
> -- 
> 2.23.0
Anand Jain Feb. 12, 2020, 2:24 p.m. UTC | #2
On 1/30/20 2:49 AM, David Sterba wrote:
> On Wed, Jan 08, 2020 at 12:16:47PM +0800, Anand Jain wrote:
>> 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.

>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> 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.

>> +
>> +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.

>> +	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.

>> +		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.

>> +	}
>> +
>> +	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.
> 
>> +	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

>> +
>> +	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
> 
>>   #define BTRFS_READ_POLICY_DEFAULT	BTRFS_READ_BY_PID
>>   enum btrfs_read_policy_type {
>>   	BTRFS_READ_BY_PID,
>> -- 
>> 2.23.0

Patch
diff mbox series

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";
+	}
+}
+
+static ssize_t btrfs_read_policy_show(struct kobject *kobj,
+				      struct kobj_attribute *a, char *buf)
+{
+	int i;
+	ssize_t len = 0;
+	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, " ");
+		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));
+	}
+
+	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);
+	if (!policy_name)
+		return -ENOMEM;
+
+	stripped = strstrip(policy_name);
+
+	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
 #define BTRFS_READ_POLICY_DEFAULT	BTRFS_READ_BY_PID
 enum btrfs_read_policy_type {
 	BTRFS_READ_BY_PID,