Re: [PATCH v4 09/17] xfs: Factor up commit from xfs_attr_try_sf_addname

Allison Collins
2019-11-08T23:13:25Z

On 11/8/19 2:04 PM, Darrick J. Wong wrote:
> On Wed, Nov 06, 2019 at 06:27:53PM -0700, Allison Collins wrote:
>> New delayed attribute routines cannot handle transactions,
>> so factor this up to the calling function.
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 23 ++++++++++++-----------
>> 1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index dda2eba..e0a38a2 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -227,8 +227,7 @@ xfs_attr_try_sf_addname(
>> struct xfs_da_args *args)
>> {
>>
>> - struct xfs_mount *mp = dp->i_mount;
>> - int error, error2;
>> + int error;
>>
>> error = xfs_attr_shortform_addname(args);
>> if (error == -ENOSPC)
>> @@ -241,12 +240,7 @@ xfs_attr_try_sf_addname(
>> if (!error && (args->name.type & ATTR_KERNOTIME) == 0)
>> xfs_trans_ichgtime(args->trans, dp, XFS_ICHGTIME_CHG);
>
> What if you moved this part (the conditional ichgtime) into
> xfs_attr_shortform_addname? Then this function can just go away.
Sure, I may do that in a separate patch though just to make it easier to
review.

>
>>
>> - if (mp->m_flags & XFS_MOUNT_WSYNC)
>> - xfs_trans_set_sync(args->trans);
>> -
>> - error2 = xfs_trans_commit(args->trans);
>> - args->trans = NULL;
>> - return error ? error : error2;
>> + return error;
>> }
>>
>> /*
>> @@ -258,7 +252,7 @@ xfs_attr_set_args(
>> {
>> struct xfs_inode *dp = args->dp;
>> struct xfs_buf *leaf_bp = NULL;
>> - int error;
>> + int error, error2 = 0;;
>>
>> /*
>> * If the attribute list is non-existent or a shortform list,
>> @@ -278,8 +272,15 @@ xfs_attr_set_args(
>> * Try to add the attr to the attribute list in the inode.
>> */
>> error = xfs_attr_try_sf_addname(dp, args);
>> - if (error != -ENOSPC)
>> - return error;
>> + if (error != -ENOSPC) {
>> + if (dp->i_mount->m_flags & XFS_MOUNT_WSYNC)
>> + xfs_trans_set_sync(args->trans);
>> +
>> + error2 = xfs_trans_commit(args->trans);
>> + args->trans = NULL;
>> + return error ? error : error2;
>
> Can error be something other than 0 or EEXIST? If so, does it make
> sense to commit even in those cases? (Have I asked this before...?)
Yeah, this came up once before:
https://patchwork.kernel.org/patch/11087647/

So I simplified it, but then I think people were more comfortable with a
straight refactor with no function change:
https://patchwork.kernel.org/patch/11134023/

So I put it back. :-)

It
> looks odd to me that we'd commit the transaction even if something
> handed back EFSCORRUPTED.
>
> Hm, it's a local attr fork so I guess the only possible error is ENOSPC?
> If that's true then please add a comment/ASSERT to that effect.

Ok, how about a comment. Something like
/* Should only be 0, EEXIST or ENOSPC */

Sound good?
Allison

>
> --D
>
>> + }
>> +
>>
>> /*
>> * It won't fit in the shortform, transform to a leaf block.
>> --
>> 2.7.4
>>