Re: [PATCH 3/5] btrfs: change the device allocation_hint property via sysfs

[Josef Bacik <josef@toxicpanda.com>]

On Sun, Mar 06, 2022 at 07:14:41PM +0100, Goffredo Baroncelli wrote:
> From: Goffredo Baroncelli <kreijack@inwind.it>
> 
> This patch allow to change the allocation_hint property writing
> a numerical value in the file.
> 
> /sysfs/fs/btrfs/<UUID>/devinfo/<devid>/allocation_hint
> 
> To update this field it is added the property "allocation_hint" in
> btrfs-prog too.
> 
> Signed-off-by: Goffredo Baroncelli <kreijack@inwind.it>
> ---
>  fs/btrfs/sysfs.c   | 76 +++++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/volumes.c |  2 +-
>  fs/btrfs/volumes.h |  2 ++
>  3 files changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> index 59d92a385a96..c6723456c0e1 100644
> --- a/fs/btrfs/sysfs.c
> +++ b/fs/btrfs/sysfs.c
> @@ -1606,7 +1606,81 @@ static ssize_t btrfs_devinfo_allocation_hint_show(struct kobject *kobj,
>  	}
>  	return scnprintf(buf, PAGE_SIZE, "<UNKNOWN>\n");
>  }
> -BTRFS_ATTR(devid, allocation_hint, btrfs_devinfo_allocation_hint_show);
> +
> +static ssize_t btrfs_devinfo_allocation_hint_store(struct kobject *kobj,
> +				 struct kobj_attribute *a,
> +				 const char *buf, size_t len)

If you're using vim, use

set cindent
set cino=(0

This will give you the proper formatting we expect, this should be something
like

statice ssize_t btrfs_devinfo_allocation_hint_store(struct kobject *kobj,
						    struct kobj_attribute *a,
						    const char *buf, size_t len)


> +{
> +	struct btrfs_fs_info *fs_info;
> +	struct btrfs_root *root;
> +	struct btrfs_device *device;
> +	int ret;
> +	struct btrfs_trans_handle *trans;
> +	int i, l;
> +	u64 type, prev_type;
> +
> +	if (len < 1)
> +		return -EINVAL;
> +
> +	/* remove trailing newline */
> +	l = len;
> +	if (buf[len-1] == '\n')
> +		l--;
> +

This is unnecessary, because lower you can do...

> +	for (i = 0 ; i < ARRAY_SIZE(allocation_hint_name) ; i++) {
> +		if (l != strlen(allocation_hint_name[i].name))
> +			continue;
> +
> +		if (strncasecmp(allocation_hint_name[i].name, buf, l))
> +			continue;
> +

strmatch(buf, allocation_hint_name[i].name)

I would make a separate patch to change strmatch to do strncasecmp instead, but
then you can just use that helper.

> +		type = allocation_hint_name[i].value;
> +		break;
> +	}
> +
> +	if (i >= ARRAY_SIZE(allocation_hint_name))
> +		return -EINVAL;
> +
> +	device = container_of(kobj, struct btrfs_device, devid_kobj);
> +	fs_info = device->fs_info;
> +	if (!fs_info)
> +		return -EPERM;
> +
> +	root = fs_info->chunk_root;
> +	if (sb_rdonly(fs_info->sb))
> +		return -EROFS;
> +
> +	/* check if a change is really needed */
> +	if ((device->type & BTRFS_DEV_ALLOCATION_HINT_MASK) == type)
> +		return len;
> +
> +	trans = btrfs_start_transaction(root, 1);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	prev_type = device->type;
> +	device->type = (device->type & ~BTRFS_DEV_ALLOCATION_HINT_MASK) | type;
> +
> +	ret = btrfs_update_device(trans, device);
> +
> +	if (ret < 0) {
> +		btrfs_abort_transaction(trans, ret);
> +		btrfs_end_transaction(trans);
> +		goto abort;
> +	}
> +
> +	ret = btrfs_commit_transaction(trans);
> +	if (ret < 0)
> +		goto abort;
> +
> +	return len;
> +abort:
> +	device->type = prev_type;
> +	return  ret;

Extra whitespace here.

> +}
> +BTRFS_ATTR_RW(devid, allocation_hint, btrfs_devinfo_allocation_hint_show,
> +				      btrfs_devinfo_allocation_hint_store);
> +
>  
>  /*
>   * Information about one device.
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 5e3e13d4940b..d4ac90f5c949 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2846,7 +2846,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
>  	return ret;
>  }
>  
> -static noinline int btrfs_update_device(struct btrfs_trans_handle *trans,
> +noinline int btrfs_update_device(struct btrfs_trans_handle *trans,
>  					struct btrfs_device *device)
>  {
>  	int ret;

You can drop the noinline thing here as well, and make sure to fix the
indentation.

> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index bd297f23d19e..93ac27d8097c 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -636,5 +636,7 @@ int btrfs_bg_type_to_factor(u64 flags);
>  const char *btrfs_bg_type_to_raid_name(u64 flags);
>  int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info);
>  bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical);
> +int btrfs_update_device(struct btrfs_trans_handle *trans,
> +			struct btrfs_device *device);

Make the indentation correct.  Thanks,

Josef