Bug in data journaling patch ?!

Bug in data journaling patch ?!

[Vijayan Prabhakaran]

Hi,

I'm using Reiserfs on Linux 2.4.25 with the data journaling patch from
 ftp://ftp.suse.com/pub/people/mason/patches/data-logging/2.4.25/.

I came across the following aberrant behavior and further
investigation led me to believe that there could be a bug in the data
journaling patch.

I ran a sequential-write workload that wrote 1 MB of data and finally
issued a fsync. I collected the block level trace and found that the
fsync() call returned _before_ flushing the journal data. This
happened in data journaling mode. The same behavior occured in ordered
journaling mode also. That is, the fsync call returned even before any
of the data was written.

I looked at the code and I guess this was caused by the data journaling patch.

Bug description:
----------------

In journal.c there is a function called do_journal_end(). There is
line in that function that initializes commit_trans_id. It looks like:

commit_trans_id = jl->j_trans_id;

The value of jl->j_trans_id was 0 (this could be due to some memset()).

Because commit_trans_id was 0, a later "if" condition failed. And, as
a result, the journal data didn't get flushed to disk. The "if"
condition looks like:

if (journal_list_still_alive(p_s_sb, commit_trans_id))
          flush_commit_list(p_s_sb, jl, 1) ;


Bug fix:
--------

I changed the commit_trans_id initialization to the following and the
code worked fine.

commit_trans_id = SB_JOURNAL(p_s_sb)->j_trans_id ;


I'd greatly appreciate if someone can see if this really is a bug and
if the fix is appropriate.

Thanks,
Vijayan

Re: Bug in data journaling patch ?!

[Chris Mason]

On Mon, 2004-08-16 at 11:47, Vijayan Prabhakaran wrote:

> Bug description:
> ------------------
> In journal.c there is a function called do_journal_end(). There is
> line in that function that initializes commit_trans_id. It looks like:
> 
> commit_trans_id = jl->j_trans_id;
> 
> The value of jl->j_trans_id was 0 (this could be due to some memset()).
> 

This would indeed be a bug, but it's not clear how jl->j_trans_id
becomes zero.

A little farther down in do_journal_end, we set
SB_JOURNAL()->j_current_jl->j_trans_id to the transaction id of the next
transaction.  It should be impossible for it to ever be zero.

The only exception looks like the very first transaction for each
mount.  The journal_init function doesn't set j_current_jl->j_trans_id
to a non-zero value.

Can you confirm that you're only seeing this problem on the first
transaction?

-chris

Re: Bug in data journaling patch ?!

[Vijayan Prabhakaran]

Yes, this happens only on the first transaction after the mount. For
all other later transactions it works fine.

thanks,
vijayan

On Mon, 16 Aug 2004 12:07:59 -0400, Chris Mason <mason@suse.com> wrote:
> On Mon, 2004-08-16 at 11:47, Vijayan Prabhakaran wrote:
> 
> > Bug description:
> > ------------------
> > In journal.c there is a function called do_journal_end(). There is
> > line in that function that initializes commit_trans_id. It looks like:
> >
> > commit_trans_id = jl->j_trans_id;
> >
> > The value of jl->j_trans_id was 0 (this could be due to some memset()).
> >
> 
> This would indeed be a bug, but it's not clear how jl->j_trans_id
> becomes zero.
> 
> A little farther down in do_journal_end, we set
> SB_JOURNAL()->j_current_jl->j_trans_id to the transaction id of the next
> transaction.  It should be impossible for it to ever be zero.
> 
> The only exception looks like the very first transaction for each
> mount.  The journal_init function doesn't set j_current_jl->j_trans_id
> to a non-zero value.
> 
> Can you confirm that you're only seeing this problem on the first
> transaction?
> 
> -chris
> 
>

Re: Bug in data journaling patch ?!

[Chris Mason]

On Mon, 2004-08-16 at 12:23, Vijayan Prabhakaran wrote:
> Yes, this happens only on the first transaction after the mount. For
> all other later transactions it works fine.

Perfect, thanks for the verification, I'll cook up patches for both
2.6.x and 2.4.x.

-chris