Re: ext3/jbd oops in journal_start

[Dmitry Monakhov]

2009/11/3 Sage Weil <sage@xxxxxxxxxxxx>:
> On Sat, 31 Oct 2009, Dmitry Monakhov wrote:
>
>> Sage Weil <sage@xxxxxxxxxxxx> writes:
>>
>> > Hi,
>> >
>> > I'm consistently seeing ext3 oops on a fresh ~60 GB fs on 2.6.32-rc3 (and
>> > 2.6.31). Âdata=writeback or data=ordered. ÂIt's not the hardware or
>> > drive... I have 8 boxes (each with slightly different hardware) that crash
>> > identically.
>> Strange, 2.6.31 with ext3 is quite popular configuration...
>> Can you please post exact test-case.
>> >
>> > The oops is at fs/jbd/transaction.c, journal_start():
>> >
>> > Â Â Â Â Â Â J_ASSERT(handle->h_transaction->t_journal == journal);
>> *handle = journal_current_handle()
>>
>> IMHO it's looks like you have entered here with current->journal_info != NULL
>>
>> , but journal_info contains unexpected data
>> This may happens in two cases:
>> 1) calling jbd code from other filesystem.
>> 2) Some fs forget to zero current->journal_info on exit from vfs
>> According to call trace we have got second case. Do you use some
>> unusual/experimental fs?
>
> Yep, it was #2. ÂIt turns out btrfs s setting current->journal_info
> (for no reason that I can see?), and with the transaction ioctl a
> transaction can span multiple calls.
Whait a minute. Thats ioctl totally wrong! How it can prevent from problems?
issues:
1) If we are using this thechnic to preserving atomic behaviour, but
process dies unexpectedly in the midle of complex
operation which  must be done attomically. Then we call trans_end
which result in committing some internal file state.

2) What happens when second process try to touch file with opened
transaction? what behavuour expected from fsync?
process 1                                    process 2
1 fd = open("./file)                         open("./file")
2 ioctl_start_trans
3 write(fd, "a", 1)                           write(fd,"b", 1)
4                                                 fsync
5
What data do you expect after state (4), and after state (5)
>
> Chris, is it ok to just remove the journal_info bits? ÂNothing in fs/btrfs
> even looks at it. ÂI'm not sure what the point of only conditionally
> setting/clearly journal_info would be either, unless it's for debugging or
> something?
>
> Thanks-
> sage
>
> ---
> From: Sage Weil <sage@xxxxxxxxxxxx>
> Date: Mon, 2 Nov 2009 14:21:29 -0800
> Subject: [PATCH] Btrfs: don't set current->journal_info
>
> Btrfs doesn't use current->journal_info for anything, so don't set it.
> We currently cause a NULL dereference in jbd if a process starts a btrfs
> user transaction and then touches another mounted fs that uses jbd, since
> current->journal_info is only supposed to be set for the duration of a
> single call into the fs.

NAK The root of bad thing is ioctl patch.
If you really want to design external(from kernel point of view)
atomic interface then you have to use another
locking mechanism not transaction. Some thing like lock_flag. And it
must be stored on inode in order to provide
mutal exclusion for concurent task. Otherwise this mechanism is useless.
This patch just hide problem under a carpet. Actually setting
current->journal_info
is a good way to catch  fs to fs switching.
>
> Signed-off-by: Sage Weil <sage@xxxxxxxxxxxx>
> ---
> Âfs/btrfs/transaction.c | Â Â8 --------
> Â1 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index bca82a4..c6dbbb8 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -186,9 +186,6 @@ static struct btrfs_trans_handle *start_transaction(struct btrfs_root *root,
> Â Â Â Âh->alloc_exclude_start = 0;
> Â Â Â Âh->delayed_ref_updates = 0;
>
> - Â Â Â if (!current->journal_info)
> - Â Â Â Â Â Â Â current->journal_info = h;
> -
> Â Â Â Âroot->fs_info->running_transaction->use_count++;
> Â Â Â Ârecord_root_in_trans(h, root);
> Â Â Â Âmutex_unlock(&root->fs_info->trans_mutex);
> @@ -321,8 +318,6 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
> Â Â Â Âput_transaction(cur_trans);
> Â Â Â Âmutex_unlock(&info->trans_mutex);
>
> - Â Â Â if (current->journal_info == trans)
> - Â Â Â Â Â Â Â current->journal_info = NULL;
> Â Â Â Âmemset(trans, 0, sizeof(*trans));
> Â Â Â Âkmem_cache_free(btrfs_trans_handle_cachep, trans);
>
> @@ -1105,9 +1100,6 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans,
>
> Â Â Â Âmutex_unlock(&root->fs_info->trans_mutex);
>
> - Â Â Â if (current->journal_info == trans)
> - Â Â Â Â Â Â Â current->journal_info = NULL;
> -
> Â Â Â Âkmem_cache_free(btrfs_trans_handle_cachep, trans);
> Â Â Â Âreturn ret;
> Â}
> --
> 1.5.6.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/