Patchwork [linux-next] jbd: start_this_handle(): remove kfree() redundant null check

login
register
mail settings
Submitter Tim Gardner
Date Feb. 12, 2013, 6:23 p.m.
Message ID <1360693404-61617-1-git-send-email-tim.gardner@canonical.com>
Download mbox | patch
Permalink /patch/219934/
State Rejected
Headers show

Comments

Tim Gardner - Feb. 12, 2013, 6:23 p.m.
smatch analysis:

fs/jbd/transaction.c:236 start_this_handle() info: redundant null
 check on new_transaction calling kfree()

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Jan Kara <jack@suse.cz>
Cc: linux-ext4@vger.kernel.org
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/jbd/transaction.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Jan Kara - Feb. 14, 2013, 4:15 p.m.
On Tue 12-02-13 11:23:24, Tim Gardner wrote:
> smatch analysis:
> 
> fs/jbd/transaction.c:236 start_this_handle() info: redundant null
>  check on new_transaction calling kfree()
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: linux-ext4@vger.kernel.org
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  fs/jbd/transaction.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
> index 071d690..ee40b4e 100644
> --- a/fs/jbd/transaction.c
> +++ b/fs/jbd/transaction.c
> @@ -232,8 +232,7 @@ repeat_locked:
>  
>  	lock_map_acquire(&handle->h_lockdep_map);
>  out:
> -	if (unlikely(new_transaction))		/* It's usually NULL */
> -		kfree(new_transaction);
> +	kfree(new_transaction);
>  	return ret;
  Thanks for the patch but I actually think this is deliberate because
new_transaction is usually NULL while kfree() is optimized for passed
pointer to be usually != NULL.  Also we save a function call in the common
case. So I'm for the code to stay as is.

								Honza

Patch

diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
index 071d690..ee40b4e 100644
--- a/fs/jbd/transaction.c
+++ b/fs/jbd/transaction.c
@@ -232,8 +232,7 @@  repeat_locked:
 
 	lock_map_acquire(&handle->h_lockdep_map);
 out:
-	if (unlikely(new_transaction))		/* It's usually NULL */
-		kfree(new_transaction);
+	kfree(new_transaction);
 	return ret;
 }