Re: [PATCH v3 2/2] btrfs: sysfs, add read_policy attribute

[Anand Jain <anand.jain@oracle.com>]

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