lkml.org 
[lkml]   [2001]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2005-03-22 13:13    [W:3.138 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site