Messages in this thread Patch in this message | | | Date | Sat, 1 Dec 2001 16:08:39 +0000 | From | "Stephen C. Tweedie" <> | Subject | Re: [PATCH] JBD code path (kfree cleanup) |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |