[v10,3/3] btrfs: create read policy sysfs attribute, pid

Commit Message

Anand Jain Oct. 28, 2020, 1:14 p.m. UTC

Add

 /sys/fs/btrfs/UUID/read_policy

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

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

For example:
  $cat /sys/fs/btrfs/UUID/read_policy
  [pid]
  $echo pid > /sys/fs/btrfs/UUID/read_policy

Signed-off-by: Anand Jain <[e-mail]>
Reviewed-by: Josef Bacik <[e-mail]>
---
v10: use scnprintf instead of snprintf
v9: fix C coding style, static const char*
v5: 
  Title rename: old: btrfs: sysfs, add read_policy attribute
  Uses the btrfs_strmatch() helper (BTRFS_READ_POLICY_NAME_MAX dropped).
  Use the table for the policy names.
  Rename len to ret.
  Use a simple logic to prefix space in btrfs_read_policy_show()
  Reviewed-by: Josef Bacik <[e-mail]> dropped.

v4:-
v3: rename [by_pid] to [pid]
v2: v2: check input len before strip and kstrdup

 fs/btrfs/sysfs.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

David Sterba Oct. 29, 2020, 4:45 p.m. UTC | #1

On Wed, Oct 28, 2020 at 09:14:47PM +0800, Anand Jain wrote:
> +static ssize_t btrfs_read_policy_store(struct kobject *kobj,
> +				       struct kobj_attribute *a,
> +				       const char *buf, size_t len)
> +{
> +	int i;
> +	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
> +
> +	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
> +		if (btrfs_strmatch(buf, btrfs_read_policy_name[i])) {

Does sysfs guarantee that the buf is nul terminated string or that it
contains a null at all? Because if not, the skip_space step of strmatch
could run out of the buffer.

Anand Jain Oct. 29, 2020, 7:30 p.m. UTC | #2

On 30/10/20 12:45 am, David Sterba wrote:
> On Wed, Oct 28, 2020 at 09:14:47PM +0800, Anand Jain wrote:
>> +static ssize_t btrfs_read_policy_store(struct kobject *kobj,
>> +				       struct kobj_attribute *a,
>> +				       const char *buf, size_t len)
>> +{
>> +	int i;
>> +	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
>> +
>> +	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
>> +		if (btrfs_strmatch(buf, btrfs_read_policy_name[i])) {
> 
> Does sysfs guarantee that the buf is nul terminated string or that it
> contains a null at all? Because if not, the skip_space step of strmatch
> could run out of the buffer.
> 

It does

[  173.555507]  ? btrfs_read_policy_store+0x3e/0x12d [btrfs]
[  173.555541]  ? kobj_attr_store+0x16/0x30
[  173.555562]  ? sysfs_kf_write+0x54/0x80
[  173.555582]  ? kernfs_fop_write+0xfa/0x290
[  173.555611]  ? vfs_write+0xee/0x2f0
[  173.555641]  ? ksys_write+0x80/0x170
[  173.555671]  ? __x64_sys_write+0x1e/0x30
[  173.555692]  ? do_syscall_64+0x4b/0x80
[  173.555708]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9


static ssize_t kernfs_fop_write(struct file *file, const char __user 
*user_buf,
                                 size_t count, loff_t *ppos)
{
::
         buf[len] = '\0';        /* guarantee string termination */


Thanks, Anand

David Sterba Oct. 29, 2020, 7:30 p.m. UTC | #3

On Fri, Oct 30, 2020 at 03:30:10AM +0800, Anand Jain wrote:
> 
> 
> On 30/10/20 12:45 am, David Sterba wrote:
> > On Wed, Oct 28, 2020 at 09:14:47PM +0800, Anand Jain wrote:
> >> +static ssize_t btrfs_read_policy_store(struct kobject *kobj,
> >> +				       struct kobj_attribute *a,
> >> +				       const char *buf, size_t len)
> >> +{
> >> +	int i;
> >> +	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
> >> +
> >> +	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
> >> +		if (btrfs_strmatch(buf, btrfs_read_policy_name[i])) {
> > 
> > Does sysfs guarantee that the buf is nul terminated string or that it
> > contains a null at all? Because if not, the skip_space step of strmatch
> > could run out of the buffer.
> > 
> 
> It does
> 
> [  173.555507]  ? btrfs_read_policy_store+0x3e/0x12d [btrfs]
> [  173.555541]  ? kobj_attr_store+0x16/0x30
> [  173.555562]  ? sysfs_kf_write+0x54/0x80
> [  173.555582]  ? kernfs_fop_write+0xfa/0x290
> [  173.555611]  ? vfs_write+0xee/0x2f0
> [  173.555641]  ? ksys_write+0x80/0x170
> [  173.555671]  ? __x64_sys_write+0x1e/0x30
> [  173.555692]  ? do_syscall_64+0x4b/0x80
> [  173.555708]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> 
> static ssize_t kernfs_fop_write(struct file *file, const char __user 
> *user_buf,
>                                  size_t count, loff_t *ppos)
> {
> ::
>          buf[len] = '\0';        /* guarantee string termination */

Ok thanks.

Patch

diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
index 5955379d3d9e..4dbf90ff088a 100644
--- a/fs/btrfs/sysfs.c
+++ b/fs/btrfs/sysfs.c
@@ -906,6 +906,54 @@  static bool btrfs_strmatch(const char *given, const char *golden)
 	return false;
 }
 
+static const char * const btrfs_read_policy_name[] = { "pid" };
+
+static ssize_t btrfs_read_policy_show(struct kobject *kobj,
+				      struct kobj_attribute *a, char *buf)
+{
+	int i;
+	ssize_t ret = 0;
+	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
+
+	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
+		if (fs_devices->read_policy == i)
+			ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s[%s]",
+					 (ret == 0 ? "" : " "),
+					 btrfs_read_policy_name[i]);
+		else
+			ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
+					 (ret == 0 ? "" : " "),
+					 btrfs_read_policy_name[i]);
+	}
+
+	ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
+
+	return ret;
+}
+
+static ssize_t btrfs_read_policy_store(struct kobject *kobj,
+				       struct kobj_attribute *a,
+				       const char *buf, size_t len)
+{
+	int i;
+	struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj);
+
+	for (i = 0; i < BTRFS_NR_READ_POLICY; i++) {
+		if (btrfs_strmatch(buf, btrfs_read_policy_name[i])) {
+			if (i != fs_devices->read_policy) {
+				fs_devices->read_policy = i;
+				btrfs_info(fs_devices->fs_info,
+					   "read policy set to '%s'",
+					   btrfs_read_policy_name[i]);
+			}
+			return len;
+		}
+	}
+
+	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),
@@ -916,6 +964,7 @@  static const struct attribute *btrfs_attrs[] = {
 	BTRFS_ATTR_PTR(, checksum),
 	BTRFS_ATTR_PTR(, exclusive_operation),
 	BTRFS_ATTR_PTR(, generation),
+	BTRFS_ATTR_PTR(, read_policy),
 	NULL,
 };