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 | 8 +++++---
fs/jbd/transaction.c | 38 +++++++++++++++++++++++++-------------
include/linux/ext3_jbd.h | 15 ++++++++-------
include/linux/jbd.h | 6 ++++--
4 files changed, 42 insertions(+), 25 deletions(-)
diff -puN fs/ext3/balloc.c~jbd-510-h_credits-fix fs/ext3/balloc.c
--- 25/fs/ext3/balloc.c~jbd-510-h_credits-fix 2003-05-28 23:11:09.000000000 -0700
+++ 25-akpm/fs/ext3/balloc.c 2003-05-28 23:11:09.000000000 -0700
@@ -171,7 +171,7 @@ do_more:
*/
/* @@@ 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
{
int i, fatal = 0;
int have_access = 0;
+ int credits = 0;
*errp = 0;
@@ -432,7 +433,8 @@ got:
* 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 @@ fail:
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;
}
diff -puN include/linux/jbd.h~jbd-510-h_credits-fix include/linux/jbd.h
--- 25/include/linux/jbd.h~jbd-510-h_credits-fix 2003-05-28 23:11:09.000000000 -0700
+++ 25-akpm/include/linux/jbd.h 2003-05-28 23:11:09.000000000 -0700
@@ -887,10 +887,12 @@ extern int journal_restart (handle_t *,
extern int journal_extend (handle_t *, int nblocks);
extern int journal_get_write_access (handle_t *, struct buffer_head *);
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 *,
diff -puN include/linux/ext3_jbd.h~jbd-510-h_credits-fix include/linux/ext3_jbd.h
--- 25/include/linux/ext3_jbd.h~jbd-510-h_credits-fix 2003-05-28 23:11:09.000000000 -0700
+++ 25-akpm/include/linux/ext3_jbd.h 2003-05-28 23:11:09.000000000 -0700
@@ -97,10 +97,10 @@ void ext3_journal_abort_handle(const cha
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;
@@ -117,9 +117,10 @@ __ext3_journal_get_write_access(const ch
}
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,8 +160,8 @@ __ext3_journal_dirty_metadata(const char
}
-#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))
#define ext3_journal_revoke(handle, blocknr, bh) \
diff -puN fs/jbd/transaction.c~jbd-510-h_credits-fix fs/jbd/transaction.c
--- 25/fs/jbd/transaction.c~jbd-510-h_credits-fix 2003-05-28 23:11:09.000000000 -0700
+++ 25-akpm/fs/jbd/transaction.c 2003-05-28 23:11:09.000000000 -0700
@@ -521,7 +521,8 @@ static void jbd_unexpected_dirty_buffer(
*/
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;
@@ -599,6 +600,8 @@ repeat:
J_ASSERT_JH(jh, handle->h_buffer_credits > 0);
handle->h_buffer_credits--;
+ if (credits)
+ (*credits)++;
goto done_locked;
}
@@ -675,6 +678,8 @@ repeat:
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
@@ -736,7 +741,7 @@ int journal_get_write_access (handle_t *
/* 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;
}
@@ -825,7 +830,8 @@ out:
* 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
@@ -844,9 +850,10 @@ out:
* 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);
@@ -854,10 +861,12 @@ int journal_get_undo_access(handle_t *ha
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;
@@ -1129,9 +1138,13 @@ out:
*
* 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;
@@ -1148,12 +1161,11 @@ void journal_release_buffer(handle_t *ha
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");
}
_