On Wed 25-03-09 20:07:46, Alexander Beregalov wrote: > 2009/3/25 Jan Kara <jack@xxxxxxx>: > > On Wed 25-03-09 18:29:10, Alexander Beregalov wrote: > >> 2009/3/25 Jan Kara <jack@xxxxxxx>: > >> > On Wed 25-03-09 18:18:43, Alexander Beregalov wrote: > >> >> 2009/3/25 Jan Kara <jack@xxxxxxx>: > >> >> >> > So, I think I need to try it on 2.6.29-rc7 again. > >> >> >> I've looked into this. Obviously, what's happenning is that we delete > >> >> >> an inode and jbd2_journal_release_jbd_inode() finds inode is just under > >> >> >> writeout in transaction commit and thus it waits. But it gets never woken > >> >> >> up and because it has a handle from the transaction, every one eventually > >> >> >> blocks on waiting for a transaction to finish. > >> >> >> But I don't really see how that can happen. The code is really > >> >> >> straightforward and everything happens under j_list_lock... Strange. > >> >> > BTW: Is the system SMP? > >> >> No, it is UP system. > >> > Even stranger. And do you have CONFIG_PREEMPT set? > >> > > >> >> The bug exists even in 2.6.29, I posted it with a new topic. > >> > OK, I've sort-of expected this. > >> > >> CONFIG_PREEMPT_RCU=y > >> CONFIG_PREEMPT_RCU_TRACE=y > >> # CONFIG_PREEMPT_NONE is not set > >> # CONFIG_PREEMPT_VOLUNTARY is not set > >> CONFIG_PREEMPT=y > >> CONFIG_DEBUG_PREEMPT=y > >> # CONFIG_PREEMPT_TRACER is not set > >> > >> config is attached. > > Thanks for the data. I still don't see how the wakeup can get lost. The > > process even cannot be preempted when we are in the section protected by > > j_list_lock... Can you send me a disassembly of functions > > jbd2_journal_release_jbd_inode() and journal_submit_data_buffers() so that > > I can see whether the compiler has not reordered something unexpectedly? Thanks for the disassembly... > By default gcc inlines journal_submit_data_buffers() > Here is -fno-inline version. Default version is in attach. > ==== > > static int journal_submit_data_buffers(journal_t *journal, > transaction_t *commit_transaction) > { > 9c: 9d e3 bf 40 save %sp, -192, %sp > a0: 11 00 00 00 sethi %hi(0), %o0 > struct jbd2_inode *jinode; > int err, ret = 0; > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > a4: a4 06 25 70 add %i0, 0x570, %l2 > * our inode list. We use JI_COMMIT_RUNNING flag to protect inode we currently > * operate on from being released while we write out pages. > */ > static int journal_submit_data_buffers(journal_t *journal, > transaction_t *commit_transaction) > { > a8: 90 12 20 00 mov %o0, %o0 > ac: 40 00 00 00 call ac <journal_submit_data_buffers+0x10> > b0: b0 10 20 00 clr %i0 > struct jbd2_inode *jinode; > int err, ret = 0; > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) { > b4: a6 06 60 60 add %i1, 0x60, %l3 > { > struct jbd2_inode *jinode; > int err, ret = 0; > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > b8: 40 00 00 00 call b8 <journal_submit_data_buffers+0x1c> > bc: 90 10 00 12 mov %l2, %o0 > list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) { > c0: 10 68 00 1d b %xcc, 134 <journal_submit_data_buffers+0x98> > c4: c2 5e 60 60 ldx [ %i1 + 0x60 ], %g1 > mapping = jinode->i_vfs_inode->i_mapping; > jinode->i_flags |= JI_COMMIT_RUNNING; > spin_unlock(&journal->j_list_lock); > c8: 90 10 00 12 mov %l2, %o0 > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) { > mapping = jinode->i_vfs_inode->i_mapping; > jinode->i_flags |= JI_COMMIT_RUNNING; > cc: c2 04 60 28 ld [ %l1 + 0x28 ], %g1 Here we load jbd2_inode->i_flags into %g1. > int err, ret = 0; > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) { > mapping = jinode->i_vfs_inode->i_mapping; > d0: e0 58 a1 e0 ldx [ %g2 + 0x1e0 ], %l0 > jinode->i_flags |= JI_COMMIT_RUNNING; > d4: 82 10 60 01 or %g1, 1, %g1 Here we set JI_COMMIT_RUNNING. > spin_unlock(&journal->j_list_lock); > d8: 40 00 00 00 call d8 <journal_submit_data_buffers+0x3c> Here we seem to call preempt_disable() (it would be useful if we could confirm that - easiest option I know is compiling JBD2 into a kernel but some object file trickery should be able to find it out as well...) > dc: c2 24 60 28 st %g1, [ %l1 + 0x28 ] And here we store the register back to memory - but we could be already preempted here which could cause bugs... > * submit the inode data buffers. We use writepage > * instead of writepages. Because writepages can do > * block allocation with delalloc. We need to write > * only allocated blocks here. > */ > err = journal_submit_inode_data_buffers(mapping); > e0: 7f ff ff d3 call 2c <journal_submit_inode_data_buffers> > e4: 90 10 00 10 mov %l0, %o0 > if (!ret) > e8: 80 a6 20 00 cmp %i0, 0 > ec: b1 64 40 08 move %icc, %o0, %i0 > ret = err; > spin_lock(&journal->j_list_lock); > f0: 40 00 00 00 call f0 <journal_submit_data_buffers+0x54> > f4: 90 10 00 12 mov %l2, %o0 > J_ASSERT(jinode->i_transaction == commit_transaction); > f8: c2 5c 40 00 ldx [ %l1 ], %g1 > fc: 80 a0 40 19 cmp %g1, %i1 > 100: 22 68 00 07 be,a %xcc, 11c > <journal_submit_data_buffers+0x80> > 104: c2 04 60 28 ld [ %l1 + 0x28 ], %g1 Again, here we load jinode->i_flags. > 108: 11 00 00 00 sethi %hi(0), %o0 > 10c: 92 10 21 04 mov 0x104, %o1 > 110: 40 00 00 00 call 110 <journal_submit_data_buffers+0x74> > 114: 90 12 20 00 mov %o0, %o0 > 118: 91 d0 20 05 ta 5 > jinode->i_flags &= ~JI_COMMIT_RUNNING; > wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING); > 11c: 90 04 60 28 add %l1, 0x28, %o0 > 120: 92 10 20 00 clr %o1 > err = journal_submit_inode_data_buffers(mapping); > if (!ret) > ret = err; > spin_lock(&journal->j_list_lock); > J_ASSERT(jinode->i_transaction == commit_transaction); > jinode->i_flags &= ~JI_COMMIT_RUNNING; > 124: 82 08 7f fe and %g1, -2, %g1 Here we go &= ~JI_COMMIT_RUNNING > wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING); > 128: 40 00 00 00 call 128 <journal_submit_data_buffers+0x8c> > 12c: c2 24 60 28 st %g1, [ %l1 + 0x28 ] And only here we store it back to memory... > struct jbd2_inode *jinode; > int err, ret = 0; > struct address_space *mapping; > > spin_lock(&journal->j_list_lock); > list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) { > 130: c2 5c 60 10 ldx [ %l1 + 0x10 ], %g1 > 134: a2 00 7f f0 add %g1, -16, %l1 > * prefetches into the prefetch-cache which only is accessible > * by floating point operations in UltraSPARC-III and later. > * By contrast, "#one_write" prefetches into the L2 cache > * in shared state. > */ > __asm__ __volatile__("prefetch [%0], #one_write" > 138: c2 5c 60 10 ldx [ %l1 + 0x10 ], %g1 > 13c: c7 68 40 00 prefetch [ %g1 ], #one_write > 140: 82 04 60 10 add %l1, 0x10, %g1 > 144: 80 a4 c0 01 cmp %l3, %g1 > 148: 32 6f ff e0 bne,a %xcc, c8 > <journal_submit_data_buffers+0x2c> > 14c: c4 5c 60 20 ldx [ %l1 + 0x20 ], %g2 > spin_lock(&journal->j_list_lock); > J_ASSERT(jinode->i_transaction == commit_transaction); > wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING); > } > spin_unlock(&journal->j_list_lock); > 150: 90 10 00 12 mov %l2, %o0 > 154: 40 00 00 00 call 154 <journal_submit_data_buffers+0xb8> > 158: b1 3e 20 00 sra %i0, 0, %i0 > return ret; > } > 15c: 81 cf e0 08 rett %i7 + 8 > 160: 01 00 00 00 nop So the compiled code looks a bit suspitious to me. Having the disassembly with symbols properly resolved would help confirm it. I'm adding sparc list to CC just in case someone sees the problem... Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html