* [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() @ 2018-06-22 2:58 Chengguang Xu 2018-06-22 5:59 ` Qu Wenruo 2018-06-22 10:48 ` David Sterba 0 siblings, 2 replies; 6+ messages in thread From: Chengguang Xu @ 2018-06-22 2:58 UTC (permalink / raw) To: clm, jbacik, dsterba; +Cc: linux-btrfs, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() 2018-06-22 2:58 [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() Chengguang Xu @ 2018-06-22 5:59 ` Qu Wenruo 2018-06-22 7:42 ` cgxu519 2018-06-22 10:48 ` David Sterba 1 sibling, 1 reply; 6+ messages in thread From: Qu Wenruo @ 2018-06-22 5:59 UTC (permalink / raw) To: Chengguang Xu, clm, jbacik, dsterba; +Cc: linux-btrfs [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() 2018-06-22 5:59 ` Qu Wenruo @ 2018-06-22 7:42 ` cgxu519 2018-06-22 7:58 ` Qu Wenruo 0 siblings, 1 reply; 6+ messages in thread From: cgxu519 @ 2018-06-22 7:42 UTC (permalink / raw) To: Qu Wenruo, clm, jbacik, dsterba; +Cc: linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() 2018-06-22 7:42 ` cgxu519 @ 2018-06-22 7:58 ` Qu Wenruo 0 siblings, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2018-06-22 7:58 UTC (permalink / raw) To: cgxu519, clm, jbacik, dsterba; +Cc: linux-btrfs [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() 2018-06-22 2:58 [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() Chengguang Xu 2018-06-22 5:59 ` Qu Wenruo @ 2018-06-22 10:48 ` David Sterba 2018-06-23 3:40 ` cgxu519 1 sibling, 1 reply; 6+ messages in thread From: David Sterba @ 2018-06-22 10:48 UTC (permalink / raw) To: Chengguang Xu; +Cc: clm, jbacik, dsterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() 2018-06-22 10:48 ` David Sterba @ 2018-06-23 3:40 ` cgxu519 0 siblings, 0 replies; 6+ messages in thread From: cgxu519 @ 2018-06-23 3:40 UTC (permalink / raw) To: dsterba, clm, jbacik, dsterba, linux-btrfs 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-23 3:40 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-22 2:58 [PATCH] btrfs: treat -ERANGE as an error case in btrfs_get_acl() Chengguang Xu 2018-06-22 5:59 ` Qu Wenruo 2018-06-22 7:42 ` cgxu519 2018-06-22 7:58 ` Qu Wenruo 2018-06-22 10:48 ` David Sterba 2018-06-23 3:40 ` cgxu519
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.