On Sat, 31 Oct 2009, Dmitry Monakhov wrote:> Sage Weil <sage@newdream.net> 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. 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@newdream.net> 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. Signed-off-by: Sage Weil <sage@newdream.net> --- 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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2009/11/3 Sage Weil <sage@newdream.net>:> On Sat, 31 Oct 2009, Dmitry Monakhov wrote: > >> Sage Weil <sage@newdream.net> 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@newdream.net> > 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@newdream.net> > --- > 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-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 02, 2009 at 10:06:36PM -0800, Sage Weil wrote:> On Sat, 31 Oct 2009, Dmitry Monakhov wrote: > > > Sage Weil <sage@newdream.net> 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. > > 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?Josef has plans to use it later on, but he sent along patches that will avoid setting journal_info for userland trans. I''ll get these integrated and pushed out. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html