From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:8401 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754144AbaFXP0B (ORCPT ); Tue, 24 Jun 2014 11:26:01 -0400 Message-ID: <53A99873.9080007@fb.com> Date: Tue, 24 Jun 2014 08:25:39 -0700 From: Josef Bacik MIME-Version: 1.0 To: , Miao Xie CC: "linux-btrfs@vger.kernel.org" Subject: Re: [PATCH] Btrfs: fix crash when starting transaction References: <1403524739-28436-1-git-send-email-fdmanana@gmail.com> <53A9517C.9020904@cn.fujitsu.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: On 06/24/2014 03:29 AM, Filipe David Manana wrote: > On Tue, Jun 24, 2014 at 11:22 AM, Miao Xie wrote: >> CC Josef >> >> On Mon, 23 Jun 2014 12:58:59 +0100, Filipe David Borba Manana wrote: >>> Often when starting a transaction we commit the currently running transaction, >>> which can end up writing block group caches when the current process has its >>> journal_info set to NULL (and not to a transaction). This makes our assertion >>> at btrfs_check_data_free_space() (current_journal != NULL) fail, resulting >>> in a crash/hang. Therefore fix it by setting journal_info. >>> >>> Two different traces of this issue follow below. >>> >>> 1) >>> >>> [51502.241936] BTRFS: assertion failed: current->journal_info, file: fs/btrfs/extent-tree.c, line: 3670 >>> [51502.242213] ------------[ cut here ]------------ >>> [51502.242493] kernel BUG at fs/btrfs/ctree.h:3964! >>> [51502.242669] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC >>> (...) >>> [51502.244010] Call Trace: >>> [51502.244010] [] btrfs_check_data_free_space+0x395/0x3a0 [btrfs] >>> [51502.244010] [] btrfs_write_dirty_block_groups+0x4ac/0x640 [btrfs] >>> [51502.244010] [] commit_cowonly_roots+0x164/0x226 [btrfs] >>> [51502.244010] [] btrfs_commit_transaction+0x4ed/0xab0 [btrfs] >>> [51502.244010] [] ? _raw_spin_unlock+0x2b/0x40 >>> [51502.244010] [] start_transaction+0x459/0x620 [btrfs] >>> [51502.244010] [] btrfs_start_transaction+0x1b/0x20 [btrfs] >>> [51502.244010] [] __unlink_start_trans+0x31/0xe0 [btrfs] >>> [51502.244010] [] btrfs_unlink+0x37/0xc0 [btrfs] >>> [51502.244010] [] ? do_unlinkat+0x114/0x2a0 >>> [51502.244010] [] vfs_unlink+0xcc/0x150 >>> [51502.244010] [] do_unlinkat+0x260/0x2a0 >>> [51502.244010] [] ? filp_close+0x64/0x90 >>> [51502.244010] [] ? trace_hardirqs_on_caller+0x16/0x1e0 >>> [51502.244010] [] ? trace_hardirqs_on_thunk+0x3a/0x3f >>> [51502.244010] [] SyS_unlinkat+0x1b/0x40 >>> [51502.244010] [] system_call_fastpath+0x16/0x1b >>> [51502.244010] Code: 0b 55 48 89 e5 0f 0b 55 48 89 e5 0f 0b 55 89 f1 48 c7 c2 71 13 36 a0 48 89 fe 31 c0 48 c7 c7 b8 43 36 a0 48 89 e5 e8 5d b0 32 e1 <0f> 0b 0f 1f 44 00 00 55 b9 11 00 00 00 48 89 e5 41 55 49 89 f5 >>> [51502.244010] RIP [] assfail.constprop.88+0x1e/0x20 [btrfs] >>> >>> 2) >>> >>> [25405.097230] BTRFS: assertion failed: current->journal_info, file: fs/btrfs/extent-tree.c, line: 3670 >>> [25405.097488] ------------[ cut here ]------------ >>> [25405.097767] kernel BUG at fs/btrfs/ctree.h:3964! >>> [25405.097940] invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC >>> (...) >>> [25405.100008] Call Trace: >>> [25405.100008] [] btrfs_check_data_free_space+0x395/0x3a0 [btrfs] >>> [25405.100008] [] btrfs_write_dirty_block_groups+0x4ac/0x640 [btrfs] >>> [25405.100008] [] commit_cowonly_roots+0x164/0x226 [btrfs] >>> [25405.100008] [] btrfs_commit_transaction+0x4ed/0xab0 [btrfs] >>> [25405.100008] [] ? bit_waitqueue+0xc0/0xc0 >>> [25405.100008] [] start_transaction+0x459/0x620 [btrfs] >>> [25405.100008] [] btrfs_start_transaction+0x1b/0x20 [btrfs] >>> [25405.100008] [] btrfs_create+0x47/0x210 [btrfs] >>> [25405.100008] [] ? btrfs_permission+0x3c/0x80 [btrfs] >>> [25405.100008] [] vfs_create+0x9b/0x130 >>> [25405.100008] [] do_last+0x849/0xe20 >>> [25405.100008] [] ? link_path_walk+0x79/0x820 >>> [25405.100008] [] path_openat+0xc5/0x690 >>> [25405.100008] [] ? trace_hardirqs_on+0xd/0x10 >>> [25405.100008] [] ? __alloc_fd+0x32/0x1d0 >>> [25405.100008] [] do_filp_open+0x43/0xa0 >>> [25405.100008] [] ? __alloc_fd+0x151/0x1d0 >>> [25405.100008] [] do_sys_open+0x13c/0x230 >>> [25405.100008] [] ? trace_hardirqs_on_caller+0x16/0x1e0 >>> [25405.100008] [] SyS_open+0x22/0x30 >>> [25405.100008] [] system_call_fastpath+0x16/0x1b >>> [25405.100008] Code: 0b 55 48 89 e5 0f 0b 55 48 89 e5 0f 0b 55 89 f1 48 c7 c2 51 13 36 a0 48 89 fe 31 c0 48 c7 c7 d0 43 36 a0 48 89 e5 e8 6d b5 32 e1 <0f> 0b 0f 1f 44 00 00 55 b9 11 00 00 00 48 89 e5 41 55 49 89 f5 >>> [25405.100008] RIP [] assfail.constprop.88+0x1e/0x20 [btrfs] >>> >>> Signed-off-by: Filipe David Borba Manana >>> --- >>> fs/btrfs/transaction.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c >>> index ac984a3..fe4abe9 100644 >>> --- a/fs/btrfs/transaction.c >>> +++ b/fs/btrfs/transaction.c >>> @@ -491,7 +491,11 @@ again: >>> smp_mb(); >>> if (cur_trans->state >= TRANS_STATE_BLOCKED && >>> may_wait_transaction(root, type)) { >>> + void *journal_info = current->journal_info; >>> + if (!journal_info) >>> + current->journal_info = h; >>> btrfs_commit_transaction(h, root); >>> + current->journal_info = journal_info; >>> goto again; >> >> It seems it is impossible that the task who set BTRFS_SEND_TRANS_STUB would start a transaction, >> that is if current->journal_info is not NULL, we are sure there is a transaction handle in it, >> so we just join that handle and then go out. In other words, current->journal_info here must be NULL, >> so the if sentence is unnecessary, > > I think that is true too (current->journal_info == NULL here). > The if test is done mostly because it's done some lines below too. > Yeah journal_info should always == NULL here. In fact I'd actually like to put an ASSERT() to make sure we don't have TRANS_SUB set coming into start_transaction, but you can leave that for another day if you like. Good find btw, Josef