| Message ID | 20120404075520.GA5725@quack.suse.cz |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Apr 04, 2012 at 09:55:20AM +0200, Jan Kara wrote: > On Tue 10-01-12 13:12:55, Josef Bacik wrote: > > If we are journalling data (ie journal=data or big symlinks) we can discard > > buffers and move them to different transactions to make sure they get cleaned up > > properly. The problem is b_modified could still be set from the last > > transaction that touched it, so putting it on the currently running transaction > > or setting it up to be put on the next transaction will run into problems if the > > buffer gets reused in that transaction as the space accounting logic won't be > > done, which will result in panics at commit time because t_nr_buffers will end > > up being more than t_outstanding_credits. Thanks to Jan Kara for pointing out > > the other part of this problem a few months ago. Thanks, > > > > Signed-off-by: Josef Bacik <josef@redhat.com> > So I think I've nailed this down. Your feeling that the problem is with > refiling buffer to BJ_Forget list of the running transaction was right. The > missing piece to the puzzle was that journal_invalidatepage() can get > called not only when underlying block is freed but also when someone > flushes page cache. The traces I have suggest that someone has flushed page > cache (likely of the block device), that moved buffer from the checkpoint > list to BJ_Forget list of the running transaction and then the same running > transaction tried to modify the buffer which triggered the accounting > problem you spotted. > > I have updated the changelog and pushed the patch to my tree (for JBD > only). I'll duplicate the patch for JBD2 tomorrow. > Ok now it's my turn to be unsure ;). I thought invalidatepage could only be called via truncate? You say it happens when someone flushes pagecache, do you mean like echo 3 > /proc/sys/vm/drop_caches? I've followed invalidatepage and can't see what you are talking about, so as usual I need it explained to me because I'm stupid. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 04-04-12 12:46:57, Josef Bacik wrote: > On Wed, Apr 04, 2012 at 09:55:20AM +0200, Jan Kara wrote: > > On Tue 10-01-12 13:12:55, Josef Bacik wrote: > > > If we are journalling data (ie journal=data or big symlinks) we can discard > > > buffers and move them to different transactions to make sure they get cleaned up > > > properly. The problem is b_modified could still be set from the last > > > transaction that touched it, so putting it on the currently running transaction > > > or setting it up to be put on the next transaction will run into problems if the > > > buffer gets reused in that transaction as the space accounting logic won't be > > > done, which will result in panics at commit time because t_nr_buffers will end > > > up being more than t_outstanding_credits. Thanks to Jan Kara for pointing out > > > the other part of this problem a few months ago. Thanks, > > > > > > Signed-off-by: Josef Bacik <josef@redhat.com> > > So I think I've nailed this down. Your feeling that the problem is with > > refiling buffer to BJ_Forget list of the running transaction was right. The > > missing piece to the puzzle was that journal_invalidatepage() can get > > called not only when underlying block is freed but also when someone > > flushes page cache. The traces I have suggest that someone has flushed page > > cache (likely of the block device), that moved buffer from the checkpoint > > list to BJ_Forget list of the running transaction and then the same running > > transaction tried to modify the buffer which triggered the accounting > > problem you spotted. > > > > I have updated the changelog and pushed the patch to my tree (for JBD > > only). I'll duplicate the patch for JBD2 tomorrow. > > > > Ok now it's my turn to be unsure ;). I thought invalidatepage could only be > called via truncate? You say it happens when someone flushes pagecache, do you > mean like echo 3 > /proc/sys/vm/drop_caches? Yup, or things like BLKFLSBUF ioctl. But yes, you are right they don't end up calling ext3_invalidatepage() I often get confused by the name of invalidate_mapping_pages()... Anyway ext3_invalidatepage() definitely gets called (I see that in my traces) and now I tend to thing it's from ext3_evict_inode(). The guy was using 2.6.37 kernel which doesn't have b22570d9abb3d844e65c15c8bc0d57a78129e3b4 so truncate_inode_pages() gets called from ext3_evict_inode() before the buffer is checkpointed and that causes the described scenario. But the guy claims he's seen the problem with 3.2 as well. So I guess I'll forward-port the buffer tracking patches and ask him to reproduce with 3.2. Honza
On Wed, Apr 04, 2012 at 11:14:44PM +0200, Jan Kara wrote: > On Wed 04-04-12 12:46:57, Josef Bacik wrote: > > On Wed, Apr 04, 2012 at 09:55:20AM +0200, Jan Kara wrote: > > > On Tue 10-01-12 13:12:55, Josef Bacik wrote: > > > > If we are journalling data (ie journal=data or big symlinks) we can discard > > > > buffers and move them to different transactions to make sure they get cleaned up > > > > properly. The problem is b_modified could still be set from the last > > > > transaction that touched it, so putting it on the currently running transaction > > > > or setting it up to be put on the next transaction will run into problems if the > > > > buffer gets reused in that transaction as the space accounting logic won't be > > > > done, which will result in panics at commit time because t_nr_buffers will end > > > > up being more than t_outstanding_credits. Thanks to Jan Kara for pointing out > > > > the other part of this problem a few months ago. Thanks, > > > > > > > > Signed-off-by: Josef Bacik <josef@redhat.com> > > > So I think I've nailed this down. Your feeling that the problem is with > > > refiling buffer to BJ_Forget list of the running transaction was right. The > > > missing piece to the puzzle was that journal_invalidatepage() can get > > > called not only when underlying block is freed but also when someone > > > flushes page cache. The traces I have suggest that someone has flushed page > > > cache (likely of the block device), that moved buffer from the checkpoint > > > list to BJ_Forget list of the running transaction and then the same running > > > transaction tried to modify the buffer which triggered the accounting > > > problem you spotted. > > > > > > I have updated the changelog and pushed the patch to my tree (for JBD > > > only). I'll duplicate the patch for JBD2 tomorrow. > > > > > > > Ok now it's my turn to be unsure ;). I thought invalidatepage could only be > > called via truncate? You say it happens when someone flushes pagecache, do you > > mean like echo 3 > /proc/sys/vm/drop_caches? > Yup, or things like BLKFLSBUF ioctl. But yes, you are right they don't > end up calling ext3_invalidatepage() I often get confused by the name of > invalidate_mapping_pages()... Anyway ext3_invalidatepage() definitely gets > called (I see that in my traces) and now I tend to thing it's from > ext3_evict_inode(). The guy was using 2.6.37 kernel which doesn't have > b22570d9abb3d844e65c15c8bc0d57a78129e3b4 so truncate_inode_pages() gets > called from ext3_evict_inode() before the buffer is checkpointed and that > causes the described scenario. But the guy claims he's seen the problem > with 3.2 as well. So I guess I'll forward-port the buffer tracking patches > and ask him to reproduce with 3.2. > Ah yeah and my reports are from RHEL5 which calls truncate_inode_pages from generic_forget_inode, so that makes sense, but yeah why it would happen on newer stuff is weird. Let me know how that works out ;). If anything the patch is obviously correct, I'm ok with patch and praying. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From d433e0479c9cde46b29b30a5c5996c1dbe57005f Mon Sep 17 00:00:00 2001 From: Josef Bacik <josef@redhat.com> Date: Tue, 10 Jan 2012 13:12:55 -0500 Subject: [PATCH] jbd: clear b_modified before moving the jh to a different transaction journal_forget() and journal_invalidatepage() functions move buffer to BJ_Forget list of a running transaction so that the buffer gets cleaned up when the transaction is committed. This usually happens when underlying block is freed but journal_invalidatepage() can also move the buffer when page cache of the corresponding inode (may be a block device) gets flushed. When the buffer had b_modfied set from the previous transaction and we happen to modify it again in the current transaction, we won't properly account for the modified buffer by subtracting the number of reserved credits of the running transaction because do_get_write_access() won't clear b_modified (buffer already is on running transaction so do_get_write_access() things it has nothing to do). This then results in assertion failure in commit code because the transaction has more buffers than reserved credits (t_nr_buffers > t_outstanding_credits). We fix the issue by clearing b_modified before moving buffer to a BJ_Forget list of another transaction because logically, it's not changed for that transaction anymore. CC: stable@kernel.org Signed-off-by: Josef Bacik <josef@redhat.com> Signed-off-by: Jan Kara <jack@suse.cz> --- fs/jbd/transaction.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c index febc10d..fb48e44 100644 --- a/fs/jbd/transaction.c +++ b/fs/jbd/transaction.c @@ -1788,6 +1788,7 @@ static int __dispose_buffer(struct journal_head *jh, transaction_t *transaction) */ clear_buffer_dirty(bh); __journal_file_buffer(jh, transaction, BJ_Forget); + jh->b_modified = 0; may_free = 0; } else { JBUFFER_TRACE(jh, "on running transaction"); @@ -1956,8 +1957,10 @@ static int journal_unmap_buffer(journal_t *journal, struct buffer_head *bh) * clear dirty bits when it is done with the buffer. */ set_buffer_freed(bh); - if (journal->j_running_transaction && buffer_jbddirty(bh)) + if (journal->j_running_transaction && buffer_jbddirty(bh)) { + jh->b_modified = 0; jh->b_next_transaction = journal->j_running_transaction; + } journal_put_journal_head(jh); spin_unlock(&journal->j_list_lock); jbd_unlock_bh_state(bh); -- 1.7.1