Re: [PATCH v4 15/17] xfs: Check for -ENOATTR or -EEXIST

Allison Collins
2019-11-12T00:33:46Z

On 11/11/19 11:24 AM, Brian Foster wrote:
> On Wed, Nov 06, 2019 at 06:27:59PM -0700, Allison Collins wrote:
>> Delayed operations cannot return error codes. So we must check for
>> these conditions first before starting set or remove operations
>>
>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_attr.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>> index 5dcb19f..626d4a98 100644
>> --- a/fs/xfs/libxfs/xfs_attr.c
>> +++ b/fs/xfs/libxfs/xfs_attr.c
>> @@ -458,6 +458,27 @@ xfs_attr_set(
>> goto out_trans_cancel;
>>
>> xfs_trans_ijoin(args.trans, dp, 0);
>> +
>> + error = xfs_has_attr(&args);
>> + if (error == -EEXIST) {
>> + if (name->type & ATTR_CREATE)
>> + goto out_trans_cancel;
>> + else
>> + name->type |= ATTR_REPLACE;
>> + }
>> +
>> + if (error == -ENOATTR && (name->type & ATTR_REPLACE))
>> + goto out_trans_cancel;
>> +
>> + if (name->type & ATTR_REPLACE) {
>> + name->type &= ~ATTR_REPLACE;
>> + error = xfs_attr_remove_args(&args);
>> + if (error)
>> + goto out_trans_cancel;
>> +
>> + name->type |= ATTR_CREATE;
>> + }
>> +
>
> I see Darrick already commented on this.. I think the behavior of the
> existing rename code is to essentially create the new xattr with the
> INCOMPLETE flag set so we can roll transactions, etc. without any
> observable behavior to userspace. Once the new xattr is fully in place,
> the rename is performed atomically from the userspace perspective by
> flipping the INCOMPLETE flag from the newly constructed xattr to the old
> one and we can then remove the old xattr from there.
Yes, I will add this logic in the next revision

>
>> error = xfs_attr_set_args(&args);
>> if (error)
>> goto out_trans_cancel;
>> @@ -543,6 +564,10 @@ xfs_attr_remove(
>> */
>> xfs_trans_ijoin(args.trans, dp, 0);
>>
>> + error = xfs_has_attr(&args);
>> + if (error == -ENOATTR)
>> + goto out;
>> +
>
> Wouldn't we want to return any error that might occur here (except
> -EEXIST), not just -ENOATTR if there's actually no xattr?
>
> Brian

Ok, I will change this to (error != -EEXIST)
Thanks for the reviews!

Allison

>
>> error = xfs_attr_remove_args(&args);
>> if (error)
>> goto out;
>> --
>> 2.7.4
>>
>