[PATCH] erofs: avoid useless memory allocation

[PATCH] erofs: avoid useless memory allocation

[Noboru Asai]

The variable 'vi->xattr_shared_count' could be ZERO.

Signed-off-by: Noboru Asai <asai@sijam.com>
---
 fs/erofs/xattr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
index 60729b1220b6..5164813a693b 100644
--- a/fs/erofs/xattr.c
+++ b/fs/erofs/xattr.c
@@ -80,6 +80,8 @@ static int init_inode_xattrs(struct inode *inode)
 
 	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
 	vi->xattr_shared_count = ih->h_shared_count;
+	if (!vi->xattr_shared_count)
+		goto out_unlock;
 	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
 						sizeof(uint), GFP_KERNEL);
 	if (!vi->xattr_shared_xattrs) {
-- 
2.39.2

Re: [PATCH] erofs: avoid useless memory allocation

[Gao Xiang]

On 2023/3/3 15:52, Noboru Asai wrote:
> The variable 'vi->xattr_shared_count' could be ZERO.
> 
> Signed-off-by: Noboru Asai <asai@sijam.com>

Reviewed-by: Gao Xiang <hsiangkao@linux.alibaba.com>

Thanks,
Gao Xiang

> ---
>   fs/erofs/xattr.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 60729b1220b6..5164813a693b 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -80,6 +80,8 @@ static int init_inode_xattrs(struct inode *inode)
>   
>   	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>   	vi->xattr_shared_count = ih->h_shared_count;
> +	if (!vi->xattr_shared_count)
> +		goto out_unlock;
>   	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>   						sizeof(uint), GFP_KERNEL);
>   	if (!vi->xattr_shared_xattrs) {

Re: [PATCH] erofs: avoid useless memory allocation

[Yue Hu]

On Fri,  3 Mar 2023 16:52:18 +0900
Noboru Asai <asai@sijam.com> wrote:

> The variable 'vi->xattr_shared_count' could be ZERO.
> 
> Signed-off-by: Noboru Asai <asai@sijam.com>
> ---
>  fs/erofs/xattr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> index 60729b1220b6..5164813a693b 100644
> --- a/fs/erofs/xattr.c
> +++ b/fs/erofs/xattr.c
> @@ -80,6 +80,8 @@ static int init_inode_xattrs(struct inode *inode)
>  
>  	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>  	vi->xattr_shared_count = ih->h_shared_count;
> +	if (!vi->xattr_shared_count)
> +		goto out_unlock;

Questions: ret = 0? no need to erofs_put_metabuf?

I think we can keep current since kmalloc_array() will check whether the
size(->xattr_shared_count) is zero size or not. rt?

>  	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>  						sizeof(uint), GFP_KERNEL);
>  	if (!vi->xattr_shared_xattrs) {

Re: [PATCH] erofs: avoid useless memory allocation

[Gao Xiang]

On 2023/3/3 17:45, Yue Hu wrote:
> On Fri,  3 Mar 2023 16:52:18 +0900
> Noboru Asai <asai@sijam.com> wrote:
> 
>> The variable 'vi->xattr_shared_count' could be ZERO.
>>
>> Signed-off-by: Noboru Asai <asai@sijam.com>
>> ---
>>   fs/erofs/xattr.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
>> index 60729b1220b6..5164813a693b 100644
>> --- a/fs/erofs/xattr.c
>> +++ b/fs/erofs/xattr.c
>> @@ -80,6 +80,8 @@ static int init_inode_xattrs(struct inode *inode)
>>   
>>   	ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
>>   	vi->xattr_shared_count = ih->h_shared_count;
>> +	if (!vi->xattr_shared_count)
>> +		goto out_unlock;
> 
> Questions: ret = 0? no need to erofs_put_metabuf?
> 
> I think we can keep current since kmalloc_array() will check whether the
> size(->xattr_shared_count) is zero size or not. rt?

Personally I'd like to avoid 0-sized kmalloc, but you're right,
we have to do erofs_put_metabuf() here and it needs to be fixed.

> 
>>   	vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
>>   						sizeof(uint), GFP_KERNEL);
>>   	if (!vi->xattr_shared_xattrs) {

Re: [PATCH] erofs: avoid useless memory allocation

[浅井登]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

2023年3月3日(金) 18:39 Yue Hu <zbestahu@gmail.com>:

> On Fri,  3 Mar 2023 16:52:18 +0900
> Noboru Asai <asai@sijam.com> wrote:
>
> > The variable 'vi->xattr_shared_count' could be ZERO.
> >
> > Signed-off-by: Noboru Asai <asai@sijam.com>
> > ---
> >  fs/erofs/xattr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/erofs/xattr.c b/fs/erofs/xattr.c
> > index 60729b1220b6..5164813a693b 100644
> > --- a/fs/erofs/xattr.c
> > +++ b/fs/erofs/xattr.c
> > @@ -80,6 +80,8 @@ static int init_inode_xattrs(struct inode *inode)
> >
> >       ih = (struct erofs_xattr_ibody_header *)(it.kaddr + it.ofs);
> >       vi->xattr_shared_count = ih->h_shared_count;
> > +     if (!vi->xattr_shared_count)
> > +             goto out_unlock;
>
> Questions: ret = 0? no need to erofs_put_metabuf?
>

You are right. this patch needs to call erofs_put_metabuf() before goto
out_unlock.


> I think we can keep current since kmalloc_array() will check whether the
> size(->xattr_shared_count) is zero size or not. rt?
>

In case of xattr_shared_count == 0, kmalloc_array() return ZERO_SIZE_PTR.
And kfree() do noting in this case (safe).

#define ZERO_SIZE_PTR ((void *)16)



> >       vi->xattr_shared_xattrs = kmalloc_array(vi->xattr_shared_count,
> >                                               sizeof(uint), GFP_KERNEL);
> >       if (!vi->xattr_shared_xattrs) {
>
>

[-- Attachment #2: Type: text/html, Size: 3138 bytes --]