Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop

From: Nikolay Borisov <nborisov@suse.com>
To: Anand Jain <anand.jain@oracle.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 3/4 RESEND] btrfs: open code btrfs_set_prop in inherit_prop
Date: Tue, 2 Apr 2019 16:35:12 +0300	[thread overview]
Message-ID: <8fedbab1-6fec-92cb-25b2-c9b9ff264103@suse.com> (raw)
In-Reply-To: <20190402100742.8355-5-anand.jain@oracle.com>

On 2.04.19 г. 13:07 ч., Anand Jain wrote:
> When an inode inherits property from its parent, we call btrfs_set_prop().
> btrfs_set_prop() does an elaborate checks, which is not required in the
> context of inheriting a property. Instead just open-code only the required
> items from btrfs_set_prop() and then call btrfs_setxattr() directly. So
> now the only user of btrfs_set_prop() is gone, (except for the wraper
> function btrfs_set_prop_trans()).
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/props.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
> index 72a06c4d3c70..fb84e76f3b1d 100644
> --- a/fs/btrfs/props.c
> +++ b/fs/btrfs/props.c
> @@ -367,20 +367,35 @@ static int inherit_props(struct btrfs_trans_handle *trans,
>  		if (!value)
>  			continue;
>  
> +		/* may be removed */

This comment brings no value. Either explain *why* it can be removed or
remove it altogether.

> +		ret = h->validate(value, strlen(value));
> +		if (ret)
> +			continue;
> +
>  		num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1);
>  		ret = btrfs_block_rsv_add(root, trans->block_rsv,
>  					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
>  		if (ret)
> -			goto out;
> -		ret = btrfs_set_prop(trans, inode, h->xattr_name, value,
> +			return ret;
> +
> +		ret = btrfs_setxattr(trans, inode, h->xattr_name, value,
>  				     strlen(value), 0);
> +		if (!ret) {
> +			ret = h->apply(inode, value, strlen(value));
> +			if (ret)
> +				btrfs_setxattr(trans, inode, h->xattr_name,
> +					       NULL, 0, 0);
> +			else
> +				set_bit(BTRFS_INODE_HAS_PROPS,
> +					&BTRFS_I(inode)->runtime_flags);
> +		}
> +
>  		btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes);

(not related to this patch) but should the reservation be released after
setxattr is called, even if it succeeds, since we need to hold it until
the reservation is committed. E.g. if we have to inherit 2 props then in
the first iteration of the loop we reserve space, we call btrfs_Setxattr
with enough reserved space, then in the 2nd reservation we need another
reservation on top of the previous one, no ?

Otherwise the change per se looks good:

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

>  		if (ret)
> -			goto out;
> +			return ret;
>  	}
> -	ret = 0;
> -out:
> -	return ret;
> +
> +	return 0;
>  }
>  
>  int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,
> 