[3/4,RESEND] btrfs: open code btrfs_set_prop in inherit_prop
diff mbox series

Message ID 20190402100742.8355-5-anand.jain@oracle.com
State New
Headers show
Series
  • btrfs: fix property bugs
Related show

Commit Message

Anand Jain April 2, 2019, 10:07 a.m. UTC
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(-)

Comments

Nikolay Borisov April 2, 2019, 1:35 p.m. UTC | #1
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,
>
David Sterba April 2, 2019, 9:41 p.m. UTC | #2
On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
> 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 ?

I think that's the problem Josef's patches were supposed to fix.
David Sterba April 2, 2019, 9:41 p.m. UTC | #3
On Tue, Apr 02, 2019 at 06:07:41PM +0800, 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>

Reviewed-by: David Sterba <dsterba@suse.com>
David Sterba April 2, 2019, 11:24 p.m. UTC | #4
On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
> > @@ -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.

Comment replaced by 

                 /*                                                                                                                                                                                               
                  * This is not strictly necessary as the property should be                                                                                                                                      
                  * valid, but in case it isn't, don't propagate it futher.                                                                                                                                       
                  */

I'd keep it the because that's what the direct opencoding transformation would
do. If we want to remove it, then the reasoning should be provided.
Anand Jain April 3, 2019, 12:42 a.m. UTC | #5
On 3/4/19 7:24 AM, David Sterba wrote:
> On Tue, Apr 02, 2019 at 04:35:12PM +0300, Nikolay Borisov wrote:
>>> @@ -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.
> 
> Comment replaced by
> 
>                   /*
>                    * This is not strictly necessary as the property should be
>                    * valid, but in case it isn't, don't propagate it futher.
>                    */
> 
> I'd keep it the because that's what the direct opencoding transformation would
> do. If we want to remove it, then the reasoning should be provided.

   Agreed with the reasoning.

Thanks, Anand

Patch
diff mbox series

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 */
+		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);
 		if (ret)
-			goto out;
+			return ret;
 	}
-	ret = 0;
-out:
-	return ret;
+
+	return 0;
 }
 
 int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,