2008-01-23 09:07:53

by Abhishek Rai

Subject: Confused by journaling code in ext3_new_blocks()

Hi,

I have the following question related to the journaling code in
ext3_new_blocks() function of fs/ext3/balloc.c, any help in this
regard will be greatly appreciated. The code snippet below is taken
from 2.6.24-rc8-mm1 but this code has changed little for a long time
so it's likely that any other recent kernel will also have very
similar code.

ext3_new_blocks()
-> ext3_try_to_allocate_with_rsv()
-> ext3_journal_dirty_metadata(handle, bitmap_bh)
...
allocated:
if ( /*sanity checking of newly allocated block numbers*/) {
ext3_error(...);
goto out;
}

out:
...
brelse(bitmap_bh);
return 0;


In the above code snippet, ext3_try_to_allocate_with_rsv() is marking
bitmap_bh as dirty metadata _before_ the sanity checks. I'm not
worried about the sanity checks as such, but it seems to me that if
the checks were to fail, we'd be committing the bitmap_bh updates to
disk even though it violates metadata sanity, or am I missing
something here ?

Thanks,
Abhishek

2008-01-23 20:20:46

by Jan Kara

Subject: Re: Confused by journaling code in ext3_new_blocks()

Hello,

> I have the following question related to the journaling code in
> ext3_new_blocks() function of fs/ext3/balloc.c, any help in this
> regard will be greatly appreciated. The code snippet below is taken
> from 2.6.24-rc8-mm1 but this code has changed little for a long time
> so it's likely that any other recent kernel will also have very
> similar code.
>
> ext3_new_blocks()
> -> ext3_try_to_allocate_with_rsv()
> -> ext3_journal_dirty_metadata(handle, bitmap_bh)
> ...
> allocated:
> if ( /*sanity checking of newly allocated block numbers*/) {
> ext3_error(...);
> goto out;
> }
>
> out:
> ...
> brelse(bitmap_bh);
> return 0;
>
>
> In the above code snippet, ext3_try_to_allocate_with_rsv() is marking
> bitmap_bh as dirty metadata _before_ the sanity checks. I'm not
> worried about the sanity checks as such, but it seems to me that if
> the checks were to fail, we'd be committing the bitmap_bh updates to
> disk even though it violates metadata sanity, or am I missing
> something here ?
Actually, it depends. Because if sanity checks fail, we call
ext3_error() which remounts fs R/O or panics in most cases. Only if
errors=continue is set, we really get to committing the wrong bitmap
data. But I don't think this really is a problem - fsck has to be run
anyway and it rebuilds the bitmaps from scratch.

Honza
--
Jan Kara <[email protected]>
SuSE CR Labs