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

Allison Collins
2020-02-25T23:29:36Z

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 <allison.henderson@oracle.com>
>> Reviewed-by: Brian Foster <bfoster@redhat.com>
>> ---
>> 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.
>