[v7,07/19] xfs: Factor out xfs_attr_leaf_addname helper

Commit Message

Allison Henderson Feb. 23, 2020, 2:05 a.m. UTC

Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
that we can use, and move the commit into the calling function.

Signed-off-by: Allison Collins <[e-mail]>
Reviewed-by: Brian Foster <[e-mail]>
---
 fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 57 insertions(+), 31 deletions(-)

Comments

Amir Goldstein Feb. 23, 2020, 12:42 p.m. UTC | #1

On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
<[e-mail]> wrote:
>
> Factor out new helper function xfs_attr_leaf_try_add.

Right. This should be the subject.
Not factor out xfs_attr_leaf_addname helper.

> Because new delayed attribute
> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> that we can use, and move the commit into the calling function.

And that is a different change.
It is hard enough to review a pure factor out of a helper.
Adding changed inside a pure re-factor is not good.

Belongs to another change - move transaction commit to caller.

Thanks,
Amir.

Allison Henderson Feb. 23, 2020, 6:38 p.m. UTC | #2

On 2/23/20 5:42 AM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> <[e-mail]> wrote:
>>
>> Factor out new helper function xfs_attr_leaf_try_add.
> 
> Right. This should be the subject.
> Not factor out xfs_attr_leaf_addname helper.
> 
>> Because new delayed attribute
>> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
>> that we can use, and move the commit into the calling function.
> 
> And that is a different change.
> It is hard enough to review a pure factor out of a helper.
> Adding changed inside a pure re-factor is not good.
> 
> Belongs to another change - move transaction commit to caller.

Yes, this came up in the last review, but the reason I avoid it is 
because I think the transaction looks weird in either helper.  The 
reason the roll is here is because there is no room to add the attr as a 
leaf, and so we need to hand it off to the node subroutines.  But that 
seems like something that should be managed by the caller, not leaf 
helpers.  So I was concerned that separating the split and the hoist 
would generate more weird looks from people trying to understand the 
split until the see the hoist in the next patch.  If people really think 
it looks better that way, I can split them up.  But I know folks have a 
tough enough time trying to recall the discussion history, so I'm trying 
to avoid confusion of another type :-)

Thoughts?

Allison

> 
> Thanks,
> Amir.
>

Amir Goldstein Feb. 24, 2020, 6:38 a.m. UTC | #3

On Sun, Feb 23, 2020 at 8:38 PM Allison Collins
<[e-mail]> wrote:
>
> On 2/23/20 5:42 AM, Amir Goldstein wrote:
> > On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> > <[e-mail]> wrote:
> >>
> >> Factor out new helper function xfs_attr_leaf_try_add.
> >
> > Right. This should be the subject.
> > Not factor out xfs_attr_leaf_addname helper.
> >
> >> Because new delayed attribute
> >> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> >> that we can use, and move the commit into the calling function.
> >
> > And that is a different change.
> > It is hard enough to review a pure factor out of a helper.
> > Adding changed inside a pure re-factor is not good.
> >
> > Belongs to another change - move transaction commit to caller.
>
> Yes, this came up in the last review, but the reason I avoid it is
> because I think the transaction looks weird in either helper.  The
> reason the roll is here is because there is no room to add the attr as a
> leaf, and so we need to hand it off to the node subroutines.  But that
> seems like something that should be managed by the caller, not leaf
> helpers.  So I was concerned that separating the split and the hoist
> would generate more weird looks from people trying to understand the
> split until the see the hoist in the next patch.  If people really think
> it looks better that way, I can split them up.  But I know folks have a
> tough enough time trying to recall the discussion history, so I'm trying
> to avoid confusion of another type :-)
>
> Thoughts?
>

It's fine, so long as you document it properly in commit message.
See, we are so bad in this review process, that we rely on humans
to verify that logic preserving re-factoring patches are indeed logic
preserving. This is so lame to begin with, because there are static
checker bots out there that would gladly do that work for us :) ...
if we only annotated the logic preserving patches as such.
In the mean while, while we are substituting the review robots,
the least a developer could do is not call out a patch "re-factoring"
and sneak in a logic change without due notice to reviewers.

Thanks,
Amir.

Allison Henderson Feb. 24, 2020, 7:09 a.m. UTC | #4

On 2/23/20 11:38 PM, Amir Goldstein wrote:
> On Sun, Feb 23, 2020 at 8:38 PM Allison Collins
> <[e-mail]> wrote:
>>
>> On 2/23/20 5:42 AM, Amir Goldstein wrote:
>>> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
>>> <[e-mail]> wrote:
>>>>
>>>> Factor out new helper function xfs_attr_leaf_try_add.
>>>
>>> Right. This should be the subject.
>>> Not factor out xfs_attr_leaf_addname helper.
>>>
>>>> Because new delayed attribute
>>>> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
>>>> that we can use, and move the commit into the calling function.
>>>
>>> And that is a different change.
>>> It is hard enough to review a pure factor out of a helper.
>>> Adding changed inside a pure re-factor is not good.
>>>
>>> Belongs to another change - move transaction commit to caller.
>>
>> Yes, this came up in the last review, but the reason I avoid it is
>> because I think the transaction looks weird in either helper.  The
>> reason the roll is here is because there is no room to add the attr as a
>> leaf, and so we need to hand it off to the node subroutines.  But that
>> seems like something that should be managed by the caller, not leaf
>> helpers.  So I was concerned that separating the split and the hoist
>> would generate more weird looks from people trying to understand the
>> split until the see the hoist in the next patch.  If people really think
>> it looks better that way, I can split them up.  But I know folks have a
>> tough enough time trying to recall the discussion history, so I'm trying
>> to avoid confusion of another type :-)
>>
>> Thoughts?
>>
> 
> It's fine, so long as you document it properly in commit message.
> See, we are so bad in this review process, that we rely on humans
> to verify that logic preserving re-factoring patches are indeed logic
> preserving. This is so lame to begin with, because there are static
> checker bots out there that would gladly do that work for us :) ...
I didnt know there were tools out there that did that.  They sound 
helpful though!

> if we only annotated the logic preserving patches as such.
> In the mean while, while we are substituting the review robots,
> the least a developer could do is not call out a patch "re-factoring"
> and sneak in a logic change without due notice to reviewers.
Ok, what if the title were to say something like:
xfs: Split out node handling from xfs_attr_leaf_addname

Or maybe just "Split apart xfs_attr_leaf_addname"?  Does that sound like 
it would be a better description here?

Thanks!
Allison

> 
> Thanks,
> Amir.
>

Amir Goldstein Feb. 24, 2020, 7:27 a.m. UTC | #5

On Mon, Feb 24, 2020 at 9:09 AM Allison Collins
<[e-mail]> wrote:
>
>
>
> On 2/23/20 11:38 PM, Amir Goldstein wrote:
> > On Sun, Feb 23, 2020 at 8:38 PM Allison Collins
> > <[e-mail]> wrote:
> >>
> >> On 2/23/20 5:42 AM, Amir Goldstein wrote:
> >>> On Sun, Feb 23, 2020 at 4:07 AM Allison Collins
> >>> <[e-mail]> wrote:
> >>>>
> >>>> Factor out new helper function xfs_attr_leaf_try_add.
> >>>
> >>> Right. This should be the subject.
> >>> Not factor out xfs_attr_leaf_addname helper.
> >>>
> >>>> Because new delayed attribute
> >>>> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> >>>> that we can use, and move the commit into the calling function.
> >>>
> >>> And that is a different change.
> >>> It is hard enough to review a pure factor out of a helper.
> >>> Adding changed inside a pure re-factor is not good.
> >>>
> >>> Belongs to another change - move transaction commit to caller.
> >>
> >> Yes, this came up in the last review, but the reason I avoid it is
> >> because I think the transaction looks weird in either helper.  The
> >> reason the roll is here is because there is no room to add the attr as a
> >> leaf, and so we need to hand it off to the node subroutines.  But that
> >> seems like something that should be managed by the caller, not leaf
> >> helpers.  So I was concerned that separating the split and the hoist
> >> would generate more weird looks from people trying to understand the
> >> split until the see the hoist in the next patch.  If people really think
> >> it looks better that way, I can split them up.  But I know folks have a
> >> tough enough time trying to recall the discussion history, so I'm trying
> >> to avoid confusion of another type :-)
> >>
> >> Thoughts?
> >>
> >
> > It's fine, so long as you document it properly in commit message.
> > See, we are so bad in this review process, that we rely on humans
> > to verify that logic preserving re-factoring patches are indeed logic
> > preserving. This is so lame to begin with, because there are static
> > checker bots out there that would gladly do that work for us :) ...
> I didnt know there were tools out there that did that.  They sound
> helpful though!
>
> > if we only annotated the logic preserving patches as such.
> > In the mean while, while we are substituting the review robots,
> > the least a developer could do is not call out a patch "re-factoring"
> > and sneak in a logic change without due notice to reviewers.
> Ok, what if the title were to say something like:
> xfs: Split out node handling from xfs_attr_leaf_addname
>
> Or maybe just "Split apart xfs_attr_leaf_addname"?  Does that sound like
> it would be a better description here?
>

Sure, both are fine.
All I'm saying is that "factor out" is a well established keyword
that shouldn't be abused.

Thanks,
Amir.

Dave Chinner Feb. 25, 2020, 6:42 a.m. UTC | #6

On Sat, Feb 22, 2020 at 07:05:59PM -0700, Allison Collins wrote:
> Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> that we can use, and move the commit into the calling function.

68-72 columns :P

> 
> Signed-off-by: Allison Collins <[e-mail]>
> Reviewed-by: Brian Foster <[e-mail]>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index cf0cba7..b2f0780 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -305,10 +305,30 @@ xfs_attr_set_args(
>  		}
>  	}
>  
> -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>  		error = xfs_attr_leaf_addname(args);
> -	else
> -		error = xfs_attr_node_addname(args);
> +		if (error != -ENOSPC)
> +			return error;
> +
> +		/*
> +		 * Commit that transaction so that the node_addname()
> +		 * call can manage its own transactions.
> +		 */
> +		error = xfs_defer_finish(&args->trans);
> +		if (error)
> +			return error;
> +
> +		/*
> +		 * Commit the current trans (including the inode) and
> +		 * start a new one.
> +		 */
> +		error = xfs_trans_roll_inode(&args->trans, dp);
> +		if (error)
> +			return error;
> +
> +	}
> +
> +	error = xfs_attr_node_addname(args);
>  	return error;

	return xfs_attr_node_addname(args);

better yet:

	if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
		return xfs_attr_node_addname(args);

	error = xfs_attr_leaf_addname(args);
	if (error != -ENOSPC)
		return error;
	.....

BTW, I think I see the pattern now - isolate all the metadata
changes from the mechanism of rolling the transactions, which means
it can be converted to a set of operations connected by a generic
transaction rolling mechanism. It's all starting to make more sense
now :P

> @@ -679,31 +700,36 @@ xfs_attr_leaf_addname(
>  	retval = xfs_attr3_leaf_add(bp, args);
>  	if (retval == -ENOSPC) {
>  		/*
> -		 * Promote the attribute list to the Btree format, then
> -		 * Commit that transaction so that the node_addname() call
> -		 * can manage its own transactions.
> +		 * Promote the attribute list to the Btree format.
> +		 * Unless an error occurs, retain the -ENOSPC retval
>  		 */

Comments should use all 80 columns...

Cheers,

Dave.

Brian Foster Feb. 25, 2020, 1:26 p.m. UTC | #7

On Tue, Feb 25, 2020 at 05:42:07PM +1100, Dave Chinner wrote:
> On Sat, Feb 22, 2020 at 07:05:59PM -0700, Allison Collins wrote:
> > Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
> > routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> > that we can use, and move the commit into the calling function.
> 
> 68-72 columns :P
> 
> > 
> > Signed-off-by: Allison Collins <[e-mail]>
> > Reviewed-by: Brian Foster <[e-mail]>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 57 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index cf0cba7..b2f0780 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -305,10 +305,30 @@ xfs_attr_set_args(
> >  		}
> >  	}
> >  
> > -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> > +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
> >  		error = xfs_attr_leaf_addname(args);
> > -	else
> > -		error = xfs_attr_node_addname(args);
> > +		if (error != -ENOSPC)
> > +			return error;
> > +
> > +		/*
> > +		 * Commit that transaction so that the node_addname()
> > +		 * call can manage its own transactions.
> > +		 */
> > +		error = xfs_defer_finish(&args->trans);
> > +		if (error)
> > +			return error;
> > +
> > +		/*
> > +		 * Commit the current trans (including the inode) and
> > +		 * start a new one.
> > +		 */
> > +		error = xfs_trans_roll_inode(&args->trans, dp);
> > +		if (error)
> > +			return error;
> > +
> > +	}
> > +
> > +	error = xfs_attr_node_addname(args);
> >  	return error;
> 
> 	return xfs_attr_node_addname(args);
> 
> better yet:
> 
> 	if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> 		return xfs_attr_node_addname(args);
> 
> 	error = xfs_attr_leaf_addname(args);
> 	if (error != -ENOSPC)
> 		return error;
> 	.....
> 
> BTW, I think I see the pattern now - isolate all the metadata
> changes from the mechanism of rolling the transactions, which means
> it can be converted to a set of operations connected by a generic
> transaction rolling mechanism. It's all starting to make more sense
> now :P
> 

Yeah.. IIRC the initial attempt at this was going down the path of
creating an entire separate xattr codepath for xattr intents. The
existing codepath has the issue of rolling transactions all over the
place, which makes it difficult to execute from dfops context. The goal
is then to try and take this thing apart such that it works in dfops
context for intents and at the same time can be driven by a high level
transaction rolling loop to support the traditional xattr codepath for
backwards compatibility.

The whole state/context thing fell out of that. I think most agree that
it's ugly, but it's a useful intermediate step for breaking down this
hunk of code into components that can be further evaluated for
simplification and reduction (while making functional progress as well).
E.g., perhaps the top-level states can be eliminated in favor of simply
looking at current xattr fork format. This gets hairier deeper down in
the set path, but I think we may eventually see some similar patterns
emerge when you consider that leaf and node adds follow a very similar
flow and make many of the same function calls with regard to handling
remote values, renames, etc. etc. All in all, I think it will be
interesting if we could come up with some abstractions that ultimately
facilitate removal of the state stuff. That's certainly not clear
enough to me at least right now, but something to keep in mind when
working through this code..

Brian

> > @@ -679,31 +700,36 @@ xfs_attr_leaf_addname(
> >  	retval = xfs_attr3_leaf_add(bp, args);
> >  	if (retval == -ENOSPC) {
> >  		/*
> > -		 * Promote the attribute list to the Btree format, then
> > -		 * Commit that transaction so that the node_addname() call
> > -		 * can manage its own transactions.
> > +		 * Promote the attribute list to the Btree format.
> > +		 * Unless an error occurs, retain the -ENOSPC retval
> >  		 */
> 
> Comments should use all 80 columns...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]
>

Allison Henderson Feb. 25, 2020, 11:26 p.m. UTC | #8

On 2/24/20 11:42 PM, Dave Chinner wrote:
> On Sat, Feb 22, 2020 at 07:05:59PM -0700, Allison Collins wrote:
>> Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
>> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
>> that we can use, and move the commit into the calling function.
> 
> 68-72 columns :P
Yes, I had adjusted the configs on the editor and thought I had it 
fixed.  Will fix
> 
>>
>> Signed-off-by: Allison Collins <[e-mail]>
>> Reviewed-by: Brian Foster <[e-mail]>
>> ---
>>   fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 57 insertions(+), 31 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index cf0cba7..b2f0780 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -305,10 +305,30 @@ xfs_attr_set_args(
>>   		}
>>   	}
>>   
>> -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
>> +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>>   		error = xfs_attr_leaf_addname(args);
>> -	else
>> -		error = xfs_attr_node_addname(args);
>> +		if (error != -ENOSPC)
>> +			return error;
>> +
>> +		/*
>> +		 * Commit that transaction so that the node_addname()
>> +		 * call can manage its own transactions.
>> +		 */
>> +		error = xfs_defer_finish(&args->trans);
>> +		if (error)
>> +			return error;
>> +
>> +		/*
>> +		 * Commit the current trans (including the inode) and
>> +		 * start a new one.
>> +		 */
>> +		error = xfs_trans_roll_inode(&args->trans, dp);
>> +		if (error)
>> +			return error;
>> +
>> +	}
>> +
>> +	error = xfs_attr_node_addname(args);
>>   	return error;
> 
> 	return xfs_attr_node_addname(args);
> 
> better yet:
> 
> 	if (!xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> 		return xfs_attr_node_addname(args);
> 
> 	error = xfs_attr_leaf_addname(args);
> 	if (error != -ENOSPC)
> 		return error;
> 	.....
Sure, let me dig around first though, because that's going to move 
around where the states appear in patch 14.  The set is admittedly a bit 
of a balancing act :-)

> 
> BTW, I think I see the pattern now - isolate all the metadata
> changes from the mechanism of rolling the transactions, which means
> it can be converted to a set of operations connected by a generic
> transaction rolling mechanism. It's all starting to make more sense
> now :P
Alrighty, I'm glad it's coming together. :-)

> 
>> @@ -679,31 +700,36 @@ xfs_attr_leaf_addname(
>>   	retval = xfs_attr3_leaf_add(bp, args);
>>   	if (retval == -ENOSPC) {
>>   		/*
>> -		 * Promote the attribute list to the Btree format, then
>> -		 * Commit that transaction so that the node_addname() call
>> -		 * can manage its own transactions.
>> +		 * Promote the attribute list to the Btree format.
>> +		 * Unless an error occurs, retain the -ENOSPC retval
>>   		 */
> 
> Comments should use all 80 columns...
Will fix

Thanks for the reviews!
Allison
> 
> Cheers,
> 
> Dave.
>

Chandan Rajendra Feb. 28, 2020, 6:51 a.m. UTC | #9

On Sunday, February 23, 2020 7:35 AM Allison Collins wrote: 
> Factor out new helper function xfs_attr_leaf_try_add. Because new delayed attribute
> routines cannot roll transactions, we carve off the parts of xfs_attr_leaf_addname
> that we can use, and move the commit into the calling function.
>

I don't see any logical errors.

Reviewed-by: Chandan Rajendra <[e-mail]>

> Signed-off-by: Allison Collins <[e-mail]>
> Reviewed-by: Brian Foster <[e-mail]>
> ---
>  fs/xfs/libxfs/xfs_attr.c | 88 +++++++++++++++++++++++++++++++-----------------
>  1 file changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index cf0cba7..b2f0780 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -305,10 +305,30 @@ xfs_attr_set_args(
>  		}
>  	}
>  
> -	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
> +	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
>  		error = xfs_attr_leaf_addname(args);
> -	else
> -		error = xfs_attr_node_addname(args);
> +		if (error != -ENOSPC)
> +			return error;
> +
> +		/*
> +		 * Commit that transaction so that the node_addname()
> +		 * call can manage its own transactions.
> +		 */
> +		error = xfs_defer_finish(&args->trans);
> +		if (error)
> +			return error;
> +
> +		/*
> +		 * Commit the current trans (including the inode) and
> +		 * start a new one.
> +		 */
> +		error = xfs_trans_roll_inode(&args->trans, dp);
> +		if (error)
> +			return error;
> +
> +	}
> +
> +	error = xfs_attr_node_addname(args);
>  	return error;
>  }
>  
> @@ -620,20 +640,21 @@ xfs_attr_shortform_addname(xfs_da_args_t *args)
>   *========================================================================*/
>  
>  /*
> - * Add a name to the leaf attribute list structure
> + * Tries to add an attribute to an inode in leaf form
>   *
> - * This leaf block cannot have a "remote" value, we only call this routine
> - * if bmap_one_block() says there is only one block (ie: no remote blks).
> + * This function is meant to execute as part of a delayed operation and leaves
> + * the transaction handling to the caller.  On success the attribute is added
> + * and the inode and transaction are left dirty.  If there is not enough space,
> + * the attr data is converted to node format and -ENOSPC is returned. Caller is
> + * responsible for handling the dirty inode and transaction or adding the attr
> + * in node format.
>   */
>  STATIC int
> -xfs_attr_leaf_addname(
> -	struct xfs_da_args	*args)
> +xfs_attr_leaf_try_add(
> +	struct xfs_da_args	*args,
> +	struct xfs_buf		*bp)
>  {
> -	struct xfs_buf		*bp;
> -	int			retval, error, forkoff;
> -	struct xfs_inode	*dp = args->dp;
> -
> -	trace_xfs_attr_leaf_addname(args);
> +	int			retval, error;
>  
>  	/*
>  	 * Look up the given attribute in the leaf block.  Figure out if
> @@ -679,31 +700,36 @@ xfs_attr_leaf_addname(
>  	retval = xfs_attr3_leaf_add(bp, args);
>  	if (retval == -ENOSPC) {
>  		/*
> -		 * Promote the attribute list to the Btree format, then
> -		 * Commit that transaction so that the node_addname() call
> -		 * can manage its own transactions.
> +		 * Promote the attribute list to the Btree format.
> +		 * Unless an error occurs, retain the -ENOSPC retval
>  		 */
>  		error = xfs_attr3_leaf_to_node(args);
>  		if (error)
>  			return error;
> -		error = xfs_defer_finish(&args->trans);
> -		if (error)
> -			return error;
> +	}
> +	return retval;
> +}
>  
> -		/*
> -		 * Commit the current trans (including the inode) and start
> -		 * a new one.
> -		 */
> -		error = xfs_trans_roll_inode(&args->trans, dp);
> -		if (error)
> -			return error;
>  
> -		/*
> -		 * Fob the whole rest of the problem off on the Btree code.
> -		 */
> -		error = xfs_attr_node_addname(args);
> +/*
> + * Add a name to the leaf attribute list structure
> + *
> + * This leaf block cannot have a "remote" value, we only call this routine
> + * if bmap_one_block() says there is only one block (ie: no remote blks).
> + */
> +STATIC int
> +xfs_attr_leaf_addname(
> +	struct xfs_da_args	*args)
> +{
> +	int			error, forkoff;
> +	struct xfs_buf		*bp = NULL;
> +	struct xfs_inode	*dp = args->dp;
> +
> +	trace_xfs_attr_leaf_addname(args);
> +
> +	error = xfs_attr_leaf_try_add(args, bp);
> +	if (error)
>  		return error;
> -	}
>  
>  	/*
>  	 * Commit the transaction that added the attr name so that
>

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index cf0cba7..b2f0780 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -305,10 +305,30 @@  xfs_attr_set_args(
 		}
 	}
 
-	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK))
+	if (xfs_bmap_one_block(dp, XFS_ATTR_FORK)) {
 		error = xfs_attr_leaf_addname(args);
-	else
-		error = xfs_attr_node_addname(args);
+		if (error != -ENOSPC)
+			return error;
+
+		/*
+		 * Commit that transaction so that the node_addname()
+		 * call can manage its own transactions.
+		 */
+		error = xfs_defer_finish(&args->trans);
+		if (error)
+			return error;
+
+		/*
+		 * Commit the current trans (including the inode) and
+		 * start a new one.
+		 */
+		error = xfs_trans_roll_inode(&args->trans, dp);
+		if (error)
+			return error;
+
+	}
+
+	error = xfs_attr_node_addname(args);
 	return error;
 }
 
@@ -620,20 +640,21 @@  xfs_attr_shortform_addname(xfs_da_args_t *args)
  *========================================================================*/
 
 /*
- * Add a name to the leaf attribute list structure
+ * Tries to add an attribute to an inode in leaf form
  *
- * This leaf block cannot have a "remote" value, we only call this routine
- * if bmap_one_block() says there is only one block (ie: no remote blks).
+ * This function is meant to execute as part of a delayed operation and leaves
+ * the transaction handling to the caller.  On success the attribute is added
+ * and the inode and transaction are left dirty.  If there is not enough space,
+ * the attr data is converted to node format and -ENOSPC is returned. Caller is
+ * responsible for handling the dirty inode and transaction or adding the attr
+ * in node format.
  */
 STATIC int
-xfs_attr_leaf_addname(
-	struct xfs_da_args	*args)
+xfs_attr_leaf_try_add(
+	struct xfs_da_args	*args,
+	struct xfs_buf		*bp)
 {
-	struct xfs_buf		*bp;
-	int			retval, error, forkoff;
-	struct xfs_inode	*dp = args->dp;
-
-	trace_xfs_attr_leaf_addname(args);
+	int			retval, error;
 
 	/*
 	 * Look up the given attribute in the leaf block.  Figure out if
@@ -679,31 +700,36 @@  xfs_attr_leaf_addname(
 	retval = xfs_attr3_leaf_add(bp, args);
 	if (retval == -ENOSPC) {
 		/*
-		 * Promote the attribute list to the Btree format, then
-		 * Commit that transaction so that the node_addname() call
-		 * can manage its own transactions.
+		 * Promote the attribute list to the Btree format.
+		 * Unless an error occurs, retain the -ENOSPC retval
 		 */
 		error = xfs_attr3_leaf_to_node(args);
 		if (error)
 			return error;
-		error = xfs_defer_finish(&args->trans);
-		if (error)
-			return error;
+	}
+	return retval;
+}
 
-		/*
-		 * Commit the current trans (including the inode) and start
-		 * a new one.
-		 */
-		error = xfs_trans_roll_inode(&args->trans, dp);
-		if (error)
-			return error;
 
-		/*
-		 * Fob the whole rest of the problem off on the Btree code.
-		 */
-		error = xfs_attr_node_addname(args);
+/*
+ * Add a name to the leaf attribute list structure
+ *
+ * This leaf block cannot have a "remote" value, we only call this routine
+ * if bmap_one_block() says there is only one block (ie: no remote blks).
+ */
+STATIC int
+xfs_attr_leaf_addname(
+	struct xfs_da_args	*args)
+{
+	int			error, forkoff;
+	struct xfs_buf		*bp = NULL;
+	struct xfs_inode	*dp = args->dp;
+
+	trace_xfs_attr_leaf_addname(args);
+
+	error = xfs_attr_leaf_try_add(args, bp);
+	if (error)
 		return error;
-	}
 
 	/*
 	 * Commit the transaction that added the attr name so that