[PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[Chengguang Xu]

Currently, when encoutering -ERANGE in btrfs_get_acl(),
just set acl to NULL so that we cannot get proper
acl information but the operation looks successful.

This patch treats -ERANGE as an error case and meanwhile
print real errno before translating errno to -EIO.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/btrfs/acl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
index 15e1dfef56a5..7b3a83dd917c 100644
--- a/fs/btrfs/acl.c
+++ b/fs/btrfs/acl.c
@@ -42,9 +42,10 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
 	}
 	if (size > 0) {
 		acl = posix_acl_from_xattr(&init_user_ns, value, size);
-	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
+	} else if (size == -ENODATA || size == 0) {
 		acl = NULL;
 	} else {
+		pr_err_ratelimited("BTRFS: get acl failed, err=%d\n", size);
 		acl = ERR_PTR(-EIO);
 	}
 	kfree(value);
-- 
2.17.1

Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1414 bytes --]



On 2018年06月22日 10:58, Chengguang Xu wrote:
> Currently, when encoutering -ERANGE in btrfs_get_acl(),
> just set acl to NULL so that we cannot get proper
> acl information but the operation looks successful.
> 
> This patch treats -ERANGE as an error case and meanwhile
> print real errno before translating errno to -EIO.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> ---
>  fs/btrfs/acl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
> index 15e1dfef56a5..7b3a83dd917c 100644
> --- a/fs/btrfs/acl.c
> +++ b/fs/btrfs/acl.c
> @@ -42,9 +42,10 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
>  	}
>  	if (size > 0) {
>  		acl = posix_acl_from_xattr(&init_user_ns, value, size);
> -	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
> +	} else if (size == -ENODATA || size == 0) {
>  		acl = NULL;
>  	} else {
> +		pr_err_ratelimited("BTRFS: get acl failed, err=%d\n", size);

Is there any special reason to output this message even it's rate limited?
This looks much like a debug output, no to mention we have
btrfs_err/warn/info() wrapper to output with proper fs UUID.

>  		acl = ERR_PTR(-EIO);

in fact we should let @acl to contain the correct error code from
btrfs_getxattr(), other than overriding it with -EIO.

Thanks,
Qu

>  	}
>  	kfree(value);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[cgxu519]

On 06/22/2018 01:59 PM, Qu Wenruo wrote:
>
> On 2018年06月22日 10:58, Chengguang Xu wrote:
>> Currently, when encoutering -ERANGE in btrfs_get_acl(),
>> just set acl to NULL so that we cannot get proper
>> acl information but the operation looks successful.
>>
>> This patch treats -ERANGE as an error case and meanwhile
>> print real errno before translating errno to -EIO.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>> ---
>>   fs/btrfs/acl.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
>> index 15e1dfef56a5..7b3a83dd917c 100644
>> --- a/fs/btrfs/acl.c
>> +++ b/fs/btrfs/acl.c
>> @@ -42,9 +42,10 @@ struct posix_acl *btrfs_get_acl(struct inode *inode, int type)
>>   	}
>>   	if (size > 0) {
>>   		acl = posix_acl_from_xattr(&init_user_ns, value, size);
>> -	} else if (size == -ERANGE || size == -ENODATA || size == 0) {
>> +	} else if (size == -ENODATA || size == 0) {
>>   		acl = NULL;
>>   	} else {
>> +		pr_err_ratelimited("BTRFS: get acl failed, err=%d\n", size);
> Is there any special reason to output this message even it's rate limited?
> This looks much like a debug output, no to mention we have
> btrfs_err/warn/info() wrapper to output with proper fs UUID.
Yeah, it will be better replacing pr_err with btrfs_err. The motivation to
print error message here is for helping debug when failing into error case.
As you know, most error code here will be override to -EIO, so I hope
to record a hint to indicate what has happened.

>
>>   		acl = ERR_PTR(-EIO);
> in fact we should let @acl to contain the correct error code from
> btrfs_getxattr(), other than overriding it with -EIO.
I'm also not so sure about the reason for overriding the errno with -EIO,
maybe it's easy to understand for end-user?

Thanks,
Chengguang.

Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2559 bytes --]



On 2018年06月22日 15:42, cgxu519 wrote:
> On 06/22/2018 01:59 PM, Qu Wenruo wrote:
>>
>> On 2018年06月22日 10:58, Chengguang Xu wrote:
>>> Currently, when encoutering -ERANGE in btrfs_get_acl(),
>>> just set acl to NULL so that we cannot get proper
>>> acl information but the operation looks successful.
>>>
>>> This patch treats -ERANGE as an error case and meanwhile
>>> print real errno before translating errno to -EIO.
>>>
>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>> ---
>>>   fs/btrfs/acl.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c
>>> index 15e1dfef56a5..7b3a83dd917c 100644
>>> --- a/fs/btrfs/acl.c
>>> +++ b/fs/btrfs/acl.c
>>> @@ -42,9 +42,10 @@ struct posix_acl *btrfs_get_acl(struct inode
>>> *inode, int type)
>>>       }
>>>       if (size > 0) {
>>>           acl = posix_acl_from_xattr(&init_user_ns, value, size);
>>> -    } else if (size == -ERANGE || size == -ENODATA || size == 0) {
>>> +    } else if (size == -ENODATA || size == 0) {
>>>           acl = NULL;
>>>       } else {
>>> +        pr_err_ratelimited("BTRFS: get acl failed, err=%d\n", size);
>> Is there any special reason to output this message even it's rate
>> limited?
>> This looks much like a debug output, no to mention we have
>> btrfs_err/warn/info() wrapper to output with proper fs UUID.
> Yeah, it will be better replacing pr_err with btrfs_err. The motivation to
> print error message here is for helping debug when failing into error case.
> As you know, most error code here will be override to -EIO, so I hope
> to record a hint to indicate what has happened.

If it's something went wrong searching the tree, the return value will
be -EIO and more useful error message would be output, like tree block
csum error.

If other case, like ENOMEM, returning the original errno will be much
better, and more obvious for end-user.

Or did you hit some special case where needs the extra handling?
If so, maybe btrfs_debug_rl() would be a better fit here.

> 
>>
>>>           acl = ERR_PTR(-EIO);
>> in fact we should let @acl to contain the correct error code from
>> btrfs_getxattr(), other than overriding it with -EIO.
> I'm also not so sure about the reason for overriding the errno with -EIO,
> maybe it's easy to understand for end-user?

Just some bad old practice.
Feel free to correct it.

Thanks,
Qu

> 
> Thanks,
> Chengguang.
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[David Sterba]

On Fri, Jun 22, 2018 at 10:58:16AM +0800, Chengguang Xu wrote:
> Currently, when encoutering -ERANGE in btrfs_get_acl(),
> just set acl to NULL so that we cannot get proper
> acl information but the operation looks successful.

Do you have a reproducer for that?

ERANGE is returned from btrfs_getxattr in case the buffer is not large
enough to store the restult, but the first call to btrfs_getxattr will
read the size and then a temporary buffer of that size is allocated.

So ERANGE should not be able to make it to the the condition at all.

Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl()

[cgxu519]

On 06/22/2018 06:48 PM, David Sterba wrote:
> On Fri, Jun 22, 2018 at 10:58:16AM +0800, Chengguang Xu wrote:
>> Currently, when encoutering -ERANGE in btrfs_get_acl(),
>> just set acl to NULL so that we cannot get proper
>> acl information but the operation looks successful.
> Do you have a reproducer for that?
>
> ERANGE is returned from btrfs_getxattr in case the buffer is not large
> enough to store the restult, but the first call to btrfs_getxattr will
> read the size and then a temporary buffer of that size is allocated.
>
> So ERANGE should not be able to make it to the the condition at all.
Yes, I think you are right.

It might only happen in distributed filesystems(like cephfs) by concurrent
set/get from different clients, so in this case, checking -ERANGE condition
is reasonable(can add retry or some other error handlings) but the check
seems meaningless for local filesystems.

I have tested on some local filesystems before posting patch and found
there is no chance to make it to the -ERANGE condition. Even if we can
get into that condition set acl to NULL looks not correct.

In any case, I think we should remove the check 'size == -ERANGE' in 
btrfs_gat_acl(),
maybe I should change commit log to avoid misunderstanding.


Thanks,
Chengguang.