[PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[weiyj_lk]

From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>

Use vzalloc() instead of vmalloc() and memset(0).

Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
---
 fs/orangefs/devorangefs-req.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
index a287a66..e839073 100644
--- a/fs/orangefs/devorangefs-req.c
+++ b/fs/orangefs/devorangefs-req.c
@@ -442,14 +442,12 @@ static ssize_t orangefs_devreq_write_iter(struct kiocb *iocb,
 	if (op->downcall.type != ORANGEFS_VFS_OP_READDIR)
 		goto wakeup;
 
-	op->downcall.trailer_buf =
-		vmalloc(op->downcall.trailer_size);
+	op->downcall.trailer_buf = vzalloc(op->downcall.trailer_size);
 	if (op->downcall.trailer_buf == NULL) {
 		gossip_err("%s: failed trailer vmalloc.\n",
 			   __func__);
 		goto Enomem;
 	}
-	memset(op->downcall.trailer_buf, 0, op->downcall.trailer_size);
 	n = copy_from_iter(op->downcall.trailer_buf,
 			   op->downcall.trailer_size,
 			   iter);

Re: [PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[Mike Marshall]

Wei...

I have applied your patch and tested it, it seems good.

The gossip statement still references vmalloc, I used
"git commit --amend" to fix that for my test. I'll do the
same when I push to kernel.org too, if you're OK with that,
unless you want to fix it yourself and resend the patch...

Thanks!

-Mike

On Wed, Jul 6, 2016 at 9:23 AM,  <weiyj_lk@163.com> wrote:
> From: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
>
> Use vzalloc() instead of vmalloc() and memset(0).
>
> Signed-off-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
> ---
>  fs/orangefs/devorangefs-req.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
> index a287a66..e839073 100644
> --- a/fs/orangefs/devorangefs-req.c
> +++ b/fs/orangefs/devorangefs-req.c
> @@ -442,14 +442,12 @@ static ssize_t orangefs_devreq_write_iter(struct kiocb *iocb,
>         if (op->downcall.type != ORANGEFS_VFS_OP_READDIR)
>                 goto wakeup;
>
> -       op->downcall.trailer_buf =
> -               vmalloc(op->downcall.trailer_size);
> +       op->downcall.trailer_buf = vzalloc(op->downcall.trailer_size);
>         if (op->downcall.trailer_buf == NULL) {
>                 gossip_err("%s: failed trailer vmalloc.\n",
>                            __func__);
>                 goto Enomem;
>         }
> -       memset(op->downcall.trailer_buf, 0, op->downcall.trailer_size);
>         n = copy_from_iter(op->downcall.trailer_buf,
>                            op->downcall.trailer_size,
>                            iter);
>
>
>
>

Re: [PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[Joe Perches]

On Wed, 2016-07-06 at 13:28 -0400, Mike Marshall wrote:
> Wei...
> 
> I have applied your patch and tested it, it seems good.
> 
> The gossip statement still references vmalloc, I used
> "git commit --amend" to fix that for my test. I'll do the
> same when I push to kernel.org too, if you're OK with that,
> unless you want to fix it yourself and resend the patch...

Better still would be to remove the gossip_err() altogether
as v.alloc already does a dump_stack on OOM.
> > diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
[]
> > @@ -442,14 +442,12 @@ static ssize_t orangefs_devreq_write_iter(struct kiocb *iocb,
> >         if (op->downcall.type != ORANGEFS_VFS_OP_READDIR)
> >                 goto wakeup;
> > 
> > -       op->downcall.trailer_buf =
> > -               vmalloc(op->downcall.trailer_size);
> > +       op->downcall.trailer_buf = vzalloc(op->downcall.trailer_size);
> >         if (op->downcall.trailer_buf == NULL) {
> >                 gossip_err("%s: failed trailer vmalloc.\n",
> >                            __func__);
> >                 goto Enomem;
> >         }
> > -       memset(op->downcall.trailer_buf, 0, op->downcall.trailer_size);

Re: [PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[Mike Marshall]

Hi Joe...

I looked around at other code to see how they were using vzalloc...
even if it is just the caller of the function that uses vzalloc, most
fs's that use vzalloc print an error message when vzalloc fails... I sure do
like to follow the flow of our kernel module all in one place
with the gossip statements...

-Mike

On Wed, Jul 6, 2016 at 1:49 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2016-07-06 at 13:28 -0400, Mike Marshall wrote:
>> Wei...
>>
>> I have applied your patch and tested it, it seems good.
>>
>> The gossip statement still references vmalloc, I used
>> "git commit --amend" to fix that for my test. I'll do the
>> same when I push to kernel.org too, if you're OK with that,
>> unless you want to fix it yourself and resend the patch...
>
> Better still would be to remove the gossip_err() altogether
> as v.alloc already does a dump_stack on OOM.
>> > diff --git a/fs/orangefs/devorangefs-req.c b/fs/orangefs/devorangefs-req.c
> []
>> > @@ -442,14 +442,12 @@ static ssize_t orangefs_devreq_write_iter(struct kiocb *iocb,
>> >         if (op->downcall.type != ORANGEFS_VFS_OP_READDIR)
>> >                 goto wakeup;
>> >
>> > -       op->downcall.trailer_buf =
>> > -               vmalloc(op->downcall.trailer_size);
>> > +       op->downcall.trailer_buf = vzalloc(op->downcall.trailer_size);
>> >         if (op->downcall.trailer_buf == NULL) {
>> >                 gossip_err("%s: failed trailer vmalloc.\n",
>> >                            __func__);
>> >                 goto Enomem;
>> >         }
>> > -       memset(op->downcall.trailer_buf, 0, op->downcall.trailer_size);
>

Re: [PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[Joe Perches]

On Wed, 2016-07-06 at 14:00 -0400, Mike Marshall wrote:
> Hi Joe...

Hi Mike.

> I looked around at other code to see how they were using vzalloc...
> even if it is just the caller of the function that uses vzalloc, most
> fs's that use vzalloc print an error message when vzalloc fails...

That's incorrect.

There are 72 v.alloc calls in fs/
2 of those calls have a specific OOM message.
There's 1 in reiserfs (old and unsupported) and 1 in orangefs

>  I sure do
> like to follow the flow of our kernel module all in one place
> with the gossip statements...

I don't see how that matters here as there's
already an OOM on v.alloc failure.

cheers, Joe

Re: [PATCH -next] orangefs: use vzalloc() instead of vmalloc()/memset(0)

[Mike Marshall]

Joe> That's incorrect.

<blush>... you are right. They mostly return -ENOMEM, but they
mostly don't print an error message. I like printing an error message
when I resolve to an error... a whole bunch of that function is sanity
checking input from userspace, with a specific error message for
any particular failure.

Joe> I don't see how that matters here as there's
Joe> already an OOM on v.alloc failure.

I'd find that if needed, but the first place I'd look when investigating
an Orangefs failure would be the Orangefs logs...

-Mike

On Wed, Jul 6, 2016 at 2:11 PM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2016-07-06 at 14:00 -0400, Mike Marshall wrote:
>> Hi Joe...
>
> Hi Mike.
>
>> I looked around at other code to see how they were using vzalloc...
>> even if it is just the caller of the function that uses vzalloc, most
>> fs's that use vzalloc print an error message when vzalloc fails...
>
> That's incorrect.
>
> There are 72 v.alloc calls in fs/
> 2 of those calls have a specific OOM message.
> There's 1 in reiserfs (old and unsupported) and 1 in orangefs
>
>>  I sure do
>> like to follow the flow of our kernel module all in one place
>> with the gossip statements...
>
> I don't see how that matters here as there's
> already an OOM on v.alloc failure.
>
> cheers, Joe