[PATCH] JBD code path (kfree cleanup)

[PATCH] JBD code path (kfree cleanup)

[Zwane Mwaikambo]

The intent of this patch is to remove unecessary if (foo) kfree(foo)
checks and also a slight change in the code path. This is a small part of
my i-spent-friday-night-at-home-grepping-for-kfree patch to cleanup kfree
usage.

Please comment on the code path change, it seems sane to me.

Regards,
	Zwane Mwaikambo

diffed against 2.5.1-pre4

diff -urN linux-2.5.1-pre4.orig/fs/jbd/commit.c linux-2.5.1-pre4.kfree/fs/jbd/commit.c
--- linux-2.5.1-pre4.orig/fs/jbd/commit.c	Sat Nov 10 00:25:04 2001
+++ linux-2.5.1-pre4.kfree/fs/jbd/commit.c	Fri Nov 30 23:08:58 2001
@@ -619,17 +619,15 @@
 		 *
 		 * Otherwise, we can just throw away the frozen data now.
 		 */
-		if (jh->b_committed_data) {
-			kfree(jh->b_committed_data);
-			jh->b_committed_data = NULL;
-			if (jh->b_frozen_data) {
-				jh->b_committed_data = jh->b_frozen_data;
-				jh->b_frozen_data = NULL;
-			}
-		} else if (jh->b_frozen_data) {
+		kfree(jh->b_committed_data);
+		jh->b_committed_data = NULL;
+
+		if (jh->b_frozen_data)
+			jh->b_committed_data = jh->b_frozen_data;
+		else
 			kfree(jh->b_frozen_data);
-			jh->b_frozen_data = NULL;
-		}
+
+		jh->b_frozen_data = NULL;

 		spin_lock(&journal_datalist_lock);
 		cp_transaction = jh->b_cp_transaction;
diff -urN linux-2.5.1-pre4.orig/fs/jbd/transaction.c linux-2.5.1-pre4.kfree/fs/jbd/transaction.c
--- linux-2.5.1-pre4.orig/fs/jbd/transaction.c	Sat Nov 10 00:25:04 2001
+++ linux-2.5.1-pre4.kfree/fs/jbd/transaction.c	Fri Nov 30 23:29:20 2001
@@ -739,8 +739,7 @@
 	journal_cancel_revoke(handle, jh);

 out_unlocked:
-	if (frozen_buffer)
-		kfree(frozen_buffer);
+	kfree(frozen_buffer);

 	JBUFFER_TRACE(jh, "exit");
 	return error;

Re: [PATCH] JBD code path (kfree cleanup)

[Stephen C. Tweedie]

Him

On Sat, Dec 01, 2001 at 01:28:56PM +0200, Zwane Mwaikambo wrote:

> Please comment on the code path change, it seems sane to me.

No, it is broken.  Even a brief read of the comment above
this code would have revealed that:

		/*
		 * If there is undo-protected committed data against
		 * this buffer, then we can remove it now.  If it is a
		 * buffer needing such protection, the old frozen_data
		 * field now points to a committed version of the
		 * buffer, so rotate that field to the new committed
		 * data.

The old code you replaced was:

> diff -urN linux-2.5.1-pre4.orig/fs/jbd/commit.c linux-2.5.1-pre4.kfree/fs/jbd/commit.c
> --- linux-2.5.1-pre4.orig/fs/jbd/commit.c	Sat Nov 10 00:25:04 2001
> +++ linux-2.5.1-pre4.kfree/fs/jbd/commit.c	Fri Nov 30 23:08:58 2001
> @@ -619,17 +619,15 @@
>  		 *
>  		 * Otherwise, we can just throw away the frozen data now.
>  		 */
> -		if (jh->b_committed_data) {
> -			kfree(jh->b_committed_data);
> -			jh->b_committed_data = NULL;
> -			if (jh->b_frozen_data) {
> -				jh->b_committed_data = jh->b_frozen_data;
> -				jh->b_frozen_data = NULL;
> -			}

and this version  has the intended effect of replacing any existing
committed_data field with the current frozen_data field, keeping the
contents of committed_data valid.  Your new code

> +		kfree(jh->b_committed_data);
> +		jh->b_committed_data = NULL;
> +
> +		if (jh->b_frozen_data)
> +			jh->b_committed_data = jh->b_frozen_data;
> +		else
>  			kfree(jh->b_frozen_data);
> +
> +		jh->b_frozen_data = NULL;

will discard the state of committed_data entirely, and will assign any
existing frozen_data to the new committed_data field even if
committed_data was previously NULL.  That is *definitely* not the
correct behaviour.  It won't actually corrupt anything but it will
leak memory, as you'll end up creating committed_data copies of every
metadata buffer in the system, instead of this being done only for
those block bitmap buffers which need it.

Cheers,
 Stephen

Re: [PATCH] JBD code path (kfree cleanup)

[Zwane Mwaikambo]

Thanks i'll remove that bit out, i wasn't entirely sure as i'm not that
familiar with that bit of code.

Cheers,
	Zwane Mwaikambo

Re: [PATCH] JBD code path (kfree cleanup)

[Bernd Eckenfels]

In article <Pine.LNX.4.33.0112011817370.12820-100000@netfinity.realnet.co.sz> you wrote:
> Thanks i'll remove that bit out, i wasn't entirely sure as i'm not that
> familiar with that bit of code.

So why do you patch it?

Greetings
Bernd

Re: [PATCH] JBD code path (kfree cleanup)

[Stephen C. Tweedie]

On Sat, Dec 01, 2001 at 06:24:12PM +0200, Zwane Mwaikambo wrote:
> Thanks i'll remove that bit out, i wasn't entirely sure as i'm not that
> familiar with that bit of code.

Yep, but when you are making purely cosmetic changes to code you don't
know well, you really have to be careful not to change the logic at
all.

Cheers,
 Stephen

Re: [PATCH] JBD code path (kfree cleanup)

[Zwane Mwaikambo]

Bernd Eckenfels:
>So why do you patch it?
>
>Greetings
>Bernd

I wasn't sure, hence the reason why i sent a seperate email to Stephen
Tweedie to verify with.

Cheers,
	Zwane Mwaikambo