[v1] NFS: simplify check for freeing cn_resp

[PATCH] NFS: simplify check for freeing cn_resp

Posted by trix@redhat.com 4 years, 5 months ago

From: Tom Rix <trix@redhat.com>

nfs42_files_from_same_server() is called to check if freeing
cn_resp is required.  Instead of calling a function, check
the pointer.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 fs/nfs/nfs4file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index e79ae4cbc395e..677631ea4cfb3 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -180,7 +180,7 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
 	ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count,
 				nss, cnrs, sync);
 out:
-	if (!nfs42_files_from_same_server(file_in, file_out))
+	if (cn_resp)
 		kfree(cn_resp);
 	if (ret == -EAGAIN)
 		goto retry;
-- 
2.26.3

Re: [PATCH] NFS: simplify check for freeing cn_resp

Posted by Trond Myklebust 4 years, 5 months ago

Hi Tom,

On Sun, 2022-01-16 at 06:43 -0800, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> nfs42_files_from_same_server() is called to check if freeing
> cn_resp is required.  Instead of calling a function, check
> the pointer.
> 
> Signed-off-by: Tom Rix <trix@redhat.com>
> ---
>  fs/nfs/nfs4file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index e79ae4cbc395e..677631ea4cfb3 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -180,7 +180,7 @@ static ssize_t __nfs4_copy_file_range(struct file
> *file_in, loff_t pos_in,
>         ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out,
> count,
>                                 nss, cnrs, sync);
>  out:
> -       if (!nfs42_files_from_same_server(file_in, file_out))
> +       if (cn_resp)
>                 kfree(cn_resp);

The kernel convention in these circumstances is to just skip the NULL
pointer check, since kfree() does that anyway.

>         if (ret == -EAGAIN)
>                 goto retry;

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH] NFS: simplify check for freeing cn_resp

Posted by Tom Rix 4 years, 5 months ago

On 1/16/22 7:47 AM, Trond Myklebust wrote:
> Hi Tom,
>
> On Sun, 2022-01-16 at 06:43 -0800, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> nfs42_files_from_same_server() is called to check if freeing
>> cn_resp is required.  Instead of calling a function, check
>> the pointer.
>>
>> Signed-off-by: Tom Rix <trix@redhat.com>
>> ---
>>   fs/nfs/nfs4file.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>> index e79ae4cbc395e..677631ea4cfb3 100644
>> --- a/fs/nfs/nfs4file.c
>> +++ b/fs/nfs/nfs4file.c
>> @@ -180,7 +180,7 @@ static ssize_t __nfs4_copy_file_range(struct file
>> *file_in, loff_t pos_in,
>>          ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out,
>> count,
>>                                  nss, cnrs, sync);
>>   out:
>> -       if (!nfs42_files_from_same_server(file_in, file_out))
>> +       if (cn_resp)
>>                  kfree(cn_resp);
> The kernel convention in these circumstances is to just skip the NULL
> pointer check, since kfree() does that anyway.

Yes. I'll respin.

Tom

>
>>          if (ret == -EAGAIN)
>>                  goto retry;