Re: [PATCH v6 11/16] xfs: Check for -ENOATTR or -EEXIST

Allison Collins
2020-01-27T00:22:21Z

On 1/26/20 3:28 PM, Darrick J. Wong wrote:
> On Sat, Jan 25, 2020 at 09:41:47AM -0700, Allison Collins wrote:
>> On 1/21/20 9:29 PM, Allison Collins wrote:
>>>
>>>
>>> On 1/21/20 4:15 PM, Darrick J. Wong wrote:
>>>> On Sat, Jan 18, 2020 at 03:50:30PM -0700, Allison Collins wrote:
>>>>> Delayed operations cannot return error codes.  So we must check for
>>>>> these conditions first before starting set or remove operations
>>>>
>>>> Answering my own question from earlier -- I see here you actually /are/
>>>> checking the attr existence w.r.t. ATTR_{CREATE,REPLACE} right after we
>>>> allocate a transaction and ILOCK the inode, so
>>>>
>>>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> Alrighty, thank you!
>>>
>>>>
>>>> Though I am wondering if you could discard the predicates from the
>>>> second patch in favor of doing a normal lookup of the attr with a zero
>>>> valuelen to determine if there's already an attribute?
>>> I think I likely answered this in the response to that patch.  Because
>>> it's used as part of the remove procedures, we still need it.  We could
>>> make a simpler version just for this application I suppose, but it seems
>>> like it'd just be extra code since we still need the former.
>>>
>>> Thank you for the reviews!
>>> Allison
>>>
>>>>
>>>> --D
>>>>
>>>>> Signed-off-by: Allison Collins <allison.henderson@oracle.com>
>>>>> ---
>>>>>   fs/xfs/libxfs/xfs_attr.c | 12 ++++++++++++
>>>>>   1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
>>>>> index a2673fe..e9d22c1 100644
>>>>> --- a/fs/xfs/libxfs/xfs_attr.c
>>>>> +++ b/fs/xfs/libxfs/xfs_attr.c
>>>>> @@ -457,6 +457,14 @@ xfs_attr_set(
>>>>>           goto out_trans_cancel;
>>>>>       xfs_trans_ijoin(args.trans, dp, 0);
>>>>> +
>>>>> +    error = xfs_has_attr(&args);
>>>>> +    if (error == -EEXIST && (name->type & ATTR_CREATE))
>>>>> +        goto out_trans_cancel;
>>>>> +
>>>>> +    if (error == -ENOATTR && (name->type & ATTR_REPLACE))
>>>>> +        goto out_trans_cancel;
>>>>> +
>>>>>       error = xfs_attr_set_args(&args);
>> So I was thinking of adding this one to a smaller 3 patch series I mentioned
>> earlier. I was also thinking of adding in some asserts here:
>>
>> ASSERT(error != -EEXIST)
>> ASSERT(error != -ENOATTR)
>>
>> Just to make sure the changes are enforcing the behavioral changes that we
>> want. I thought this might be a good stabilizer to the rest of the delayed
>> attr series. Because chasing this bug back up through the log replay is a
>> much bigger PITA than catching it here. Thoughts?
>
> Er, are the asserts to check that xfs_attr_set_args never returns
> EEXIST/ENOATTR? I'm not sure why you'd have to chase this through log
> replay?

Yes, the idea is that EEXIST and ENOATTR are supposed to be found and
returned by the xfs_has_attr routine above. If they happen at this
point, it would actually be more of an internal error. For example: if
we're renaming an attr, and xfs_has_attr finds that it exists, but then
xfs_attr_set_args comes back with ENOATTR, clearly something unexpected
happened.

The motivation for this is just that if it does happen, it's easier to
work out this out now rather than later when we bring in the rest of the
delayed attribute code. Because in that case, the error wont happen
here, it will happen later as part of a finish_item (or a log replay).

It's not a requirement I suppose, just more of a pro-active check really.

>
> /me is in this funny place where he thinks that in general adding
> asserts (or WARN_ON) to check assumptions is a good idea, but not sure
> what's going on here.
Did that answer your question then?

>
> --D
>
>>>>>       if (error)
>>>>>           goto out_trans_cancel;
>>>>> @@ -545,6 +553,10 @@ xfs_attr_remove(
>>>>>        */
>>>>>       xfs_trans_ijoin(args.trans, dp, 0);
>>>>> +    error = xfs_has_attr(&args);
>>>>> +    if (error != -EEXIST)
>>>>> +        goto out;
>>>>> +
>> Here too:
>> ASSERT(error != -EEXIST)
>>
>> Let me know what folks think. Thanks!
>>
>> Allison
>>
>>>>>       error = xfs_attr_remove_args(&args);
>>>>>       if (error)
>>>>>           goto out;
>>>>> --
>>>>> 2.7.4
>>>>>