[v6,11/16] xfs: Check for -ENOATTR or -EEXIST
diff mbox series

Message ID 20200118225035.19503-12-allison.henderson@oracle.com
State Superseded
Headers show
Series
  • xfs: Delay Ready Attributes
Related show

Commit Message

Allison Collins Jan. 18, 2020, 10:50 p.m. UTC
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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Darrick J. Wong Jan. 21, 2020, 11:15 p.m. UTC | #1
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>

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?

--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);
>  	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;
> +
>  	error = xfs_attr_remove_args(&args);
>  	if (error)
>  		goto out;
> -- 
> 2.7.4
>
Allison Collins Jan. 22, 2020, 4:29 a.m. UTC | #2
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);
>>   	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;
>> +
>>   	error = xfs_attr_remove_args(&args);
>>   	if (error)
>>   		goto out;
>> -- 
>> 2.7.4
>>
Allison Collins Jan. 25, 2020, 4:41 p.m. UTC | #3
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?

>>>       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
>>>
Darrick J. Wong Jan. 26, 2020, 10:28 p.m. UTC | #4
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?

/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.

--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
> > > >
Allison Collins Jan. 27, 2020, 12:20 a.m. UTC | #5
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
>>>>>
Darrick J. Wong Jan. 30, 2020, 10:58 p.m. UTC | #6
On Sun, Jan 26, 2020 at 05:20:16PM -0700, Allison Collins wrote:
> 
> 
> 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.

Ok, it's purely a defensive check so that you'll notice problems early
before the fs goes bonkers, not a problem that mysteriously appears but
only after some random unexpected shutdown.

> > 
> > /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?

Yep.

--D

> > 
> > --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
> > > > > >

Patch
diff mbox series

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);
 	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;
+
 	error = xfs_attr_remove_args(&args);
 	if (error)
 		goto out;