Re: ext4: Can we talk about bforget() and metadata blocks

[Curt Wohlgemuth <curtw@google.com>]

On Wed, Sep 9, 2009 at 6:35 PM, Theodore Tso<tytso@mit.edu> wrote:
> On Wed, Sep 09, 2009 at 05:07:28PM -0700, Curt Wohlgemuth wrote:
>>
>> First, ext4_journal_forget() is called from ext4_forget() only when
>> we're journalling; without a journal, ext4_journal_forget() is only
>> called for various non-extent paths.  ext4_forget() could be changed,
>> of course...
>
> Ext4_forget() calls either ext4_journal_forget() or
> ext4_journal_revoke().  So we need to fix up both functions.
>
>                                          - Ted
>
> commit 4afdf0958f6f7b878e6d85cb4e0c0c12a0bd74e2
> Author: Theodore Ts'o <tytso@mit.edu>
> Date:   Wed Sep 9 21:32:41 2009 -0400
>
>    ext4: Use bforget() in no journal mode for ext4_journal_{forget,revoke}()
>
>    When ext4 is using a journal, a metadata block which is deallocated
>    must be passed into the journal layer so it can be dropped from the
>    current transaction and/or revoked.  This is done by calling the
>    functions ext4_journal_forget() and ext4_journal_revoke(), which call
>    jbd2_journal_forget(), and jbd2_journal_revoke(), respectively.
>
>    Since the jbd2_journal_forget() and jbd2_journal_revoke() call
>    bforget(), if ext4 is not using a journal, ext4_journal_forget() and
>    ext4_journal_revoke() must call bforget() to avoid a dirty metadata
>    block overwriting a block after it has been reallocated and reused for
>    another inode's data block.
>
>    Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
>
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index eb27fd0..ecb9ca4 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -44,7 +44,7 @@ int __ext4_journal_forget(const char *where, handle_t *handle,
>                                                  handle, err);
>        }
>        else
> -               brelse(bh);
> +               bforget(bh);
>        return err;
>  }
>
> @@ -60,7 +60,7 @@ int __ext4_journal_revoke(const char *where, handle_t *handle,
>                                                  handle, err);
>        }
>        else
> -               brelse(bh);
> +               bforget(bh);
>        return err;
>  }

This looks quite reasonable to me.  And of course, your patch fixes
the brelse() calls that I added back in July.

Of course, validating this patch on my end will be tough; I need at
least a week of no corruptions to convince myself that the problem is
fixed, to the same degree that my call to sync_dirty_buffer() works.

Thanks,
Curt
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html