jbd2: add debugging information to jbd2_journal_dirty_metadata()

Add debugging information in case jbd2_journal_dirty_metadata() is
called with a buffer_head which didn't have
jbd2_journal_get_write_access() called on it, or if the journal_head
has the wrong transaction in it.  In addition, return an error code.
This won't change anything for ocfs2, which will BUG_ON() the non-zero
exit code.

For ext4, the caller of this function is ext4_handle_dirty_metadata(),
and on seeing a non-zero return code, will call __ext4_journal_stop(),
which will print the function and line number of the (buggy) calling
function and abort the journal.  This will allow us to recover instead
of bug halting, which is better from a robustness and reliability
point of view.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index f5240aa15601a6b34c40b339b0153e6f8bc6867d..aca17901758249d4329d780714e328ef42851e35 100644 (file)
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -109,9 +109,11 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
 
        if (ext4_handle_valid(handle)) {
                err = jbd2_journal_dirty_metadata(handle, bh);
-               if (err)
-                       ext4_journal_abort_handle(where, line, __func__,
-                                                 bh, handle, err);
+               if (err) {
+                       /* Errors can only happen if there is a bug */
+                       handle->h_err = err;
+                       __ext4_journal_stop(where, line, handle);
+               }
        } else {
                if (inode)
                        mark_buffer_dirty_inode(bh, inode);

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 9124cd24e09368a94cf6bcbd490434a5cf0138e3..2c5216a8d03be84c808243c55dea8c04b86b64dc 100644 (file)
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -96,13 +96,17 @@ static int ext4_ext_get_access(handle_t *handle, struct inode *inode,
  *  - ENOMEM
  *  - EIO
  */
-static int ext4_ext_dirty(handle_t *handle, struct inode *inode,
-                               struct ext4_ext_path *path)
+#define ext4_ext_dirty(handle, inode, path) \
+               __ext4_ext_dirty(__func__, __LINE__, (handle), (inode), (path))
+static int __ext4_ext_dirty(const char *where, unsigned int line,
+                           handle_t *handle, struct inode *inode,
+                           struct ext4_ext_path *path)
 {
        int err;
        if (path->p_bh) {
                /* path points to block */
-               err = ext4_handle_dirty_metadata(handle, inode, path->p_bh);
+               err = __ext4_handle_dirty_metadata(where, line, handle,
+                                                  inode, path->p_bh);
        } else {
                /* path points to leaf/index in inode body */
                err = ext4_mark_inode_dirty(handle, inode);

diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
index 2d7109414cdd6b7a4d21bdb2e738ff20581523a4..cb56fe9aaabb6f03b79810ea7b71ed260d16bc76 100644 (file)
--- a/fs/jbd2/transaction.c
+++ b/fs/jbd2/transaction.c
@@ -1049,6 +1049,10 @@ void jbd2_buffer_abort_trigger(struct journal_head *jh,
  * mark dirty metadata which needs to be journaled as part of the current
  * transaction.
  *
+ * The buffer must have previously had jbd2_journal_get_write_access()
+ * called so that it has a valid journal_head attached to the buffer
+ * head.
+ *
  * The buffer is placed on the transaction's metadata list and is marked
  * as belonging to the transaction.
  *
@@ -1065,11 +1069,16 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
        transaction_t *transaction = handle->h_transaction;
        journal_t *journal = transaction->t_journal;
        struct journal_head *jh = bh2jh(bh);
+       int ret = 0;
 
        jbd_debug(5, "journal_head %p\n", jh);
        JBUFFER_TRACE(jh, "entry");
        if (is_handle_aborted(handle))
                goto out;
+       if (!buffer_jbd(bh)) {
+               ret = -EUCLEAN;
+               goto out;
+       }
 
        jbd_lock_bh_state(bh);
 
@@ -1093,8 +1102,20 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
         */
        if (jh->b_transaction == transaction && jh->b_jlist == BJ_Metadata) {
                JBUFFER_TRACE(jh, "fastpath");
-               J_ASSERT_JH(jh, jh->b_transaction ==
-                                       journal->j_running_transaction);
+               if (unlikely(jh->b_transaction !=
+                            journal->j_running_transaction)) {
+                       printk(KERN_EMERG "JBD: %s: "
+                              "jh->b_transaction (%llu, %p, %u) != "
+                              "journal->j_running_transaction (%p, %u)",
+                              journal->j_devname,
+                              (unsigned long long) bh->b_blocknr,
+                              jh->b_transaction,
+                              jh->b_transaction ? jh->b_transaction->t_tid : 0,
+                              journal->j_running_transaction,
+                              journal->j_running_transaction ?
+                              journal->j_running_transaction->t_tid : 0);
+                       ret = -EINVAL;
+               }
                goto out_unlock_bh;
        }
 
@@ -1108,9 +1129,32 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
         */
        if (jh->b_transaction != transaction) {
                JBUFFER_TRACE(jh, "already on other transaction");
-               J_ASSERT_JH(jh, jh->b_transaction ==
-                                       journal->j_committing_transaction);
-               J_ASSERT_JH(jh, jh->b_next_transaction == transaction);
+               if (unlikely(jh->b_transaction !=
+                            journal->j_committing_transaction)) {
+                       printk(KERN_EMERG "JBD: %s: "
+                              "jh->b_transaction (%llu, %p, %u) != "
+                              "journal->j_committing_transaction (%p, %u)",
+                              journal->j_devname,
+                              (unsigned long long) bh->b_blocknr,
+                              jh->b_transaction,
+                              jh->b_transaction ? jh->b_transaction->t_tid : 0,
+                              journal->j_committing_transaction,
+                              journal->j_committing_transaction ?
+                              journal->j_committing_transaction->t_tid : 0);
+                       ret = -EINVAL;
+               }
+               if (unlikely(jh->b_next_transaction != transaction)) {
+                       printk(KERN_EMERG "JBD: %s: "
+                              "jh->b_next_transaction (%llu, %p, %u) != "
+                              "transaction (%p, %u)",
+                              journal->j_devname,
+                              (unsigned long long) bh->b_blocknr,
+                              jh->b_next_transaction,
+                              jh->b_next_transaction ?
+                              jh->b_next_transaction->t_tid : 0,
+                              transaction, transaction->t_tid);
+                       ret = -EINVAL;
+               }
                /* And this case is illegal: we can't reuse another
                 * transaction's data buffer, ever. */
                goto out_unlock_bh;
@@ -1127,7 +1171,9 @@ out_unlock_bh:
        jbd_unlock_bh_state(bh);
 out:
        JBUFFER_TRACE(jh, "exit");
-       return 0;
+       if (ret)
+               __WARN();       /* All errors are bugs, so dump the stack */
+       return ret;
 }
 
 /*