Message ID | 20081105135349.GA22998@mit.edu |
---|---|
State | Accepted, archived |
Headers | show |
Ted-san, thank you for your reading my mail. Theodore Tso wrote: > Toshiyuki-san, > > I authored a patch a few days ago which I am about to push to Linus, > since it two people who have reported this problem has confirmed that > it solves the problem for them. That patch can be found here: > > http://lkml.org/lkml/2008/11/1/61 I haven't checked it yet. So, I check it now. > As you can see, it has a rather different theory about the root cause > of the problem; but it makes sense to me, and it has empircally solved > the problem. uh-huh. > So I read your proposed description of the root cause of the problem > with interest. If I understand you correctly, your concern is that > various functions in jbd2 are cleaning up the memory associated with > tracking the transactions, thus leaving potentially leaving > journal->j_checkpoint_transactions to be NULL, even though the on-disk > tail of the journal hasn't been updated yet in the jbd superblock. Yes. > Your solution to this is to avoid cleaning up the in-memory > representation of the transaction until log_do_checkpoint() has a > chance to clean it up. Yes. > Your reasoning and your general diagnosis is sound and I agree with > your observation. However, I disagree with your belief that the > fundamental problem is that journal->j_free is being left "out of > date", and that this is the issue that must be addressed. This is > because your proposed solution of deferring dropping the in-memory > transaction structure has a number of disadvantages. For one, it adds > a lot more code complexity; for another, it means that we are tieing > up memory until we have a chance to call log_do_checkpoint. Yes, I have concerned about it, but I thought it simple to release a checkpoint transaction in log_do_checkpoint() only. However, I agree that jbd should try to release a checkpoint transaction at any places if possible. > Therefore, I believe my original strategy of fixing > __log_wait_for_space() is the correct one, since it was a change in in > that function which introduced the regression in the first place. OK. > However, your insight that the problem is that cleanup_journal_tail() > can sometimes free up space even if journal->j_checkpoint_transactions > is NULL is very important, and it will be more efficient to try to > call cleanup_journal_tail() before trying to wait on the current > transaction to finish. I agree this idea because also it seems to be able to solve the root cause which I am thinking. > So here is my revised patch, which includes your key insight, but > which does not make a large number of changes in other parts of the > jbd code, and which allows transactions to be dropped as soon as we no > longer need to track any buffers associated with them, even though > cleanup_journal_tail() hasn't been called yet. > > - Ted > jbd: don't give up looking for space so easily in __log_wait_for_space wait > > From: Theodore Ts'o <tytso@mit.edu> Your revised patch looks good to me because your new logic is reasonable. But I think that here need to be changed from "int tid = 0" into "tid_t tid = 0". > int chkpt = journal->j_checkpoint_transactions != NULL; > + int tid = 0; And the bug which you have already known is needed to fix. ("Re: ext3: kernel BUG at fs/jbd/journal.c:412!" you posted at Thu, 6 Nov 2008 12:13:22 -0500) I tested your revised patch with the "journal mode", and then I confirmed that "no transactions" error doesn't happen. * You know, this error can happen the most easily by the "journal mode" of three modes. P.S. Another problem can happen with an original kernel plus your revised patch instead of "no transactions" error. This problem is the one that I have been working since one month ago. First patch to try to fix it was posted by me on 17/10/2008 JST. "[RFC][PATCH] JBD: release checkpoint journal heads through try_to_release_page when the memory is exhausted" I found "no transactions" error happen easily with the "journal mode" when I was fixing this problem. So, I have needed a patch which fixes "no transactions" error in order to fix it. Because my fix patch enables journal_heads release more efficiently, and then "no transactions" error can happen more easily than before by jbd to which my patch is applied on not only the "journal mode" but also "ordered mode" or "writeback mode". I will post the revised patch which fixes this problem later. Best Regards, Toshiyuki Okajima -- 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
Hi. I found it possible that even if a lot of pages can be logically released, they cannot be released by try_to_release_page, and then they keep remaining. This case enables an oom-killer to happen easily. Details of the root cause and my patch which fixes it are shown below. --- The direct data blocks can be released by the member function, releasepage() of their mapping of the filesystem i-node. (If an ext3 has the i-node, ext3_releasepage() is used as releasepage().) On the other hand, the indirect data blocks (ext3) are attempted to be released by try_to_free_buffers(). (And other metadata are also done by it.) Because a block device has its mapping, and doesn't have own member function to release a page. But try_to_free_buffers() is a generic function which releases buffer_heads (and a page), and no buffer_head can be released if a buffer_head has private data (like journal_head) because the buffer_head's reference counter is bigger than 0. Therefore, try_to_free_buffers() cannot release a buffer_head even if it is possible to release its private data. As a result, oom-killer may happen when a system memory is exhausted even if it is possible to release a lot of private data and their pages, because try_to_free_buffers() doesn't release such pages. In order to solve this situation, we add a member function into a block device to release private data and then the page. This member function is: - registered at a filesystem initialization time (get_sb_bdev()) - unregistered at a filesystem unmount time (kill_block_super()) This member function's pointer is located in a bdev_inode structure. Besides, a client which registers it is also added into this structure. A client for a filesystem is its superblock. If we use an ext3, this additional member function can do equal processing to ext3_releasepage() by using the superblock. And a block device's releasepage() is necessary to call this additional member function. Therefore we need a member function, 'releasepage' of the mapping of a block device. Changing like them becomes possible to release private data and then the page via try_to_release_page(). Therefore it becomes difficult for oom-killer to happen than before. Because this patch enables journal_heads to be released more efficiently in case of ext3. I will post patches to solve it (ext3/ext4 version): (1) [patch 1/3] vfs: release block-device-mapping buffer_heads which have the filesystem private data for avoiding oom-killer (2) [patch 2/3] ext3: release block-device-mapping buffer_heads which have the filesystem private data for avoiding oom-killer (3) [patch 3/3] ext4: release block-device-mapping buffer_heads which have the filesystem private data for avoiding oom-killer [Additional information] I have confirmed that JBD on 2.6.28-rc4 to which my patch was applied could keep running for long time without oom-killer under the heavy loads. (Of course, JBD without the patch cannot keep running for long time under the same situation.) * This patch needs Ted's fix which was posted at "Wed, 5 Nov 2008 09:05:07 -0500" * as "[PATCH] jbd: don't give up looking for space so easily in * __log_wait_for_space". * Because "no transactions" error happens easily by releasing journal_heads * efficiently with my patch. * But linux-2.6.28-rc4 includes his patch. Therefore I don't care about this. Any comments are welcome. Best Regards, Toshiyuki Okajima -- 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
diff --git a/fs/jbd/checkpoint.c b/fs/jbd/checkpoint.c index 1bd8d4a..5e856de 100644 --- a/fs/jbd/checkpoint.c +++ b/fs/jbd/checkpoint.c @@ -128,25 +128,42 @@ void __log_wait_for_space(journal_t *journal) /* * Test again, another process may have checkpointed while we * were waiting for the checkpoint lock. If there are no - * outstanding transactions there is nothing to checkpoint and - * we can't make progress. Abort the journal in this case. + * transactions ready to be checkpointed, try to recover + * journal space by calling cleanup_journal_tail(), and if + * that doesn't work, by waiting for the currently committing + * transaction to complete. If there is absolutely no way + * to make progress, this is either a BUG or corrupted + * filesystem, so abort the journal and leave a stack + * trace for forensic evidence. */ spin_lock(&journal->j_state_lock); spin_lock(&journal->j_list_lock); nblocks = jbd_space_needed(journal); if (__log_space_left(journal) < nblocks) { int chkpt = journal->j_checkpoint_transactions != NULL; + int tid = 0; + if (journal->j_committing_transaction) + tid = journal->j_committing_transaction->t_tid; spin_unlock(&journal->j_list_lock); spin_unlock(&journal->j_state_lock); if (chkpt) { log_do_checkpoint(journal); + } else if (cleanup_journal_tail(journal) == 0) { + /* We were able to recover space; yay! */ + ; + } else if (tid) { + log_wait_commit(journal, tid); } else { - printk(KERN_ERR "%s: no transactions\n", - __func__); + printk(KERN_ERR "%s: needed %d blocks and " + "only had %d space available\n", + __func__, nblocks, + __log_space_left(journal)); + printk(KERN_ERR "%s: no way to get more " + "journal space\n", __func__); + WARN_ON(1); journal_abort(journal, 0); } - spin_lock(&journal->j_state_lock); } else { spin_unlock(&journal->j_list_lock);