* [PATCH] JBD code path (kfree cleanup)
@ 2001-12-01 11:28 Zwane Mwaikambo
2001-12-01 16:08 ` Stephen C. Tweedie
0 siblings, 1 reply; 6+ messages in thread
From: Zwane Mwaikambo @ 2001-12-01 11:28 UTC (permalink / raw)
To: sct; +Cc: Linux Kernel
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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] JBD code path (kfree cleanup)
2001-12-01 11:28 [PATCH] JBD code path (kfree cleanup) Zwane Mwaikambo
@ 2001-12-01 16:08 ` Stephen C. Tweedie
2001-12-01 16:24 ` Zwane Mwaikambo
0 siblings, 1 reply; 6+ messages in thread
From: Stephen C. Tweedie @ 2001-12-01 16:08 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: sct, Linux Kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] JBD code path (kfree cleanup)
2001-12-01 16:08 ` Stephen C. Tweedie
@ 2001-12-01 16:24 ` Zwane Mwaikambo
2001-12-02 13:59 ` Bernd Eckenfels
2001-12-03 10:38 ` Stephen C. Tweedie
0 siblings, 2 replies; 6+ messages in thread
From: Zwane Mwaikambo @ 2001-12-01 16:24 UTC (permalink / raw)
To: Stephen C. Tweedie; +Cc: Linux Kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] JBD code path (kfree cleanup)
2001-12-01 16:24 ` Zwane Mwaikambo
@ 2001-12-02 13:59 ` Bernd Eckenfels
2001-12-03 10:38 ` Stephen C. Tweedie
1 sibling, 0 replies; 6+ messages in thread
From: Bernd Eckenfels @ 2001-12-02 13:59 UTC (permalink / raw)
To: linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] JBD code path (kfree cleanup)
2001-12-01 16:24 ` Zwane Mwaikambo
2001-12-02 13:59 ` Bernd Eckenfels
@ 2001-12-03 10:38 ` Stephen C. Tweedie
1 sibling, 0 replies; 6+ messages in thread
From: Stephen C. Tweedie @ 2001-12-03 10:38 UTC (permalink / raw)
To: Zwane Mwaikambo; +Cc: Stephen C. Tweedie, Linux Kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] JBD code path (kfree cleanup)
@ 2001-12-03 15:22 Zwane Mwaikambo
0 siblings, 0 replies; 6+ messages in thread
From: Zwane Mwaikambo @ 2001-12-03 15:22 UTC (permalink / raw)
To: ecki; +Cc: Linux Kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2001-12-04 3:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2001-12-01 11:28 [PATCH] JBD code path (kfree cleanup) Zwane Mwaikambo
2001-12-01 16:08 ` Stephen C. Tweedie
2001-12-01 16:24 ` Zwane Mwaikambo
2001-12-02 13:59 ` Bernd Eckenfels
2001-12-03 10:38 ` Stephen C. Tweedie
-- strict thread matches above, loose matches on Subject: below --
2001-12-03 15:22 Zwane Mwaikambo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox