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 <cgxu...@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. > >
signature.asc
Description: OpenPGP digital signature
