| Message ID | 20180619102309.22464-1-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 19, 2018 at 6:26 PM Chengguang Xu <cgxu519@gmx.com> wrote: > > When the size of acl extended attribution is larger than pre-allocated > value buffer size, we will hit error '-ERANGE' and it's probabaly caused > by concurrent get/set acl from different clients. In this case, current > logic just sets acl to NULL so that we cannot get proper information but > the operation looks successful. > > This patch adds retry logic for error -ERANGE and return -EIO if fail > from retry. > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > --- > fs/ceph/acl.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > index 59cb307b15fb..f8750c09cec3 100644 > --- a/fs/ceph/acl.c > +++ b/fs/ceph/acl.c > @@ -48,6 +48,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > const char *name; > char *value = NULL; > struct posix_acl *acl; > + bool retried = false; > > switch (type) { > case ACL_TYPE_ACCESS: > @@ -60,6 +61,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > BUG(); > } > > +retry: > size = __ceph_getxattr(inode, name, "", 0); > if (size > 0) { > value = kzalloc(size, GFP_NOFS); > @@ -68,9 +70,16 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > size = __ceph_getxattr(inode, name, value, size); > } > > + if (size == -ERANGE && !retried) { > + retried = true; > + kfree(value); > + value = NULL; > + goto retry; > + } I think we should retry more times (maybe 10 times) for -ERANGE > + > 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 > acl = ERR_PTR(-EIO); > -- > 2.17.1 > > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Yan, Thanks for your suggestion, I'll fix in v2. BTW, currently we just translate most errors to -EIO, I know it's maybe helpful for reducing end users' confusing but on the other side, it will hides real reason of failure. So I want to add an error message to indicate real errno. What do you think? On 06/20/2018 10:39 AM, Yan, Zheng wrote: > On Tue, Jun 19, 2018 at 6:26 PM Chengguang Xu <cgxu519@gmx.com> wrote: >> When the size of acl extended attribution is larger than pre-allocated >> value buffer size, we will hit error '-ERANGE' and it's probabaly caused >> by concurrent get/set acl from different clients. In this case, current >> logic just sets acl to NULL so that we cannot get proper information but >> the operation looks successful. >> >> This patch adds retry logic for error -ERANGE and return -EIO if fail >> from retry. >> >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >> --- >> fs/ceph/acl.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c >> index 59cb307b15fb..f8750c09cec3 100644 >> --- a/fs/ceph/acl.c >> +++ b/fs/ceph/acl.c >> @@ -48,6 +48,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) >> const char *name; >> char *value = NULL; >> struct posix_acl *acl; >> + bool retried = false; >> >> switch (type) { >> case ACL_TYPE_ACCESS: >> @@ -60,6 +61,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) >> BUG(); >> } >> >> +retry: >> size = __ceph_getxattr(inode, name, "", 0); >> if (size > 0) { >> value = kzalloc(size, GFP_NOFS); >> @@ -68,9 +70,16 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) >> size = __ceph_getxattr(inode, name, value, size); >> } >> >> + if (size == -ERANGE && !retried) { >> + retried = true; >> + kfree(value); >> + value = NULL; >> + goto retry; >> + } > I think we should retry more times (maybe 10 times) for -ERANGE > > >> + >> 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 >> acl = ERR_PTR(-EIO); >> -- >> 2.17.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 20, 2018 at 11:34 AM cgxu519 <cgxu519@gmx.com> wrote: > > Hi Yan, > > Thanks for your suggestion, I'll fix in v2. > BTW, currently we just translate most errors to -EIO, I know it's maybe > helpful for reducing end users' confusing > but on the other side, it will hides real reason of failure. So I want > to add an error message to indicate real errno. > What do you think? Sounds good. thanks. Yan, Zheng > > On 06/20/2018 10:39 AM, Yan, Zheng wrote: > > On Tue, Jun 19, 2018 at 6:26 PM Chengguang Xu <cgxu519@gmx.com> wrote: > >> When the size of acl extended attribution is larger than pre-allocated > >> value buffer size, we will hit error '-ERANGE' and it's probabaly caused > >> by concurrent get/set acl from different clients. In this case, current > >> logic just sets acl to NULL so that we cannot get proper information but > >> the operation looks successful. > >> > >> This patch adds retry logic for error -ERANGE and return -EIO if fail > >> from retry. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > >> --- > >> fs/ceph/acl.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >> > >> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > >> index 59cb307b15fb..f8750c09cec3 100644 > >> --- a/fs/ceph/acl.c > >> +++ b/fs/ceph/acl.c > >> @@ -48,6 +48,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > >> const char *name; > >> char *value = NULL; > >> struct posix_acl *acl; > >> + bool retried = false; > >> > >> switch (type) { > >> case ACL_TYPE_ACCESS: > >> @@ -60,6 +61,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > >> BUG(); > >> } > >> > >> +retry: > >> size = __ceph_getxattr(inode, name, "", 0); > >> if (size > 0) { > >> value = kzalloc(size, GFP_NOFS); > >> @@ -68,9 +70,16 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) > >> size = __ceph_getxattr(inode, name, value, size); > >> } > >> > >> + if (size == -ERANGE && !retried) { > >> + retried = true; > >> + kfree(value); > >> + value = NULL; > >> + goto retry; > >> + } > > I think we should retry more times (maybe 10 times) for -ERANGE > > > > > >> + > >> 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 > >> acl = ERR_PTR(-EIO); > >> -- > >> 2.17.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" 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/ceph/acl.c b/fs/ceph/acl.c index 59cb307b15fb..f8750c09cec3 100644 --- a/fs/ceph/acl.c +++ b/fs/ceph/acl.c @@ -48,6 +48,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) const char *name; char *value = NULL; struct posix_acl *acl; + bool retried = false; switch (type) { case ACL_TYPE_ACCESS: @@ -60,6 +61,7 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) BUG(); } +retry: size = __ceph_getxattr(inode, name, "", 0); if (size > 0) { value = kzalloc(size, GFP_NOFS); @@ -68,9 +70,16 @@ struct posix_acl *ceph_get_acl(struct inode *inode, int type) size = __ceph_getxattr(inode, name, value, size); } + if (size == -ERANGE && !retried) { + retried = true; + kfree(value); + value = NULL; + goto retry; + } + 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 acl = ERR_PTR(-EIO);
When the size of acl extended attribution is larger than pre-allocated value buffer size, we will hit error '-ERANGE' and it's probabaly caused by concurrent get/set acl from different clients. In this case, current logic just sets acl to NULL so that we cannot get proper information but the operation looks successful. This patch adds retry logic for error -ERANGE and return -EIO if fail from retry. Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- fs/ceph/acl.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)