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;
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
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
In article <[email protected]> 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
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
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