| Message ID | 20180623125524.29233-2-cgxu519@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: > > Though val_size1/val_size2 are both type sizt_t but > ceph_pagelist_encode_string() can only handle string > size max to U32_MAX, so change hardcode 8 to sizeof(u32). > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > --- > fs/ceph/acl.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > index 3351ea14390b..f736108604a6 100644 > --- a/fs/ceph/acl.c > +++ b/fs/ceph/acl.c > @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > > if (acl) { > size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); > - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); > + err = ceph_pagelist_reserve(pagelist, len + val_size1 + > + sizeof(u32)); > if (err) > goto out_err; > ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, > @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > } > if (default_acl) { > size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); > - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); > + err = ceph_pagelist_reserve(pagelist, len + val_size2 + > + sizeof(u32)); > if (err) > goto out_err; > err = ceph_pagelist_encode_string(pagelist, This is wrong. In both cases it reserves pages for two string-like things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. Thanks, Ilya -- 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 06/24/2018 04:49 PM, Ilya Dryomov wrote: > On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: >> Though val_size1/val_size2 are both type sizt_t but >> ceph_pagelist_encode_string() can only handle string >> size max to U32_MAX, so change hardcode 8 to sizeof(u32). >> >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >> --- >> fs/ceph/acl.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c >> index 3351ea14390b..f736108604a6 100644 >> --- a/fs/ceph/acl.c >> +++ b/fs/ceph/acl.c >> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >> >> if (acl) { >> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); >> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); >> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + >> + sizeof(u32)); >> if (err) >> goto out_err; >> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, >> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >> } >> if (default_acl) { >> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); >> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); >> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + >> + sizeof(u32)); >> if (err) >> goto out_err; >> err = ceph_pagelist_encode_string(pagelist, > This is wrong. In both cases it reserves pages for two string-like > things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? Thanks, Chengguang. -- 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 Sun, Jun 24, 2018 at 10:57 AM cgxu519 <cgxu519@gmx.com> wrote: > > > On 06/24/2018 04:49 PM, Ilya Dryomov wrote: > > On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: > >> Though val_size1/val_size2 are both type sizt_t but > >> ceph_pagelist_encode_string() can only handle string > >> size max to U32_MAX, so change hardcode 8 to sizeof(u32). > >> > >> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > >> --- > >> fs/ceph/acl.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > >> index 3351ea14390b..f736108604a6 100644 > >> --- a/fs/ceph/acl.c > >> +++ b/fs/ceph/acl.c > >> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >> > >> if (acl) { > >> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); > >> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); > >> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + > >> + sizeof(u32)); > >> if (err) > >> goto out_err; > >> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, > >> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >> } > >> if (default_acl) { > >> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); > >> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); > >> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + > >> + sizeof(u32)); > >> if (err) > >> goto out_err; > >> err = ceph_pagelist_encode_string(pagelist, > > This is wrong. In both cases it reserves pages for two string-like > > things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. > Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? No, that would be unnecessary churn. Thanks, Ilya -- 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 06/24/2018 05:05 PM, Ilya Dryomov wrote: > On Sun, Jun 24, 2018 at 10:57 AM cgxu519 <cgxu519@gmx.com> wrote: >> >> On 06/24/2018 04:49 PM, Ilya Dryomov wrote: >>> On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: >>>> Though val_size1/val_size2 are both type sizt_t but >>>> ceph_pagelist_encode_string() can only handle string >>>> size max to U32_MAX, so change hardcode 8 to sizeof(u32). >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >>>> --- >>>> fs/ceph/acl.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c >>>> index 3351ea14390b..f736108604a6 100644 >>>> --- a/fs/ceph/acl.c >>>> +++ b/fs/ceph/acl.c >>>> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >>>> >>>> if (acl) { >>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); >>>> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); >>>> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + >>>> + sizeof(u32)); >>>> if (err) >>>> goto out_err; >>>> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, >>>> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >>>> } >>>> if (default_acl) { >>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); >>>> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); >>>> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + >>>> + sizeof(u32)); >>>> if (err) >>>> goto out_err; >>>> err = ceph_pagelist_encode_string(pagelist, >>> This is wrong. In both cases it reserves pages for two string-like >>> things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. >> Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? > No, that would be unnecessary churn. Probably we can change the first reservation size from PAGE_SIZE to sizeof(u32) in ceph_pre_init_acls() because actually we encode int type 2 or 1 using ceph_pagelist_encode_32(). Am I missing something? Thanks, Chengguang. -- 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 Tue, Jun 26, 2018 at 4:09 AM cgxu519 <cgxu519@gmx.com> wrote: > > > > On 06/24/2018 05:05 PM, Ilya Dryomov wrote: > > On Sun, Jun 24, 2018 at 10:57 AM cgxu519 <cgxu519@gmx.com> wrote: > >> > >> On 06/24/2018 04:49 PM, Ilya Dryomov wrote: > >>> On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: > >>>> Though val_size1/val_size2 are both type sizt_t but > >>>> ceph_pagelist_encode_string() can only handle string > >>>> size max to U32_MAX, so change hardcode 8 to sizeof(u32). > >>>> > >>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > >>>> --- > >>>> fs/ceph/acl.c | 6 ++++-- > >>>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > >>>> index 3351ea14390b..f736108604a6 100644 > >>>> --- a/fs/ceph/acl.c > >>>> +++ b/fs/ceph/acl.c > >>>> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >>>> > >>>> if (acl) { > >>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); > >>>> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); > >>>> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + > >>>> + sizeof(u32)); > >>>> if (err) > >>>> goto out_err; > >>>> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, > >>>> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >>>> } > >>>> if (default_acl) { > >>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); > >>>> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); > >>>> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + > >>>> + sizeof(u32)); > >>>> if (err) > >>>> goto out_err; > >>>> err = ceph_pagelist_encode_string(pagelist, > >>> This is wrong. In both cases it reserves pages for two string-like > >>> things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. > >> Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? > > No, that would be unnecessary churn. > Probably we can change the first reservation size from PAGE_SIZE to > sizeof(u32) > in ceph_pre_init_acls() because actually we encode int type 2 or 1 using > ceph_pagelist_encode_32(). Am I missing something? It wouldn't change anything. It is a fresly created pagelist (i.e. a list of pages). The granularity is a page, so even if you reserve just one byte, it would still allocate a page. Thanks, Ilya -- 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 06/26/2018 04:18 PM, Ilya Dryomov wrote: > On Tue, Jun 26, 2018 at 4:09 AM cgxu519 <cgxu519@gmx.com> wrote: >> >> >> On 06/24/2018 05:05 PM, Ilya Dryomov wrote: >>> On Sun, Jun 24, 2018 at 10:57 AM cgxu519 <cgxu519@gmx.com> wrote: >>>> On 06/24/2018 04:49 PM, Ilya Dryomov wrote: >>>>> On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: >>>>>> Though val_size1/val_size2 are both type sizt_t but >>>>>> ceph_pagelist_encode_string() can only handle string >>>>>> size max to U32_MAX, so change hardcode 8 to sizeof(u32). >>>>>> >>>>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> >>>>>> --- >>>>>> fs/ceph/acl.c | 6 ++++-- >>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c >>>>>> index 3351ea14390b..f736108604a6 100644 >>>>>> --- a/fs/ceph/acl.c >>>>>> +++ b/fs/ceph/acl.c >>>>>> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >>>>>> >>>>>> if (acl) { >>>>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); >>>>>> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); >>>>>> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + >>>>>> + sizeof(u32)); >>>>>> if (err) >>>>>> goto out_err; >>>>>> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, >>>>>> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, >>>>>> } >>>>>> if (default_acl) { >>>>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); >>>>>> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); >>>>>> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + >>>>>> + sizeof(u32)); >>>>>> if (err) >>>>>> goto out_err; >>>>>> err = ceph_pagelist_encode_string(pagelist, >>>>> This is wrong. In both cases it reserves pages for two string-like >>>>> things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. >>>> Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? >>> No, that would be unnecessary churn. >> Probably we can change the first reservation size from PAGE_SIZE to >> sizeof(u32) >> in ceph_pre_init_acls() because actually we encode int type 2 or 1 using >> ceph_pagelist_encode_32(). Am I missing something? > It wouldn't change anything. It is a fresly created pagelist (i.e. > a list of pages). The granularity is a page, so even if you reserve > just one byte, it would still allocate a page. I know what do you mean and I agree actually it changes nothing in technique. I just hope to make the code a bit more readable here. Thanks, Chengguang. -- 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 Tue, Jun 26, 2018 at 10:41 AM cgxu519 <cgxu519@gmx.com> wrote: > > On 06/26/2018 04:18 PM, Ilya Dryomov wrote: > > On Tue, Jun 26, 2018 at 4:09 AM cgxu519 <cgxu519@gmx.com> wrote: > >> > >> > >> On 06/24/2018 05:05 PM, Ilya Dryomov wrote: > >>> On Sun, Jun 24, 2018 at 10:57 AM cgxu519 <cgxu519@gmx.com> wrote: > >>>> On 06/24/2018 04:49 PM, Ilya Dryomov wrote: > >>>>> On Sat, Jun 23, 2018 at 2:55 PM Chengguang Xu <cgxu519@gmx.com> wrote: > >>>>>> Though val_size1/val_size2 are both type sizt_t but > >>>>>> ceph_pagelist_encode_string() can only handle string > >>>>>> size max to U32_MAX, so change hardcode 8 to sizeof(u32). > >>>>>> > >>>>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com> > >>>>>> --- > >>>>>> fs/ceph/acl.c | 6 ++++-- > >>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c > >>>>>> index 3351ea14390b..f736108604a6 100644 > >>>>>> --- a/fs/ceph/acl.c > >>>>>> +++ b/fs/ceph/acl.c > >>>>>> @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >>>>>> > >>>>>> if (acl) { > >>>>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); > >>>>>> - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); > >>>>>> + err = ceph_pagelist_reserve(pagelist, len + val_size1 + > >>>>>> + sizeof(u32)); > >>>>>> if (err) > >>>>>> goto out_err; > >>>>>> ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, > >>>>>> @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, > >>>>>> } > >>>>>> if (default_acl) { > >>>>>> size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); > >>>>>> - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); > >>>>>> + err = ceph_pagelist_reserve(pagelist, len + val_size2 + > >>>>>> + sizeof(u32)); > >>>>>> if (err) > >>>>>> goto out_err; > >>>>>> err = ceph_pagelist_encode_string(pagelist, > >>>>> This is wrong. In both cases it reserves pages for two string-like > >>>>> things, str_size + 4 each, for a total of str_size1 + str_size2 + 8. > >>>> Ah, yes. Could we change to sizeof(u32) + sizeof(u32) instead? > >>> No, that would be unnecessary churn. > >> Probably we can change the first reservation size from PAGE_SIZE to > >> sizeof(u32) > >> in ceph_pre_init_acls() because actually we encode int type 2 or 1 using > >> ceph_pagelist_encode_32(). Am I missing something? > > It wouldn't change anything. It is a fresly created pagelist (i.e. > > a list of pages). The granularity is a page, so even if you reserve > > just one byte, it would still allocate a page. > > I know what do you mean and I agree actually it changes nothing in > technique. > I just hope to make the code a bit more readable here. Then let's remove that call to ceph_pagelist_reserve() altogether and check the return value of ceph_pagelist_encode_32() instead. Thanks, Ilya -- 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 3351ea14390b..f736108604a6 100644 --- a/fs/ceph/acl.c +++ b/fs/ceph/acl.c @@ -219,7 +219,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, if (acl) { size_t len = strlen(XATTR_NAME_POSIX_ACL_ACCESS); - err = ceph_pagelist_reserve(pagelist, len + val_size1 + 8); + err = ceph_pagelist_reserve(pagelist, len + val_size1 + + sizeof(u32)); if (err) goto out_err; ceph_pagelist_encode_string(pagelist, XATTR_NAME_POSIX_ACL_ACCESS, @@ -233,7 +234,8 @@ int ceph_pre_init_acls(struct inode *dir, umode_t *mode, } if (default_acl) { size_t len = strlen(XATTR_NAME_POSIX_ACL_DEFAULT); - err = ceph_pagelist_reserve(pagelist, len + val_size2 + 8); + err = ceph_pagelist_reserve(pagelist, len + val_size2 + + sizeof(u32)); if (err) goto out_err; err = ceph_pagelist_encode_string(pagelist,
Though val_size1/val_size2 are both type sizt_t but ceph_pagelist_encode_string() can only handle string size max to U32_MAX, so change hardcode 8 to sizeof(u32). Signed-off-by: Chengguang Xu <cgxu519@gmx.com> --- fs/ceph/acl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)