[v8,13/20] xfs: Add helpers xfs_attr_is_shortform and xfs_attr_set_shortform
diff mbox series

Message ID 20200403221229.4995-14-allison.henderson@oracle.com
State Superseded
Headers show
Series
  • xfs: Delay Ready Attributes
Related show

Commit Message

Allison Collins April 3, 2020, 10:12 p.m. UTC
In this patch, we hoist code from xfs_attr_set_args into two new helpers
xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
to simplify xfs_attr_set_args when we get into delayed attrs later.

Signed-off-by: Allison Collins <allison.henderson@oracle.com>
---
 fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 35 deletions(-)

Comments

Brian Foster April 7, 2020, 3:23 p.m. UTC | #1
On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote:
> In this patch, we hoist code from xfs_attr_set_args into two new helpers
> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
> to simplify xfs_attr_set_args when we get into delayed attrs later.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 72 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4225a94..ba26ffe 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
>  }
>  
>  /*
> + * Check to see if the attr should be upgraded from non-existent or shortform to
> + * single-leaf-block attribute list.
> + */
> +static inline bool
> +xfs_attr_is_shortform(
> +	struct xfs_inode    *ip)
> +{
> +	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> +	      (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> +	      ip->i_d.di_anextents == 0);

Logic should be indented similar to the original:

	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
	       (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
		ip->i_d.di_anextents == 0);

Otherwise looks good:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> +}
> +
> +/*
> + * Attempts to set an attr in shortform, or converts the tree to leaf form if
> + * there is not enough room.  If the attr is set, the transaction is committed
> + * and set to NULL.
> + */
> +STATIC int
> +xfs_attr_set_shortform(
> +	struct xfs_da_args	*args,
> +	struct xfs_buf		**leaf_bp)
> +{
> +	struct xfs_inode	*dp = args->dp;
> +	int			error, error2 = 0;
> +
> +	/*
> +	 * Try to add the attr to the attribute list in the inode.
> +	 */
> +	error = xfs_attr_try_sf_addname(dp, args);
> +	if (error != -ENOSPC) {
> +		error2 = xfs_trans_commit(args->trans);
> +		args->trans = NULL;
> +		return error ? error : error2;
> +	}
> +	/*
> +	 * It won't fit in the shortform, transform to a leaf block.  GROT:
> +	 * another possible req'mt for a double-split btree op.
> +	 */
> +	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
> +	 * push cannot grab the half-baked leaf buffer and run into problems
> +	 * with the write verifier. Once we're done rolling the transaction we
> +	 * can release the hold and add the attr to the leaf.
> +	 */
> +	xfs_trans_bhold(args->trans, *leaf_bp);
> +	error = xfs_defer_finish(&args->trans);
> +	xfs_trans_bhold_release(args->trans, *leaf_bp);
> +	if (error) {
> +		xfs_trans_brelse(args->trans, *leaf_bp);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
>   * Set the attribute specified in @args.
>   */
>  int
> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_buf          *leaf_bp = NULL;
> -	int			error, error2 = 0;
> +	int			error = 0;
>  
>  	/*
> -	 * If the attribute list is non-existent or a shortform list,
> -	 * upgrade it to a single-leaf-block attribute list.
> +	 * If the attribute list is already in leaf format, jump straight to
> +	 * leaf handling.  Otherwise, try to add the attribute to the shortform
> +	 * list; if there's no room then convert the list to leaf format and try
> +	 * again.
>  	 */
> -	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> -	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> -	     dp->i_d.di_anextents == 0)) {
> -
> -		/*
> -		 * Try to add the attr to the attribute list in the inode.
> -		 */
> -		error = xfs_attr_try_sf_addname(dp, args);
> -		if (error != -ENOSPC) {
> -			error2 = xfs_trans_commit(args->trans);
> -			args->trans = NULL;
> -			return error ? error : error2;
> -		}
> -
> -		/*
> -		 * It won't fit in the shortform, transform to a leaf block.
> -		 * GROT: another possible req'mt for a double-split btree op.
> -		 */
> -		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
> -		if (error)
> -			return error;
> +	if (xfs_attr_is_shortform(dp)) {
>  
>  		/*
> -		 * Prevent the leaf buffer from being unlocked so that a
> -		 * concurrent AIL push cannot grab the half-baked leaf
> -		 * buffer and run into problems with the write verifier.
> -		 * Once we're done rolling the transaction we can release
> -		 * the hold and add the attr to the leaf.
> +		 * If the attr was successfully set in shortform, the
> +		 * transaction is committed and set to NULL.  Otherwise, is it
> +		 * converted from shortform to leaf, and the transaction is
> +		 * retained.
>  		 */
> -		xfs_trans_bhold(args->trans, leaf_bp);
> -		error = xfs_defer_finish(&args->trans);
> -		xfs_trans_bhold_release(args->trans, leaf_bp);
> -		if (error) {
> -			xfs_trans_brelse(args->trans, leaf_bp);
> +		error = xfs_attr_set_shortform(args, &leaf_bp);
> +		if (error || !args->trans)
>  			return error;
> -		}
>  	}
>  
>  	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> -- 
> 2.7.4
>
Allison Collins April 7, 2020, 9:53 p.m. UTC | #2
On 4/7/20 8:23 AM, Brian Foster wrote:
> On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote:
>> In this patch, we hoist code from xfs_attr_set_args into two new helpers
>> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
>> to simplify xfs_attr_set_args when we get into delayed attrs later.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 72 insertions(+), 35 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4225a94..ba26ffe 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
>>   }
>>   
>>   /*
>> + * Check to see if the attr should be upgraded from non-existent or shortform to
>> + * single-leaf-block attribute list.
>> + */
>> +static inline bool
>> +xfs_attr_is_shortform(
>> +	struct xfs_inode    *ip)
>> +{
>> +	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> +	      (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> +	      ip->i_d.di_anextents == 0);
> 
> Logic should be indented similar to the original:
> 
> 	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> 	       (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> 		ip->i_d.di_anextents == 0);
> 
> Otherwise looks good:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
Alrighty, will fix.  Thanks!

Allison

> 
>> +}
>> +
>> +/*
>> + * Attempts to set an attr in shortform, or converts the tree to leaf form if
>> + * there is not enough room.  If the attr is set, the transaction is committed
>> + * and set to NULL.
>> + */
>> +STATIC int
>> +xfs_attr_set_shortform(
>> +	struct xfs_da_args	*args,
>> +	struct xfs_buf		**leaf_bp)
>> +{
>> +	struct xfs_inode	*dp = args->dp;
>> +	int			error, error2 = 0;
>> +
>> +	/*
>> +	 * Try to add the attr to the attribute list in the inode.
>> +	 */
>> +	error = xfs_attr_try_sf_addname(dp, args);
>> +	if (error != -ENOSPC) {
>> +		error2 = xfs_trans_commit(args->trans);
>> +		args->trans = NULL;
>> +		return error ? error : error2;
>> +	}
>> +	/*
>> +	 * It won't fit in the shortform, transform to a leaf block.  GROT:
>> +	 * another possible req'mt for a double-split btree op.
>> +	 */
>> +	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>> +	if (error)
>> +		return error;
>> +
>> +	/*
>> +	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
>> +	 * push cannot grab the half-baked leaf buffer and run into problems
>> +	 * with the write verifier. Once we're done rolling the transaction we
>> +	 * can release the hold and add the attr to the leaf.
>> +	 */
>> +	xfs_trans_bhold(args->trans, *leaf_bp);
>> +	error = xfs_defer_finish(&args->trans);
>> +	xfs_trans_bhold_release(args->trans, *leaf_bp);
>> +	if (error) {
>> +		xfs_trans_brelse(args->trans, *leaf_bp);
>> +		return error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>>    * Set the attribute specified in @args.
>>    */
>>   int
>> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>>   {
>>   	struct xfs_inode	*dp = args->dp;
>>   	struct xfs_buf          *leaf_bp = NULL;
>> -	int			error, error2 = 0;
>> +	int			error = 0;
>>   
>>   	/*
>> -	 * If the attribute list is non-existent or a shortform list,
>> -	 * upgrade it to a single-leaf-block attribute list.
>> +	 * If the attribute list is already in leaf format, jump straight to
>> +	 * leaf handling.  Otherwise, try to add the attribute to the shortform
>> +	 * list; if there's no room then convert the list to leaf format and try
>> +	 * again.
>>   	 */
>> -	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> -	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> -	     dp->i_d.di_anextents == 0)) {
>> -
>> -		/*
>> -		 * Try to add the attr to the attribute list in the inode.
>> -		 */
>> -		error = xfs_attr_try_sf_addname(dp, args);
>> -		if (error != -ENOSPC) {
>> -			error2 = xfs_trans_commit(args->trans);
>> -			args->trans = NULL;
>> -			return error ? error : error2;
>> -		}
>> -
>> -		/*
>> -		 * It won't fit in the shortform, transform to a leaf block.
>> -		 * GROT: another possible req'mt for a double-split btree op.
>> -		 */
>> -		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
>> -		if (error)
>> -			return error;
>> +	if (xfs_attr_is_shortform(dp)) {
>>   
>>   		/*
>> -		 * Prevent the leaf buffer from being unlocked so that a
>> -		 * concurrent AIL push cannot grab the half-baked leaf
>> -		 * buffer and run into problems with the write verifier.
>> -		 * Once we're done rolling the transaction we can release
>> -		 * the hold and add the attr to the leaf.
>> +		 * If the attr was successfully set in shortform, the
>> +		 * transaction is committed and set to NULL.  Otherwise, is it
>> +		 * converted from shortform to leaf, and the transaction is
>> +		 * retained.
>>   		 */
>> -		xfs_trans_bhold(args->trans, leaf_bp);
>> -		error = xfs_defer_finish(&args->trans);
>> -		xfs_trans_bhold_release(args->trans, leaf_bp);
>> -		if (error) {
>> -			xfs_trans_brelse(args->trans, leaf_bp);
>> +		error = xfs_attr_set_shortform(args, &leaf_bp);
>> +		if (error || !args->trans)
>>   			return error;
>> -		}
>>   	}
>>   
>>   	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>> -- 
>> 2.7.4
>>
>
Allison Collins April 10, 2020, 4:55 p.m. UTC | #3
On 4/7/20 2:53 PM, Allison Collins wrote:
> 
> 
> On 4/7/20 8:23 AM, Brian Foster wrote:
>> On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote:
>>> In this patch, we hoist code from xfs_attr_set_args into two new helpers
>>> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
>>> to simplify xfs_attr_set_args when we get into delayed attrs later.
>>>
>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>> ---
>>>   fs/xfs/libxfs/xfs_attr.c | 107 
>>> +++++++++++++++++++++++++++++++----------------
>>>   1 file changed, 72 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>> index 4225a94..ba26ffe 100644
>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>> @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
>>>   }
>>>   /*
>>> + * Check to see if the attr should be upgraded from non-existent or 
>>> shortform to
>>> + * single-leaf-block attribute list.
>>> + */
>>> +static inline bool
>>> +xfs_attr_is_shortform(
>>> +    struct xfs_inode    *ip)
>>> +{
>>> +    return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>>> +          (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>>> +          ip->i_d.di_anextents == 0);
>>
>> Logic should be indented similar to the original:
>>
>>     return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>>            (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>>         ip->i_d.di_anextents == 0);
Did you really mean to have the last line offset like this?  On second 
pass, it doesnt look similar to the original, and looks more like it may 
have been a typo in the review.  Just trying to avoid more cycles on 
spacing goofs.  Thx!

Allison

>>
>> Otherwise looks good:
>>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Alrighty, will fix.  Thanks!
> 
> Allison
> 
>>
>>> +}
>>> +
>>> +/*
>>> + * Attempts to set an attr in shortform, or converts the tree to 
>>> leaf form if
>>> + * there is not enough room.  If the attr is set, the transaction is 
>>> committed
>>> + * and set to NULL.
>>> + */
>>> +STATIC int
>>> +xfs_attr_set_shortform(
>>> +    struct xfs_da_args    *args,
>>> +    struct xfs_buf        **leaf_bp)
>>> +{
>>> +    struct xfs_inode    *dp = args->dp;
>>> +    int            error, error2 = 0;
>>> +
>>> +    /*
>>> +     * Try to add the attr to the attribute list in the inode.
>>> +     */
>>> +    error = xfs_attr_try_sf_addname(dp, args);
>>> +    if (error != -ENOSPC) {
>>> +        error2 = xfs_trans_commit(args->trans);
>>> +        args->trans = NULL;
>>> +        return error ? error : error2;
>>> +    }
>>> +    /*
>>> +     * It won't fit in the shortform, transform to a leaf block.  GROT:
>>> +     * another possible req'mt for a double-split btree op.
>>> +     */
>>> +    error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>>> +    if (error)
>>> +        return error;
>>> +
>>> +    /*
>>> +     * Prevent the leaf buffer from being unlocked so that a 
>>> concurrent AIL
>>> +     * push cannot grab the half-baked leaf buffer and run into 
>>> problems
>>> +     * with the write verifier. Once we're done rolling the 
>>> transaction we
>>> +     * can release the hold and add the attr to the leaf.
>>> +     */
>>> +    xfs_trans_bhold(args->trans, *leaf_bp);
>>> +    error = xfs_defer_finish(&args->trans);
>>> +    xfs_trans_bhold_release(args->trans, *leaf_bp);
>>> +    if (error) {
>>> +        xfs_trans_brelse(args->trans, *leaf_bp);
>>> +        return error;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>>    * Set the attribute specified in @args.
>>>    */
>>>   int
>>> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>>>   {
>>>       struct xfs_inode    *dp = args->dp;
>>>       struct xfs_buf          *leaf_bp = NULL;
>>> -    int            error, error2 = 0;
>>> +    int            error = 0;
>>>       /*
>>> -     * If the attribute list is non-existent or a shortform list,
>>> -     * upgrade it to a single-leaf-block attribute list.
>>> +     * If the attribute list is already in leaf format, jump 
>>> straight to
>>> +     * leaf handling.  Otherwise, try to add the attribute to the 
>>> shortform
>>> +     * list; if there's no room then convert the list to leaf format 
>>> and try
>>> +     * again.
>>>        */
>>> -    if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>>> -        (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>>> -         dp->i_d.di_anextents == 0)) {
>>> -
>>> -        /*
>>> -         * Try to add the attr to the attribute list in the inode.
>>> -         */
>>> -        error = xfs_attr_try_sf_addname(dp, args);
>>> -        if (error != -ENOSPC) {
>>> -            error2 = xfs_trans_commit(args->trans);
>>> -            args->trans = NULL;
>>> -            return error ? error : error2;
>>> -        }
>>> -
>>> -        /*
>>> -         * It won't fit in the shortform, transform to a leaf block.
>>> -         * GROT: another possible req'mt for a double-split btree op.
>>> -         */
>>> -        error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
>>> -        if (error)
>>> -            return error;
>>> +    if (xfs_attr_is_shortform(dp)) {
>>>           /*
>>> -         * Prevent the leaf buffer from being unlocked so that a
>>> -         * concurrent AIL push cannot grab the half-baked leaf
>>> -         * buffer and run into problems with the write verifier.
>>> -         * Once we're done rolling the transaction we can release
>>> -         * the hold and add the attr to the leaf.
>>> +         * If the attr was successfully set in shortform, the
>>> +         * transaction is committed and set to NULL.  Otherwise, is it
>>> +         * converted from shortform to leaf, and the transaction is
>>> +         * retained.
>>>            */
>>> -        xfs_trans_bhold(args->trans, leaf_bp);
>>> -        error = xfs_defer_finish(&args->trans);
>>> -        xfs_trans_bhold_release(args->trans, leaf_bp);
>>> -        if (error) {
>>> -            xfs_trans_brelse(args->trans, leaf_bp);
>>> +        error = xfs_attr_set_shortform(args, &leaf_bp);
>>> +        if (error || !args->trans)
>>>               return error;
>>> -        }
>>>       }
>>>       if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>> -- 
>>> 2.7.4
>>>
>>
Brian Foster April 11, 2020, 12:57 p.m. UTC | #4
On Fri, Apr 10, 2020 at 09:55:55AM -0700, Allison Collins wrote:
> 
> 
> On 4/7/20 2:53 PM, Allison Collins wrote:
> > 
> > 
> > On 4/7/20 8:23 AM, Brian Foster wrote:
> > > On Fri, Apr 03, 2020 at 03:12:22PM -0700, Allison Collins wrote:
> > > > In this patch, we hoist code from xfs_attr_set_args into two new helpers
> > > > xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
> > > > to simplify xfs_attr_set_args when we get into delayed attrs later.
> > > > 
> > > > Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> > > > ---
> > > >   fs/xfs/libxfs/xfs_attr.c | 107
> > > > +++++++++++++++++++++++++++++++----------------
> > > >   1 file changed, 72 insertions(+), 35 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > > > index 4225a94..ba26ffe 100644
> > > > --- a/fs/xfs/libxfs/xfs_attr.c
> > > > +++ b/fs/xfs/libxfs/xfs_attr.c
> > > > @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
> > > >   }
> > > >   /*
> > > > + * Check to see if the attr should be upgraded from
> > > > non-existent or shortform to
> > > > + * single-leaf-block attribute list.
> > > > + */
> > > > +static inline bool
> > > > +xfs_attr_is_shortform(
> > > > +    struct xfs_inode    *ip)
> > > > +{
> > > > +    return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > > > +          (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > > > +          ip->i_d.di_anextents == 0);
> > > 
> > > Logic should be indented similar to the original:
> > > 
> > >     return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > >            (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > >         ip->i_d.di_anextents == 0);
> Did you really mean to have the last line offset like this?  On second pass,
> it doesnt look similar to the original, and looks more like it may have been
> a typo in the review.  Just trying to avoid more cycles on spacing goofs.
> Thx!
> 

Nope. What is quoted here is not how my reply appears in my mailer. :/
Anyways, the last line is supposed to be aligned to the first character
inside the opening brace of the previous line.

Brian

> Allison
> 
> > > 
> > > Otherwise looks good:
> > > 
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > Alrighty, will fix.  Thanks!
> > 
> > Allison
> > 
> > > 
> > > > +}
> > > > +
> > > > +/*
> > > > + * Attempts to set an attr in shortform, or converts the tree
> > > > to leaf form if
> > > > + * there is not enough room.  If the attr is set, the
> > > > transaction is committed
> > > > + * and set to NULL.
> > > > + */
> > > > +STATIC int
> > > > +xfs_attr_set_shortform(
> > > > +    struct xfs_da_args    *args,
> > > > +    struct xfs_buf        **leaf_bp)
> > > > +{
> > > > +    struct xfs_inode    *dp = args->dp;
> > > > +    int            error, error2 = 0;
> > > > +
> > > > +    /*
> > > > +     * Try to add the attr to the attribute list in the inode.
> > > > +     */
> > > > +    error = xfs_attr_try_sf_addname(dp, args);
> > > > +    if (error != -ENOSPC) {
> > > > +        error2 = xfs_trans_commit(args->trans);
> > > > +        args->trans = NULL;
> > > > +        return error ? error : error2;
> > > > +    }
> > > > +    /*
> > > > +     * It won't fit in the shortform, transform to a leaf block.  GROT:
> > > > +     * another possible req'mt for a double-split btree op.
> > > > +     */
> > > > +    error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> > > > +    if (error)
> > > > +        return error;
> > > > +
> > > > +    /*
> > > > +     * Prevent the leaf buffer from being unlocked so that a
> > > > concurrent AIL
> > > > +     * push cannot grab the half-baked leaf buffer and run into
> > > > problems
> > > > +     * with the write verifier. Once we're done rolling the
> > > > transaction we
> > > > +     * can release the hold and add the attr to the leaf.
> > > > +     */
> > > > +    xfs_trans_bhold(args->trans, *leaf_bp);
> > > > +    error = xfs_defer_finish(&args->trans);
> > > > +    xfs_trans_bhold_release(args->trans, *leaf_bp);
> > > > +    if (error) {
> > > > +        xfs_trans_brelse(args->trans, *leaf_bp);
> > > > +        return error;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +/*
> > > >    * Set the attribute specified in @args.
> > > >    */
> > > >   int
> > > > @@ -212,48 +272,25 @@ xfs_attr_set_args(
> > > >   {
> > > >       struct xfs_inode    *dp = args->dp;
> > > >       struct xfs_buf          *leaf_bp = NULL;
> > > > -    int            error, error2 = 0;
> > > > +    int            error = 0;
> > > >       /*
> > > > -     * If the attribute list is non-existent or a shortform list,
> > > > -     * upgrade it to a single-leaf-block attribute list.
> > > > +     * If the attribute list is already in leaf format, jump
> > > > straight to
> > > > +     * leaf handling.  Otherwise, try to add the attribute to
> > > > the shortform
> > > > +     * list; if there's no room then convert the list to leaf
> > > > format and try
> > > > +     * again.
> > > >        */
> > > > -    if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> > > > -        (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> > > > -         dp->i_d.di_anextents == 0)) {
> > > > -
> > > > -        /*
> > > > -         * Try to add the attr to the attribute list in the inode.
> > > > -         */
> > > > -        error = xfs_attr_try_sf_addname(dp, args);
> > > > -        if (error != -ENOSPC) {
> > > > -            error2 = xfs_trans_commit(args->trans);
> > > > -            args->trans = NULL;
> > > > -            return error ? error : error2;
> > > > -        }
> > > > -
> > > > -        /*
> > > > -         * It won't fit in the shortform, transform to a leaf block.
> > > > -         * GROT: another possible req'mt for a double-split btree op.
> > > > -         */
> > > > -        error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
> > > > -        if (error)
> > > > -            return error;
> > > > +    if (xfs_attr_is_shortform(dp)) {
> > > >           /*
> > > > -         * Prevent the leaf buffer from being unlocked so that a
> > > > -         * concurrent AIL push cannot grab the half-baked leaf
> > > > -         * buffer and run into problems with the write verifier.
> > > > -         * Once we're done rolling the transaction we can release
> > > > -         * the hold and add the attr to the leaf.
> > > > +         * If the attr was successfully set in shortform, the
> > > > +         * transaction is committed and set to NULL.  Otherwise, is it
> > > > +         * converted from shortform to leaf, and the transaction is
> > > > +         * retained.
> > > >            */
> > > > -        xfs_trans_bhold(args->trans, leaf_bp);
> > > > -        error = xfs_defer_finish(&args->trans);
> > > > -        xfs_trans_bhold_release(args->trans, leaf_bp);
> > > > -        if (error) {
> > > > -            xfs_trans_brelse(args->trans, leaf_bp);
> > > > +        error = xfs_attr_set_shortform(args, &leaf_bp);
> > > > +        if (error || !args->trans)
> > > >               return error;
> > > > -        }
> > > >       }
> > > >       if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
>
Chandan Rajendra April 20, 2020, 5:30 a.m. UTC | #5
On Saturday, April 4, 2020 3:42 AM Allison Collins wrote: 
> In this patch, we hoist code from xfs_attr_set_args into two new helpers
> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
> to simplify xfs_attr_set_args when we get into delayed attrs later.
> 
> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 72 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 4225a94..ba26ffe 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
>  }
>  
>  /*
> + * Check to see if the attr should be upgraded from non-existent or shortform to
> + * single-leaf-block attribute list.
> + */
> +static inline bool
> +xfs_attr_is_shortform(
> +	struct xfs_inode    *ip)
> +{
> +	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> +	      (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> +	      ip->i_d.di_anextents == 0);
> +}
> +
> +/*
> + * Attempts to set an attr in shortform, or converts the tree to leaf form if

I think you meant to say "converts short form to leaf form".

The functional changes look logically correct,

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> + * there is not enough room.  If the attr is set, the transaction is committed
> + * and set to NULL.
> + */
> +STATIC int
> +xfs_attr_set_shortform(
> +	struct xfs_da_args	*args,
> +	struct xfs_buf		**leaf_bp)
> +{
> +	struct xfs_inode	*dp = args->dp;
> +	int			error, error2 = 0;
> +
> +	/*
> +	 * Try to add the attr to the attribute list in the inode.
> +	 */
> +	error = xfs_attr_try_sf_addname(dp, args);
> +	if (error != -ENOSPC) {
> +		error2 = xfs_trans_commit(args->trans);
> +		args->trans = NULL;
> +		return error ? error : error2;
> +	}
> +	/*
> +	 * It won't fit in the shortform, transform to a leaf block.  GROT:
> +	 * another possible req'mt for a double-split btree op.
> +	 */
> +	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
> +	if (error)
> +		return error;
> +
> +	/*
> +	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
> +	 * push cannot grab the half-baked leaf buffer and run into problems
> +	 * with the write verifier. Once we're done rolling the transaction we
> +	 * can release the hold and add the attr to the leaf.
> +	 */
> +	xfs_trans_bhold(args->trans, *leaf_bp);
> +	error = xfs_defer_finish(&args->trans);
> +	xfs_trans_bhold_release(args->trans, *leaf_bp);
> +	if (error) {
> +		xfs_trans_brelse(args->trans, *leaf_bp);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
>   * Set the attribute specified in @args.
>   */
>  int
> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>  {
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_buf          *leaf_bp = NULL;
> -	int			error, error2 = 0;
> +	int			error = 0;
>  
>  	/*
> -	 * If the attribute list is non-existent or a shortform list,
> -	 * upgrade it to a single-leaf-block attribute list.
> +	 * If the attribute list is already in leaf format, jump straight to
> +	 * leaf handling.  Otherwise, try to add the attribute to the shortform
> +	 * list; if there's no room then convert the list to leaf format and try
> +	 * again.
>  	 */
> -	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
> -	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
> -	     dp->i_d.di_anextents == 0)) {
> -
> -		/*
> -		 * Try to add the attr to the attribute list in the inode.
> -		 */
> -		error = xfs_attr_try_sf_addname(dp, args);
> -		if (error != -ENOSPC) {
> -			error2 = xfs_trans_commit(args->trans);
> -			args->trans = NULL;
> -			return error ? error : error2;
> -		}
> -
> -		/*
> -		 * It won't fit in the shortform, transform to a leaf block.
> -		 * GROT: another possible req'mt for a double-split btree op.
> -		 */
> -		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
> -		if (error)
> -			return error;
> +	if (xfs_attr_is_shortform(dp)) {
>  
>  		/*
> -		 * Prevent the leaf buffer from being unlocked so that a
> -		 * concurrent AIL push cannot grab the half-baked leaf
> -		 * buffer and run into problems with the write verifier.
> -		 * Once we're done rolling the transaction we can release
> -		 * the hold and add the attr to the leaf.
> +		 * If the attr was successfully set in shortform, the
> +		 * transaction is committed and set to NULL.  Otherwise, is it
> +		 * converted from shortform to leaf, and the transaction is
> +		 * retained.
>  		 */
> -		xfs_trans_bhold(args->trans, leaf_bp);
> -		error = xfs_defer_finish(&args->trans);
> -		xfs_trans_bhold_release(args->trans, leaf_bp);
> -		if (error) {
> -			xfs_trans_brelse(args->trans, leaf_bp);
> +		error = xfs_attr_set_shortform(args, &leaf_bp);
> +		if (error || !args->trans)
>  			return error;
> -		}
>  	}
>  
>  	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>
Allison Collins April 20, 2020, 10:45 p.m. UTC | #6
On 4/19/20 10:30 PM, Chandan Rajendra wrote:
> On Saturday, April 4, 2020 3:42 AM Allison Collins wrote:
>> In this patch, we hoist code from xfs_attr_set_args into two new helpers
>> xfs_attr_is_shortform and xfs_attr_set_shortform.  These two will help
>> to simplify xfs_attr_set_args when we get into delayed attrs later.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 107 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 72 insertions(+), 35 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 4225a94..ba26ffe 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -204,6 +204,66 @@ xfs_attr_try_sf_addname(
>>   }
>>   
>>   /*
>> + * Check to see if the attr should be upgraded from non-existent or shortform to
>> + * single-leaf-block attribute list.
>> + */
>> +static inline bool
>> +xfs_attr_is_shortform(
>> +	struct xfs_inode    *ip)
>> +{
>> +	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> +	      (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> +	      ip->i_d.di_anextents == 0);
>> +}
>> +
>> +/*
>> + * Attempts to set an attr in shortform, or converts the tree to leaf form if
> 
> I think you meant to say "converts short form to leaf form".
> 
> The functional changes look logically correct,
Ok, will fix the comment.  Thanks for the review!

Allison

> 
> Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>
> 
>> + * there is not enough room.  If the attr is set, the transaction is committed
>> + * and set to NULL.
>> + */
>> +STATIC int
>> +xfs_attr_set_shortform(
>> +	struct xfs_da_args	*args,
>> +	struct xfs_buf		**leaf_bp)
>> +{
>> +	struct xfs_inode	*dp = args->dp;
>> +	int			error, error2 = 0;
>> +
>> +	/*
>> +	 * Try to add the attr to the attribute list in the inode.
>> +	 */
>> +	error = xfs_attr_try_sf_addname(dp, args);
>> +	if (error != -ENOSPC) {
>> +		error2 = xfs_trans_commit(args->trans);
>> +		args->trans = NULL;
>> +		return error ? error : error2;
>> +	}
>> +	/*
>> +	 * It won't fit in the shortform, transform to a leaf block.  GROT:
>> +	 * another possible req'mt for a double-split btree op.
>> +	 */
>> +	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
>> +	if (error)
>> +		return error;
>> +
>> +	/*
>> +	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
>> +	 * push cannot grab the half-baked leaf buffer and run into problems
>> +	 * with the write verifier. Once we're done rolling the transaction we
>> +	 * can release the hold and add the attr to the leaf.
>> +	 */
>> +	xfs_trans_bhold(args->trans, *leaf_bp);
>> +	error = xfs_defer_finish(&args->trans);
>> +	xfs_trans_bhold_release(args->trans, *leaf_bp);
>> +	if (error) {
>> +		xfs_trans_brelse(args->trans, *leaf_bp);
>> +		return error;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>>    * Set the attribute specified in @args.
>>    */
>>   int
>> @@ -212,48 +272,25 @@ xfs_attr_set_args(
>>   {
>>   	struct xfs_inode	*dp = args->dp;
>>   	struct xfs_buf          *leaf_bp = NULL;
>> -	int			error, error2 = 0;
>> +	int			error = 0;
>>   
>>   	/*
>> -	 * If the attribute list is non-existent or a shortform list,
>> -	 * upgrade it to a single-leaf-block attribute list.
>> +	 * If the attribute list is already in leaf format, jump straight to
>> +	 * leaf handling.  Otherwise, try to add the attribute to the shortform
>> +	 * list; if there's no room then convert the list to leaf format and try
>> +	 * again.
>>   	 */
>> -	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
>> -	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
>> -	     dp->i_d.di_anextents == 0)) {
>> -
>> -		/*
>> -		 * Try to add the attr to the attribute list in the inode.
>> -		 */
>> -		error = xfs_attr_try_sf_addname(dp, args);
>> -		if (error != -ENOSPC) {
>> -			error2 = xfs_trans_commit(args->trans);
>> -			args->trans = NULL;
>> -			return error ? error : error2;
>> -		}
>> -
>> -		/*
>> -		 * It won't fit in the shortform, transform to a leaf block.
>> -		 * GROT: another possible req'mt for a double-split btree op.
>> -		 */
>> -		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
>> -		if (error)
>> -			return error;
>> +	if (xfs_attr_is_shortform(dp)) {
>>   
>>   		/*
>> -		 * Prevent the leaf buffer from being unlocked so that a
>> -		 * concurrent AIL push cannot grab the half-baked leaf
>> -		 * buffer and run into problems with the write verifier.
>> -		 * Once we're done rolling the transaction we can release
>> -		 * the hold and add the attr to the leaf.
>> +		 * If the attr was successfully set in shortform, the
>> +		 * transaction is committed and set to NULL.  Otherwise, is it
>> +		 * converted from shortform to leaf, and the transaction is
>> +		 * retained.
>>   		 */
>> -		xfs_trans_bhold(args->trans, leaf_bp);
>> -		error = xfs_defer_finish(&args->trans);
>> -		xfs_trans_bhold_release(args->trans, leaf_bp);
>> -		if (error) {
>> -			xfs_trans_brelse(args->trans, leaf_bp);
>> +		error = xfs_attr_set_shortform(args, &leaf_bp);
>> +		if (error || !args->trans)
>>   			return error;
>> -		}
>>   	}
>>   
>>   	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>
> 
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 4225a94..ba26ffe 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -204,6 +204,66 @@  xfs_attr_try_sf_addname(
 }
 
 /*
+ * Check to see if the attr should be upgraded from non-existent or shortform to
+ * single-leaf-block attribute list.
+ */
+static inline bool
+xfs_attr_is_shortform(
+	struct xfs_inode    *ip)
+{
+	return ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
+	      (ip->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
+	      ip->i_d.di_anextents == 0);
+}
+
+/*
+ * Attempts to set an attr in shortform, or converts the tree to leaf form if
+ * there is not enough room.  If the attr is set, the transaction is committed
+ * and set to NULL.
+ */
+STATIC int
+xfs_attr_set_shortform(
+	struct xfs_da_args	*args,
+	struct xfs_buf		**leaf_bp)
+{
+	struct xfs_inode	*dp = args->dp;
+	int			error, error2 = 0;
+
+	/*
+	 * Try to add the attr to the attribute list in the inode.
+	 */
+	error = xfs_attr_try_sf_addname(dp, args);
+	if (error != -ENOSPC) {
+		error2 = xfs_trans_commit(args->trans);
+		args->trans = NULL;
+		return error ? error : error2;
+	}
+	/*
+	 * It won't fit in the shortform, transform to a leaf block.  GROT:
+	 * another possible req'mt for a double-split btree op.
+	 */
+	error = xfs_attr_shortform_to_leaf(args, leaf_bp);
+	if (error)
+		return error;
+
+	/*
+	 * Prevent the leaf buffer from being unlocked so that a concurrent AIL
+	 * push cannot grab the half-baked leaf buffer and run into problems
+	 * with the write verifier. Once we're done rolling the transaction we
+	 * can release the hold and add the attr to the leaf.
+	 */
+	xfs_trans_bhold(args->trans, *leaf_bp);
+	error = xfs_defer_finish(&args->trans);
+	xfs_trans_bhold_release(args->trans, *leaf_bp);
+	if (error) {
+		xfs_trans_brelse(args->trans, *leaf_bp);
+		return error;
+	}
+
+	return 0;
+}
+
+/*
  * Set the attribute specified in @args.
  */
 int
@@ -212,48 +272,25 @@  xfs_attr_set_args(
 {
 	struct xfs_inode	*dp = args->dp;
 	struct xfs_buf          *leaf_bp = NULL;
-	int			error, error2 = 0;
+	int			error = 0;
 
 	/*
-	 * If the attribute list is non-existent or a shortform list,
-	 * upgrade it to a single-leaf-block attribute list.
+	 * If the attribute list is already in leaf format, jump straight to
+	 * leaf handling.  Otherwise, try to add the attribute to the shortform
+	 * list; if there's no room then convert the list to leaf format and try
+	 * again.
 	 */
-	if (dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL ||
-	    (dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS &&
-	     dp->i_d.di_anextents == 0)) {
-
-		/*
-		 * Try to add the attr to the attribute list in the inode.
-		 */
-		error = xfs_attr_try_sf_addname(dp, args);
-		if (error != -ENOSPC) {
-			error2 = xfs_trans_commit(args->trans);
-			args->trans = NULL;
-			return error ? error : error2;
-		}
-
-		/*
-		 * It won't fit in the shortform, transform to a leaf block.
-		 * GROT: another possible req'mt for a double-split btree op.
-		 */
-		error = xfs_attr_shortform_to_leaf(args, &leaf_bp);
-		if (error)
-			return error;
+	if (xfs_attr_is_shortform(dp)) {
 
 		/*
-		 * Prevent the leaf buffer from being unlocked so that a
-		 * concurrent AIL push cannot grab the half-baked leaf
-		 * buffer and run into problems with the write verifier.
-		 * Once we're done rolling the transaction we can release
-		 * the hold and add the attr to the leaf.
+		 * If the attr was successfully set in shortform, the
+		 * transaction is committed and set to NULL.  Otherwise, is it
+		 * converted from shortform to leaf, and the transaction is
+		 * retained.
 		 */
-		xfs_trans_bhold(args->trans, leaf_bp);
-		error = xfs_defer_finish(&args->trans);
-		xfs_trans_bhold_release(args->trans, leaf_bp);
-		if (error) {
-			xfs_trans_brelse(args->trans, leaf_bp);
+		error = xfs_attr_set_shortform(args, &leaf_bp);
+		if (error || !args->trans)
 			return error;
-		}
 	}
 
 	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {