Re: [PATCH 6/7] btrfs: Allocate btrfs_ioctl_defrag_range_args on stack

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

On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_defrag_range_args,
> allocate btrfs_ioctl_defrag_range_args on stack.
> 
> sizeof(btrfs_ioctl_defrag_range_args) = 48
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks good. A nit is below.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

> ---
>   fs/btrfs/ioctl.c | 24 ++++++++----------------
>   1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 291c16d8576b..bc38a1af45c7 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3096,7 +3096,7 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>   {
>   	struct inode *inode = file_inode(file);
>   	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	struct btrfs_ioctl_defrag_range_args *range;
> +	struct btrfs_ioctl_defrag_range_args range = {0};
>   	int ret;
>   
>   	ret = mnt_want_write_file(file);
> @@ -3128,33 +3128,25 @@ static int btrfs_ioctl_defrag(struct file *file, void __user *argp)
>   			goto out;
>   		}
>   
> -		range = kzalloc(sizeof(*range), GFP_KERNEL);
> -		if (!range) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
>   		if (argp) {
> -			if (copy_from_user(range, argp,
> -					   sizeof(*range))) {
> +			if (copy_from_user(&range, argp,
> +					   sizeof(range))) {

Nit.
This fits in a line.


>   				ret = -EFAULT;
> -				kfree(range);
>   				goto out;
>   			}
>   			/* compression requires us to start the IO */
> -			if ((range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
> -				range->flags |= BTRFS_DEFRAG_RANGE_START_IO;
> -				range->extent_thresh = (u32)-1;
> +			if ((range.flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
> +				range.flags |= BTRFS_DEFRAG_RANGE_START_IO;
> +				range.extent_thresh = (u32)-1;
>   			}
>   		} else {
>   			/* the rest are all set to zero by kzalloc */
> -			range->len = (u64)-1;
> +			range.len = (u64)-1;
>   		}
>   		ret = btrfs_defrag_file(file_inode(file), file,
> -					range, BTRFS_OLDEST_GENERATION, 0);
> +					&range, BTRFS_OLDEST_GENERATION, 0);
>   		if (ret > 0)
>   			ret = 0;
> -		kfree(range);
>   		break;
>   	default:
>   		ret = -EINVAL;
>