[PATCH] JBD: journal_release_buffer: handle credits fix

There's a bug: a caller tries to journal a buffer and then decides he didn't
want to after all.  He calls journal_release_buffer().

But journal_release_buffer() is only allowed to give the caller a buffer
credit back if it was the caller who added the buffer in the first place.

journal_release_buffer() currently looks at the buffer state to work that
out, but gets it wrong: if the buffer has been moved onto a different list by
some other part of ext3 the credit is bogusly not returned to the caller and
the fs can later go BUG due to handle credit exhaustion.


The fix:

Change journal_get_undo_access() to return the number of buffers which the
caller actually added to the journal.  (one or zero).

When the caller later calls journal_release_buffer(), he passes in that
count, to tell journal_release_buffer() how many credits the caller should
get back.

For API consistency this change should also be made to
journal_get_create_access() and journal_get_write_access().  But there is no
requirement for that in ext3 at this time.


The remaining bug:

This logic effectively gives another transaction handle a free buffer credit.
These could conceivably accumulate and cause a journal overflow.  This is a
separate problem and needs changes to the t_outstanding_credits accounting
and the logic in start_this_handle.

fs/ext3/balloc.c

@@ -171,7 +171,7 @@ void ext3_free_blocks (handle_t *handle, struct inode * inode,
 	 */
 	/* @@@ check errors */
 	BUFFER_TRACE(bitmap_bh, "getting undo access");
-	err = ext3_journal_get_undo_access(handle, bitmap_bh);
+	err = ext3_journal_get_undo_access(handle, bitmap_bh, NULL);
 	if (err)
 		goto error_return;
 	
@@ -406,6 +406,7 @@ ext3_try_to_allocate(struct super_block *sb, handle_t *handle, int group,
 {
 	int i, fatal = 0;
 	int have_access = 0;
+	int credits = 0;
 
 	*errp = 0;
 
@@ -432,7 +433,8 @@ ext3_try_to_allocate(struct super_block *sb, handle_t *handle, int group,
 	 	 * committing transaction.
 		 */
 		BUFFER_TRACE(bitmap_bh, "get undo access for new block");
-		fatal = ext3_journal_get_undo_access(handle, bitmap_bh);
+		fatal = ext3_journal_get_undo_access(handle, bitmap_bh,
+							&credits);
 		if (fatal) {
 			*errp = fatal;
 			goto fail;
@@ -465,7 +467,7 @@ ext3_try_to_allocate(struct super_block *sb, handle_t *handle, int group,
 	if (have_access) {
 		BUFFER_TRACE(bitmap_bh, "journal_release_buffer");
 		jbd_unlock_bh_state(bitmap_bh);
-		ext3_journal_release_buffer(handle, bitmap_bh);
+		ext3_journal_release_buffer(handle, bitmap_bh, credits);
 	}
 	return -1;
 }

fs/ext3/ialloc.c

@@ -472,8 +472,11 @@ struct inode *ext3_new_inode(handle_t *handle, struct inode * dir, int mode)
 		ino = ext3_find_first_zero_bit((unsigned long *)
 				bitmap_bh->b_data, EXT3_INODES_PER_GROUP(sb));
 		if (ino < EXT3_INODES_PER_GROUP(sb)) {
+			int credits = 0;
+
 			BUFFER_TRACE(bitmap_bh, "get_write_access");
-			err = ext3_journal_get_write_access(handle, bitmap_bh);
+			err = ext3_journal_get_write_access_credits(handle,
+							bitmap_bh, &credits);
 			if (err)
 				goto fail;
 
@@ -489,7 +492,7 @@ struct inode *ext3_new_inode(handle_t *handle, struct inode * dir, int mode)
 				goto got;
 			}
 			/* we lost it */
-			journal_release_buffer(handle, bitmap_bh);
+			journal_release_buffer(handle, bitmap_bh, credits);
 		}
 
 		/*

fs/jbd/transaction.c

@@ -526,7 +526,8 @@ static void jbd_unexpected_dirty_buffer(struct journal_head *jh)
  */
 
 static int
-do_get_write_access(handle_t *handle, struct journal_head *jh, int force_copy) 
+do_get_write_access(handle_t *handle, struct journal_head *jh,
+			int force_copy, int *credits) 
 {
 	struct buffer_head *bh;
 	transaction_t *transaction = handle->h_transaction;
@@ -604,6 +605,8 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, int force_copy)
 
 		J_ASSERT_JH(jh, handle->h_buffer_credits > 0);
 		handle->h_buffer_credits--;
+		if (credits)
+			(*credits)++;
 		goto done_locked;
 	}
 	
@@ -680,6 +683,8 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, int force_copy)
 
 	J_ASSERT(handle->h_buffer_credits > 0);
 	handle->h_buffer_credits--;
+	if (credits)
+		(*credits)++;
 
 	/* Finally, if the buffer is not journaled right now, we need to
 	 * make sure it doesn't get written to disk before the caller
@@ -733,7 +738,8 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, int force_copy)
  * because we're write()ing a buffer which is also part of a shared mapping.
  */
 
-int journal_get_write_access (handle_t *handle, struct buffer_head *bh) 
+int journal_get_write_access(handle_t *handle,
+			struct buffer_head *bh, int *credits)
 {
 	struct journal_head *jh = journal_add_journal_head(bh);
 	int rc;
@@ -741,7 +747,7 @@ int journal_get_write_access (handle_t *handle, struct buffer_head *bh)
 	/* We do not want to get caught playing with fields which the
 	 * log thread also manipulates.  Make sure that the buffer
 	 * completes any outstanding IO before proceeding. */
-	rc = do_get_write_access(handle, jh, 0);
+	rc = do_get_write_access(handle, jh, 0, NULL);
 	journal_put_journal_head(jh);
 	return rc;
 }
@@ -830,7 +836,8 @@ int journal_get_create_access(handle_t *handle, struct buffer_head *bh)
  *     non-rewindable consequences
  * @handle: transaction
  * @bh: buffer to undo
- * 
+ * @credits: store the number of taken credits here (if not NULL)
+ *
  * Sometimes there is a need to distinguish between metadata which has
  * been committed to disk and that which has not.  The ext3fs code uses
  * this for freeing and allocating space, we have to make sure that we
@@ -849,9 +856,10 @@ int journal_get_create_access(handle_t *handle, struct buffer_head *bh)
  * will be committed to a new transaction in due course, at which point
  * we can discard the old committed data pointer.
  *
- * Returns error number or 0 on success.  
+ * Returns error number or 0 on success.
  */
-int journal_get_undo_access(handle_t *handle, struct buffer_head *bh)
+int journal_get_undo_access(handle_t *handle, struct buffer_head *bh,
+				int *credits)
 {
 	int err;
 	struct journal_head *jh = journal_add_journal_head(bh);
@@ -859,10 +867,12 @@ int journal_get_undo_access(handle_t *handle, struct buffer_head *bh)
 
 	JBUFFER_TRACE(jh, "entry");
 
-	/* Do this first --- it can drop the journal lock, so we want to
+	/*
+	 * Do this first --- it can drop the journal lock, so we want to
 	 * make sure that obtaining the committed_data is done
-	 * atomically wrt. completion of any outstanding commits. */
-	err = do_get_write_access (handle, jh, 1);
+	 * atomically wrt. completion of any outstanding commits.
+	 */
+	err = do_get_write_access(handle, jh, 1, credits);
 	if (err)
 		goto out;
 
@@ -1134,9 +1144,13 @@ int journal_dirty_metadata(handle_t *handle, struct buffer_head *bh)
  *
  * journal_get_write_access() can block, so it is quite possible for a
  * journaling component to decide after the write access is returned
- * that global state has changed and the update is no longer required.  */
-
-void journal_release_buffer(handle_t *handle, struct buffer_head *bh)
+ * that global state has changed and the update is no longer required.
+ *
+ * The caller passes in the number of credits which should be put back for
+ * this buffer (zero or one).
+ */
+void
+journal_release_buffer(handle_t *handle, struct buffer_head *bh, int credits)
 {
 	transaction_t *transaction = handle->h_transaction;
 	journal_t *journal = transaction->t_journal;
@@ -1153,12 +1167,11 @@ void journal_release_buffer(handle_t *handle, struct buffer_head *bh)
 	if (jh->b_jlist == BJ_Reserved && jh->b_transaction == transaction &&
 	    !buffer_jbddirty(jh2bh(jh))) {
 		JBUFFER_TRACE(jh, "unused: refiling it");
-		handle->h_buffer_credits++;
 		__journal_refile_buffer(jh);
 	}
 	spin_unlock(&journal->j_list_lock);
 	jbd_unlock_bh_state(bh);
-
+	handle->h_buffer_credits += credits;
 	JBUFFER_TRACE(jh, "exit");
 }
 

include/linux/ext3_jbd.h

@@ -97,29 +97,30 @@ void ext3_journal_abort_handle(const char *caller, const char *err_fn,
 		struct buffer_head *bh, handle_t *handle, int err);
 
 static inline int
-__ext3_journal_get_undo_access(const char *where,
-			       handle_t *handle, struct buffer_head *bh)
+__ext3_journal_get_undo_access(const char *where, handle_t *handle,
+				struct buffer_head *bh, int *credits)
 {
-	int err = journal_get_undo_access(handle, bh);
+	int err = journal_get_undo_access(handle, bh, credits);
 	if (err)
 		ext3_journal_abort_handle(where, __FUNCTION__, bh, handle,err);
 	return err;
 }
 
 static inline int
-__ext3_journal_get_write_access(const char *where,
-				handle_t *handle, struct buffer_head *bh)
+__ext3_journal_get_write_access(const char *where, handle_t *handle,
+				struct buffer_head *bh, int *credits)
 {
-	int err = journal_get_write_access(handle, bh);
+	int err = journal_get_write_access(handle, bh, credits);
 	if (err)
 		ext3_journal_abort_handle(where, __FUNCTION__, bh, handle,err);
 	return err;
 }
 
 static inline void
-ext3_journal_release_buffer(handle_t *handle, struct buffer_head *bh)
+ext3_journal_release_buffer(handle_t *handle, struct buffer_head *bh,
+				int credits)
 {
-	journal_release_buffer(handle, bh);
+	journal_release_buffer(handle, bh, credits);
 }
 
 static inline void
@@ -159,10 +160,12 @@ __ext3_journal_dirty_metadata(const char *where,
 }
 
 
-#define ext3_journal_get_undo_access(handle, bh) \
-	__ext3_journal_get_undo_access(__FUNCTION__, (handle), (bh))
+#define ext3_journal_get_undo_access(handle, bh, credits) \
+	__ext3_journal_get_undo_access(__FUNCTION__, (handle), (bh), (credits))
 #define ext3_journal_get_write_access(handle, bh) \
-	__ext3_journal_get_write_access(__FUNCTION__, (handle), (bh))
+	__ext3_journal_get_write_access(__FUNCTION__, (handle), (bh), NULL)
+#define ext3_journal_get_write_access_credits(handle, bh, credits) \
+	__ext3_journal_get_write_access(__FUNCTION__, (handle), (bh), (credits))
 #define ext3_journal_revoke(handle, blocknr, bh) \
 	__ext3_journal_revoke(__FUNCTION__, (handle), (blocknr), (bh))
 #define ext3_journal_get_create_access(handle, bh) \

include/linux/jbd.h

@@ -885,12 +885,15 @@ static inline handle_t *journal_current_handle(void)
 extern handle_t *journal_start(journal_t *, int nblocks);
 extern int	 journal_restart (handle_t *, int nblocks);
 extern int	 journal_extend (handle_t *, int nblocks);
-extern int	 journal_get_write_access (handle_t *, struct buffer_head *);
+extern int	 journal_get_write_access(handle_t *, struct buffer_head *,
+						int *credits);
 extern int	 journal_get_create_access (handle_t *, struct buffer_head *);
-extern int	 journal_get_undo_access (handle_t *, struct buffer_head *);
+extern int	 journal_get_undo_access(handle_t *, struct buffer_head *,
+						int *credits);
 extern int	 journal_dirty_data (handle_t *, struct buffer_head *);
 extern int	 journal_dirty_metadata (handle_t *, struct buffer_head *);
-extern void	 journal_release_buffer (handle_t *, struct buffer_head *);
+extern void	 journal_release_buffer (handle_t *, struct buffer_head *,
+						int credits);
 extern void	 journal_forget (handle_t *, struct buffer_head *);
 extern void	 journal_sync_buffer (struct buffer_head *);
 extern int	 journal_invalidatepage(journal_t *,