| Message ID | 20180622025816.29239-1-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)