| Message ID | 6a303a3da8116c3743fa9be605dde540d8a60f1a.1735748715.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | raid1 balancing methods | expand |
On Thu, Jan 02, 2025 at 02:06:33AM +0800, Anand Jain wrote: > This change enables specifying additional configuration values alongside > the raid1 balancing / read policy in a single input string. > > Updated btrfs_read_policy_to_enum() to parse and handle a value associated > with the policy in the format `policy:value`, the value part if present is > converted 64-bit integer. Update btrfs_read_policy_store() to accommodate > the new parameter. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/sysfs.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c > index 3b0325259c02..cf6e5322621f 100644 > --- a/fs/btrfs/sysfs.c > +++ b/fs/btrfs/sysfs.c > @@ -1307,15 +1307,26 @@ BTRFS_ATTR(, temp_fsid, btrfs_temp_fsid_show); > > static const char * const btrfs_read_policy_name[] = { "pid" }; > > -static int btrfs_read_policy_to_enum(const char *str) > +static int btrfs_read_policy_to_enum(const char *str, s64 *value) > { > char param[32] = {'\0'}; > + char *__maybe_unused value_str; > > if (!str || strlen(str) == 0) > return 0; > > strncpy(param, str, sizeof(param) - 1); > > +#ifdef CONFIG_BTRFS_EXPERIMENTAL > + /* Separate value from input in policy:value format. */ > + if ((value_str = strchr(param, ':'))) { > + *value_str = '\0'; > + value_str++; > + if (value && kstrtou64(value_str, 10, value) != 0) > + return -EINVAL; I think I've mentioned that before, this lacks validation of the 'value', there should be some lower and upper bound check. Minimum can be the sectorsize and maximum maybe 1G or 2G, so the u32 type is sufficient. The silent conversion from u64 to s64 should be avoided.
On 2/1/25 23:59, David Sterba wrote: > On Thu, Jan 02, 2025 at 02:06:33AM +0800, Anand Jain wrote: >> This change enables specifying additional configuration values alongside >> the raid1 balancing / read policy in a single input string. >> >> Updated btrfs_read_policy_to_enum() to parse and handle a value associated >> with the policy in the format `policy:value`, the value part if present is >> converted 64-bit integer. Update btrfs_read_policy_store() to accommodate >> the new parameter. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/sysfs.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c >> index 3b0325259c02..cf6e5322621f 100644 >> --- a/fs/btrfs/sysfs.c >> +++ b/fs/btrfs/sysfs.c >> @@ -1307,15 +1307,26 @@ BTRFS_ATTR(, temp_fsid, btrfs_temp_fsid_show); >> >> static const char * const btrfs_read_policy_name[] = { "pid" }; >> >> -static int btrfs_read_policy_to_enum(const char *str) >> +static int btrfs_read_policy_to_enum(const char *str, s64 *value) >> { >> char param[32] = {'\0'}; >> + char *__maybe_unused value_str; >> >> if (!str || strlen(str) == 0) >> return 0; >> >> strncpy(param, str, sizeof(param) - 1); >> >> +#ifdef CONFIG_BTRFS_EXPERIMENTAL >> + /* Separate value from input in policy:value format. */ >> + if ((value_str = strchr(param, ':'))) { >> + *value_str = '\0'; >> + value_str++; >> + if (value && kstrtou64(value_str, 10, value) != 0) >> + return -EINVAL; > > I think I've mentioned that before, this lacks validation of the > 'value', there should be some lower and upper bound check. Minimum can > be the sectorsize and maximum maybe 1G or 2G, so the u32 type is > sufficient. Regarding the lack of validation, as I replied earlier, only the minimum needs validation, not the upper limit. A 1TB disk or SAN storage can be connected to a 32-bit host, and we can still read the full 1TB from a single mirror. IMO, it depends on the use case, or we can revisit it based on feedback.? The patch already handles the lower limit. $ echo round-robin:1024 > /sys/fs/btrfs/b03580af-900e-4940-a7f8-3f04f9981a12/read_policy $ cat /sys/fs/btrfs/b03580af-900e-4940-a7f8-3f04f9981a12/read_policy pid [round-robin:4096] devid:1 $ dmesg -k | tail -1 BTRFS warning (device sda): read_policy: min contiguous read 1024 should be multiples of the sectorsize 4096, rounded to 4096 > The silent conversion from u64 to s64 should be avoided. Do you mean from s64 to u64? Thanks, Anand
On Fri, Jan 03, 2025 at 08:34:12PM +0530, Anand Jain wrote: > >> +#ifdef CONFIG_BTRFS_EXPERIMENTAL > >> + /* Separate value from input in policy:value format. */ > >> + if ((value_str = strchr(param, ':'))) { > >> + *value_str = '\0'; > >> + value_str++; > >> + if (value && kstrtou64(value_str, 10, value) != 0) > >> + return -EINVAL; > > > > I think I've mentioned that before, this lacks validation of the > > 'value', there should be some lower and upper bound check. Minimum can > > be the sectorsize and maximum maybe 1G or 2G, so the u32 type is > > sufficient. > > Regarding the lack of validation, as I replied earlier, only the > minimum needs validation, not the upper limit. A 1TB disk or SAN > storage can be connected to a 32-bit host, and we can still read > the full 1TB from a single mirror. IMO, it depends on the use case, > or we can revisit it based on feedback.? SAN is not a typical storage, what the load balancing should assume is a set of individual rotational or ssd/nvme disks. Also 32bit host is not something we're expecting by default. > The patch already handles the lower limit. > > $ echo round-robin:1024 > > /sys/fs/btrfs/b03580af-900e-4940-a7f8-3f04f9981a12/read_policy > > $ cat /sys/fs/btrfs/b03580af-900e-4940-a7f8-3f04f9981a12/read_policy > pid [round-robin:4096] devid:1 > > $ dmesg -k | tail -1 > BTRFS warning (device sda): read_policy: min contiguous read 1024 should > be multiples of the sectorsize 4096, rounded to 4096 Right. > > The silent conversion from u64 to s64 should be avoided. > > Do you mean from s64 to u64? Yes, I've used kstrtos64 and reject negative values, then the types match. I'm not sure about the upper bound, the idea was like 1G or 2G but a fast NVME can do a few gigabytes per second so we may need more than a 32bit type can hold. But it's counted in blocks it's 4G * 4K so it's safe.
diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 3b0325259c02..cf6e5322621f 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -1307,15 +1307,26 @@ BTRFS_ATTR(, temp_fsid, btrfs_temp_fsid_show); static const char * const btrfs_read_policy_name[] = { "pid" }; -static int btrfs_read_policy_to_enum(const char *str) +static int btrfs_read_policy_to_enum(const char *str, s64 *value) { char param[32] = {'\0'}; + char *__maybe_unused value_str; if (!str || strlen(str) == 0) return 0; strncpy(param, str, sizeof(param) - 1); +#ifdef CONFIG_BTRFS_EXPERIMENTAL + /* Separate value from input in policy:value format. */ + if ((value_str = strchr(param, ':'))) { + *value_str = '\0'; + value_str++; + if (value && kstrtou64(value_str, 10, value) != 0) + return -EINVAL; + } +#endif + return sysfs_match_string(btrfs_read_policy_name, param); } @@ -1351,8 +1362,9 @@ static ssize_t btrfs_read_policy_store(struct kobject *kobj, { struct btrfs_fs_devices *fs_devices = to_fs_devs(kobj); int index; + s64 value = -1; - index = btrfs_read_policy_to_enum(buf); + index = btrfs_read_policy_to_enum(buf, &value); if (index < 0) return -EINVAL;
This change enables specifying additional configuration values alongside the raid1 balancing / read policy in a single input string. Updated btrfs_read_policy_to_enum() to parse and handle a value associated with the policy in the format `policy:value`, the value part if present is converted 64-bit integer. Update btrfs_read_policy_store() to accommodate the new parameter. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/sysfs.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)