* Re: [PATCH 3.2.x] jbd2: add mutex_lock on j_checkpoint_mutex in jbd2_journal_flush
2015-09-29 15:16 ` Jan Kara
@ 2015-10-03 15:35 ` Ben Hutchings
0 siblings, 0 replies; 4+ messages in thread
From: Ben Hutchings @ 2015-10-03 15:35 UTC (permalink / raw)
To: Jan Kara; +Cc: zerg2000, stable
[-- Attachment #1.1: Type: text/plain, Size: 3659 bytes --]
On Tue, 2015-09-29 at 17:16 +0200, Jan Kara wrote:
> On Sun 27-09-15 14:44:09, Ben Hutchings wrote:
> > On Sun, 2015-09-06 at 03:25 +0200, Bartosz Kwitniewski wrote:
> > > Commit a3ceb22921615827bfed39d7612a9a370bff0edb (upstream
> > > 79feb521a44705262d15cc819a4117a447b11ea7) in 3.2.x tree introduced
> > > __jbd2_update_log_tail which requires j_checkpoint_mutex, but locking of
> > > j_checkpoint_mutex in jbd2_journal_flush was not backported from upstream.
> >
> > Oops.
> >
> > > Fixes kernel BUG at fs/jbd2/journal.c:832 (__jbd2_update_log_tail):
> > > [] ? jbd2_cleanup_journal_tail+0x5d/0x61 [jbd2]
> > > [] ? jbd2_journal_flush+0xc2/0x156 [jbd2]
> > > [] ? ext4_freeze+0x2f/0x71 [ext4]
> > > [] ? filemap_write_and_wait+0x26/0x32
> > > [] ? freeze_super+0x8c/0xdd
> > > [] ? freeze_bdev+0x5b/0xa1
> > > [] ? start_cow_session+0xb3/0x2d6 [hcpdriver]
> > > [] ? printk+0x40/0x49
> > > [] ? alloc_cts_session+0x2e/0x33 [hcpdriver]
> > > [] ? ioctl_start_hcp_session+0x131/0x20d [hcpdriver]
> > > [] ? handle_ioctlStartHC2+0x95/0x1ab [hcpdriver]
> > > [] ? cow_ioctl_unlocked+0x13/0x18 [hcpdriver]
> > > [] ? do_vfs_ioctl+0x55a/0x5a9
> > > [] ? pax_randomize_kstack+0x4c/0x60
> > > [] ? sysret_check+0x20/0x62
> > > [] ? do_sys_open+0x11e/0x130
> > > [] ? sys_ioctl+0x3c/0x5f
> > > [] ? system_call_fastpath+0x16/0x1b
> > >
> > > Signed-off-by: Bartosz Kwitniewski <zerg2000@astral.org.pl>
> > > Cc: stable@vger.kernel.org
> > > ---
> > > --- fs/jbd2/journal.c.orig> > > > > > > > 2015-08-12 16:33:24.000000000 +0200
> > > +++ fs/jbd2/journal.c> > > > > > > > 2015-09-06 00:57:56.890894891 +0200
> > > @@ -1828,10 +1828,13 @@ int jbd2_journal_flush(journal_t *journa
> > > > > > > > > > > if (is_journal_aborted(journal))
> > > > > > > > > > > > > > > > > > return -EIO;
> > >
> > > +> > > > > > > > mutex_lock(&journal->j_checkpoint_mutex);
> > > > > > > > > > > if (!err) {
> > > > > > > > > > > > > > > > > > err = jbd2_cleanup_journal_tail(journal);
> > > -> > > > > > > > > > > > > > > if (err < 0)
> > > +> > > > > > > > > > > > > > > if (err < 0) {
> > > +> > > > > > > > > > > > > > > > > > > > > > mutex_unlock(&journal->j_checkpoint_mutex);
> > > > > > > > > > > > > > > > > > > > > > > > > goto out;
> > > +> > > > > > > > > > > > > > > }
> > > > > > > > > > > > > > > > > > err = 0;
> > > > > > > > > > > }
> > >
> > > @@ -1841,6 +1844,7 @@ int jbd2_journal_flush(journal_t *journa
> > > > > > > > > > > * commits of data to the journal will restore the current
> > > > > > > > > > > * s_start value. */
> > > > > > > > > > > jbd2_mark_journal_empty(journal);
> > > +> > > > > > > > mutex_unlock(&journal->j_checkpoint_mutex);
> > > > > > > > > > > write_lock(&journal->j_state_lock);
> > > > > > > > > > > J_ASSERT(!journal->j_running_transaction);
> > > > > > > > > > > J_ASSERT(!journal->j_committing_transaction);
> >
> > Why is it sufficient to add locking of j_checkpoint_mutex only in this
> > one function?
> >
> > Shouldn't I cherry-pick commits 24bcc89c7e7c ("jbd2: split updating of
> > journal superblock and marking journal empty")
I actually applied that one already.
> > and a78bb11d7acd ("jbd2:
> > protect all log tail updates with j_checkpoint_mutex") as well?
>
> Just to confirm, I agree these two commits are the ones you should
> cherry-pick for commit 79feb521a44705262d15cc819a4117a447b11ea7 from
> upstream to work correctly.
So here's the series of 3 patches as applied/queued to 3.2.
Ben.
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
[-- Attachment #1.2: Type: text/x-patch, Size: 9794 bytes --]
From: Jan Kara <jack@suse.cz>
Date: Tue, 13 Mar 2012 15:41:04 -0400
Subject: jbd2: split updating of journal superblock and marking journal empty
commit 24bcc89c7e7c64982e6192b4952a0a92379fc341 upstream.
There are three case of updating journal superblock. In the first case, we want
to mark journal as empty (setting s_sequence to 0), in the second case we want
to update log tail, in the third case we want to update s_errno. Split these
cases into separate functions. It makes the code slightly more straightforward
and later patches will make the distinction even more important.
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
[bwh: Prerequisite for "jbd2: fix ocfs2 corrupt when updating journal
superblock fails".
Backported to 3.2: drop changes to trace events.]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -550,7 +550,7 @@ int jbd2_cleanup_journal_tail(journal_t
(journal->j_flags & JBD2_BARRIER))
blkdev_issue_flush(journal->j_fs_dev, GFP_KERNEL, NULL);
if (!(journal->j_flags & JBD2_ABORT))
- jbd2_journal_update_superblock(journal, 1);
+ jbd2_journal_update_sb_log_tail(journal);
return 0;
}
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -340,7 +340,7 @@ void jbd2_journal_commit_transaction(jou
/* Do we need to erase the effects of a prior jbd2_journal_flush? */
if (journal->j_flags & JBD2_FLUSHED) {
jbd_debug(3, "super block updated\n");
- jbd2_journal_update_superblock(journal, 1);
+ jbd2_journal_update_sb_log_tail(journal);
} else {
jbd_debug(3, "superblock not updated\n");
}
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1143,39 +1143,28 @@ static int journal_reset(journal_t *jour
journal->j_max_transaction_buffers = journal->j_maxlen / 4;
- /* Add the dynamic fields and write it to disk. */
- jbd2_journal_update_superblock(journal, 1);
- return jbd2_journal_start_thread(journal);
-}
-
-/**
- * void jbd2_journal_update_superblock() - Update journal sb on disk.
- * @journal: The journal to update.
- * @wait: Set to '0' if you don't want to wait for IO completion.
- *
- * Update a journal's dynamic superblock fields and write it to disk,
- * optionally waiting for the IO to complete.
- */
-void jbd2_journal_update_superblock(journal_t *journal, int wait)
-{
- journal_superblock_t *sb = journal->j_superblock;
- struct buffer_head *bh = journal->j_sb_buffer;
-
/*
* As a special case, if the on-disk copy is already marked as needing
- * no recovery (s_start == 0) and there are no outstanding transactions
- * in the filesystem, then we can safely defer the superblock update
- * until the next commit by setting JBD2_FLUSHED. This avoids
+ * no recovery (s_start == 0), then we can safely defer the superblock
+ * update until the next commit by setting JBD2_FLUSHED. This avoids
* attempting a write to a potential-readonly device.
*/
- if (sb->s_start == 0 && journal->j_tail_sequence ==
- journal->j_transaction_sequence) {
+ if (sb->s_start == 0) {
jbd_debug(1, "JBD2: Skipping superblock update on recovered sb "
"(start %ld, seq %d, errno %d)\n",
journal->j_tail, journal->j_tail_sequence,
journal->j_errno);
- goto out;
+ journal->j_flags |= JBD2_FLUSHED;
+ } else {
+ /* Add the dynamic fields and write it to disk. */
+ jbd2_journal_update_sb_log_tail(journal);
}
+ return jbd2_journal_start_thread(journal);
+}
+
+static void jbd2_write_superblock(journal_t *journal)
+{
+ struct buffer_head *bh = journal->j_sb_buffer;
if (buffer_write_io_error(bh)) {
/*
@@ -1193,47 +1182,97 @@ void jbd2_journal_update_superblock(jour
set_buffer_uptodate(bh);
}
+ BUFFER_TRACE(bh, "marking dirty");
+ mark_buffer_dirty(bh);
+ sync_dirty_buffer(bh);
+ if (buffer_write_io_error(bh)) {
+ printk(KERN_ERR "JBD2: I/O error detected "
+ "when updating journal superblock for %s.\n",
+ journal->j_devname);
+ clear_buffer_write_io_error(bh);
+ set_buffer_uptodate(bh);
+ }
+}
+
+/**
+ * jbd2_journal_update_sb_log_tail() - Update log tail in journal sb on disk.
+ * @journal: The journal to update.
+ *
+ * Update a journal's superblock information about log tail and write it to
+ * disk, waiting for the IO to complete.
+ */
+void jbd2_journal_update_sb_log_tail(journal_t *journal)
+{
+ journal_superblock_t *sb = journal->j_superblock;
+
read_lock(&journal->j_state_lock);
- jbd_debug(1, "JBD2: updating superblock (start %ld, seq %d, errno %d)\n",
- journal->j_tail, journal->j_tail_sequence, journal->j_errno);
+ jbd_debug(1, "JBD2: updating superblock (start %ld, seq %d)\n",
+ journal->j_tail, journal->j_tail_sequence);
sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
sb->s_start = cpu_to_be32(journal->j_tail);
- sb->s_errno = cpu_to_be32(journal->j_errno);
read_unlock(&journal->j_state_lock);
- BUFFER_TRACE(bh, "marking dirty");
- mark_buffer_dirty(bh);
- if (wait) {
- sync_dirty_buffer(bh);
- if (buffer_write_io_error(bh)) {
- printk(KERN_ERR "JBD2: I/O error detected "
- "when updating journal superblock for %s.\n",
- journal->j_devname);
- clear_buffer_write_io_error(bh);
- set_buffer_uptodate(bh);
- }
- } else
- write_dirty_buffer(bh, WRITE);
+ jbd2_write_superblock(journal);
+ /* Log is no longer empty */
+ write_lock(&journal->j_state_lock);
+ WARN_ON(!sb->s_sequence);
+ journal->j_flags &= ~JBD2_FLUSHED;
+ write_unlock(&journal->j_state_lock);
+}
-out:
- /* If we have just flushed the log (by marking s_start==0), then
- * any future commit will have to be careful to update the
- * superblock again to re-record the true start of the log. */
+/**
+ * jbd2_mark_journal_empty() - Mark on disk journal as empty.
+ * @journal: The journal to update.
+ *
+ * Update a journal's dynamic superblock fields to show that journal is empty.
+ * Write updated superblock to disk waiting for IO to complete.
+ */
+static void jbd2_mark_journal_empty(journal_t *journal)
+{
+ journal_superblock_t *sb = journal->j_superblock;
+ read_lock(&journal->j_state_lock);
+ jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
+ journal->j_tail_sequence);
+
+ sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
+ sb->s_start = cpu_to_be32(0);
+ read_unlock(&journal->j_state_lock);
+
+ jbd2_write_superblock(journal);
+
+ /* Log is no longer empty */
write_lock(&journal->j_state_lock);
- if (sb->s_start)
- journal->j_flags &= ~JBD2_FLUSHED;
- else
- journal->j_flags |= JBD2_FLUSHED;
+ journal->j_flags |= JBD2_FLUSHED;
write_unlock(&journal->j_state_lock);
}
+
+/**
+ * jbd2_journal_update_sb_errno() - Update error in the journal.
+ * @journal: The journal to update.
+ *
+ * Update a journal's errno. Write updated superblock to disk waiting for IO
+ * to complete.
+ */
+static void jbd2_journal_update_sb_errno(journal_t *journal)
+{
+ journal_superblock_t *sb = journal->j_superblock;
+
+ read_lock(&journal->j_state_lock);
+ jbd_debug(1, "JBD2: updating superblock error (errno %d)\n",
+ journal->j_errno);
+ sb->s_errno = cpu_to_be32(journal->j_errno);
+ read_unlock(&journal->j_state_lock);
+
+ jbd2_write_superblock(journal);
+}
+
/*
* Read the superblock for a given journal, performing initial
* validation of the format.
*/
-
static int journal_get_superblock(journal_t *journal)
{
struct buffer_head *bh;
@@ -1426,15 +1465,10 @@ int jbd2_journal_destroy(journal_t *jour
spin_unlock(&journal->j_list_lock);
if (journal->j_sb_buffer) {
- if (!is_journal_aborted(journal)) {
- /* We can now mark the journal as empty. */
- journal->j_tail = 0;
- journal->j_tail_sequence =
- ++journal->j_transaction_sequence;
- jbd2_journal_update_superblock(journal, 1);
- } else {
+ if (!is_journal_aborted(journal))
+ jbd2_mark_journal_empty(journal);
+ else
err = -EIO;
- }
brelse(journal->j_sb_buffer);
}
@@ -1648,7 +1682,6 @@ int jbd2_journal_flush(journal_t *journa
{
int err = 0;
transaction_t *transaction = NULL;
- unsigned long old_tail;
write_lock(&journal->j_state_lock);
@@ -1690,14 +1723,8 @@ int jbd2_journal_flush(journal_t *journa
* the magic code for a fully-recovered superblock. Any future
* commits of data to the journal will restore the current
* s_start value. */
+ jbd2_mark_journal_empty(journal);
write_lock(&journal->j_state_lock);
- old_tail = journal->j_tail;
- journal->j_tail = 0;
- write_unlock(&journal->j_state_lock);
- jbd2_journal_update_superblock(journal, 1);
- write_lock(&journal->j_state_lock);
- journal->j_tail = old_tail;
-
J_ASSERT(!journal->j_running_transaction);
J_ASSERT(!journal->j_committing_transaction);
J_ASSERT(!journal->j_checkpoint_transactions);
@@ -1738,7 +1765,7 @@ int jbd2_journal_wipe(journal_t *journal
err = jbd2_journal_skip_recovery(journal);
if (write)
- jbd2_journal_update_superblock(journal, 1);
+ jbd2_mark_journal_empty(journal);
no_recovery:
return err;
@@ -1788,7 +1815,7 @@ static void __journal_abort_soft (journa
__jbd2_journal_abort_hard(journal);
if (errno)
- jbd2_journal_update_superblock(journal, 1);
+ jbd2_journal_update_sb_errno(journal);
}
/**
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1083,7 +1083,7 @@ extern int jbd2_journal_destroy (j
extern int jbd2_journal_recover (journal_t *journal);
extern int jbd2_journal_wipe (journal_t *, int);
extern int jbd2_journal_skip_recovery (journal_t *);
-extern void jbd2_journal_update_superblock (journal_t *, int);
+extern void jbd2_journal_update_sb_log_tail (journal_t *);
extern void __jbd2_journal_abort_hard (journal_t *);
extern void jbd2_journal_abort (journal_t *, int);
extern int jbd2_journal_errno (journal_t *);
[-- Attachment #1.3: Type: text/x-patch, Size: 4439 bytes --]
From: Jan Kara <jack@suse.cz>
Date: Tue, 13 Mar 2012 15:43:04 -0400
Subject: jbd2: protect all log tail updates with j_checkpoint_mutex
commit a78bb11d7acd525623c6a0c2ff4e213d527573fa upstream.
There are some log tail updates that are not protected by j_checkpoint_mutex.
Some of these are harmless because they happen during startup or shutdown but
updates in jbd2_journal_commit_transaction() and jbd2_journal_flush() can
really race with other log tail updates (e.g. someone doing
jbd2_journal_flush() with someone running jbd2_cleanup_journal_tail()). So
protect all log tail updates with j_checkpoint_mutex.
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
[bwh: Backported to 3.2:
- Adjust context
- Add unlock on the error path in jbd2_journal_flush()]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: Bartosz Kwitniewski <zerg2000@astral.org.pl>
---
fs/jbd2/commit.c | 2 ++
fs/jbd2/journal.c | 19 ++++++++++++++++---
2 files changed, 18 insertions(+), 3 deletions(-)
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -340,6 +340,7 @@ void jbd2_journal_commit_transaction(jou
/* Do we need to erase the effects of a prior jbd2_journal_flush? */
if (journal->j_flags & JBD2_FLUSHED) {
jbd_debug(3, "super block updated\n");
+ mutex_lock(&journal->j_checkpoint_mutex);
/*
* We hold j_checkpoint_mutex so tail cannot change under us.
* We don't need any special data guarantees for writing sb
@@ -350,6 +351,7 @@ void jbd2_journal_commit_transaction(jou
journal->j_tail_sequence,
journal->j_tail,
WRITE_SYNC);
+ mutex_unlock(&journal->j_checkpoint_mutex);
} else {
jbd_debug(3, "superblock not updated\n");
}
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1242,6 +1242,8 @@ static int journal_reset(journal_t *jour
journal->j_errno);
journal->j_flags |= JBD2_FLUSHED;
} else {
+ /* Lock here to make assertions happy... */
+ mutex_lock(&journal->j_checkpoint_mutex);
/*
* Update log tail information. We use WRITE_FUA since new
* transaction will start reusing journal space and so we
@@ -1252,6 +1254,7 @@ static int journal_reset(journal_t *jour
journal->j_tail_sequence,
journal->j_tail,
WRITE_FUA);
+ mutex_unlock(&journal->j_checkpoint_mutex);
}
return jbd2_journal_start_thread(journal);
}
@@ -1314,6 +1317,7 @@ int jbd2_journal_update_sb_log_tail(jour
journal_superblock_t *sb = journal->j_superblock;
int ret;
+ BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
tail_block, tail_tid);
@@ -1344,6 +1348,7 @@ static void jbd2_mark_journal_empty(jour
{
journal_superblock_t *sb = journal->j_superblock;
+ BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
read_lock(&journal->j_state_lock);
jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
journal->j_tail_sequence);
@@ -1577,9 +1582,11 @@ int jbd2_journal_destroy(journal_t *jour
spin_unlock(&journal->j_list_lock);
if (journal->j_sb_buffer) {
- if (!is_journal_aborted(journal))
+ if (!is_journal_aborted(journal)) {
+ mutex_lock(&journal->j_checkpoint_mutex);
jbd2_mark_journal_empty(journal);
- else
+ mutex_unlock(&journal->j_checkpoint_mutex);
+ } else
err = -EIO;
brelse(journal->j_sb_buffer);
}
@@ -1828,10 +1835,13 @@ int jbd2_journal_flush(journal_t *journa
if (is_journal_aborted(journal))
return -EIO;
+ mutex_lock(&journal->j_checkpoint_mutex);
if (!err) {
err = jbd2_cleanup_journal_tail(journal);
- if (err < 0)
+ if (err < 0) {
+ mutex_unlock(&journal->j_checkpoint_mutex);
goto out;
+ }
err = 0;
}
@@ -1841,6 +1851,7 @@ int jbd2_journal_flush(journal_t *journa
* commits of data to the journal will restore the current
* s_start value. */
jbd2_mark_journal_empty(journal);
+ mutex_unlock(&journal->j_checkpoint_mutex);
write_lock(&journal->j_state_lock);
J_ASSERT(!journal->j_running_transaction);
J_ASSERT(!journal->j_committing_transaction);
@@ -1882,8 +1893,12 @@ int jbd2_journal_wipe(journal_t *journal
write ? "Clearing" : "Ignoring");
err = jbd2_journal_skip_recovery(journal);
- if (write)
+ if (write) {
+ /* Lock to make assertions happy... */
+ mutex_lock(&journal->j_checkpoint_mutex);
jbd2_mark_journal_empty(journal);
+ mutex_unlock(&journal->j_checkpoint_mutex);
+ }
no_recovery:
return err;
[-- Attachment #1.4: Type: text/x-patch, Size: 14351 bytes --]
From: Jan Kara <jack@suse.cz>
Date: Tue, 13 Mar 2012 22:22:54 -0400
Subject: jbd2: issue cache flush after checkpointing even with internal
journal
commit 79feb521a44705262d15cc819a4117a447b11ea7 upstream.
When we reach jbd2_cleanup_journal_tail(), there is no guarantee that
checkpointed buffers are on a stable storage - especially if buffers were
written out by jbd2_log_do_checkpoint(), they are likely to be only in disk's
caches. Thus when we update journal superblock effectively removing old
transaction from journal, this write of superblock can get to stable storage
before those checkpointed buffers which can result in filesystem corruption
after a crash. Thus we must unconditionally issue a cache flush before we
update journal superblock in these cases.
A similar problem can also occur if journal superblock is written only in
disk's caches, other transaction starts reusing space of the transaction
cleaned from the log and power failure happens. Subsequent journal replay would
still try to replay the old transaction but some of it's blocks may be already
overwritten by the new transaction. For this reason we must use WRITE_FUA when
updating log tail and we must first write new log tail to disk and update
in-memory information only after that.
Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
[bwh: Prerequisite for "jbd2: fix ocfs2 corrupt when updating journal
superblock fails".
Backported to 3.2:
- Adjust context
- Drop changes to jbd2_journal_update_sb_log_tail trace event]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
--- a/fs/jbd2/checkpoint.c
+++ b/fs/jbd2/checkpoint.c
@@ -478,79 +478,28 @@ out:
int jbd2_cleanup_journal_tail(journal_t *journal)
{
- transaction_t * transaction;
tid_t first_tid;
- unsigned long blocknr, freed;
+ unsigned long blocknr;
if (is_journal_aborted(journal))
return 1;
- /* OK, work out the oldest transaction remaining in the log, and
- * the log block it starts at.
- *
- * If the log is now empty, we need to work out which is the
- * next transaction ID we will write, and where it will
- * start. */
-
- write_lock(&journal->j_state_lock);
- spin_lock(&journal->j_list_lock);
- transaction = journal->j_checkpoint_transactions;
- if (transaction) {
- first_tid = transaction->t_tid;
- blocknr = transaction->t_log_start;
- } else if ((transaction = journal->j_committing_transaction) != NULL) {
- first_tid = transaction->t_tid;
- blocknr = transaction->t_log_start;
- } else if ((transaction = journal->j_running_transaction) != NULL) {
- first_tid = transaction->t_tid;
- blocknr = journal->j_head;
- } else {
- first_tid = journal->j_transaction_sequence;
- blocknr = journal->j_head;
- }
- spin_unlock(&journal->j_list_lock);
- J_ASSERT(blocknr != 0);
-
- /* If the oldest pinned transaction is at the tail of the log
- already then there's not much we can do right now. */
- if (journal->j_tail_sequence == first_tid) {
- write_unlock(&journal->j_state_lock);
+ if (!jbd2_journal_get_log_tail(journal, &first_tid, &blocknr))
return 1;
- }
-
- /* OK, update the superblock to recover the freed space.
- * Physical blocks come first: have we wrapped beyond the end of
- * the log? */
- freed = blocknr - journal->j_tail;
- if (blocknr < journal->j_tail)
- freed = freed + journal->j_last - journal->j_first;
-
- trace_jbd2_cleanup_journal_tail(journal, first_tid, blocknr, freed);
- jbd_debug(1,
- "Cleaning journal tail from %d to %d (offset %lu), "
- "freeing %lu\n",
- journal->j_tail_sequence, first_tid, blocknr, freed);
-
- journal->j_free += freed;
- journal->j_tail_sequence = first_tid;
- journal->j_tail = blocknr;
- write_unlock(&journal->j_state_lock);
+ J_ASSERT(blocknr != 0);
/*
- * If there is an external journal, we need to make sure that
- * any data blocks that were recently written out --- perhaps
- * by jbd2_log_do_checkpoint() --- are flushed out before we
- * drop the transactions from the external journal. It's
- * unlikely this will be necessary, especially with a
- * appropriately sized journal, but we need this to guarantee
- * correctness. Fortunately jbd2_cleanup_journal_tail()
- * doesn't get called all that often.
+ * We need to make sure that any blocks that were recently written out
+ * --- perhaps by jbd2_log_do_checkpoint() --- are flushed out before
+ * we drop the transactions from the journal. It's unlikely this will
+ * be necessary, especially with an appropriately sized journal, but we
+ * need this to guarantee correctness. Fortunately
+ * jbd2_cleanup_journal_tail() doesn't get called all that often.
*/
- if ((journal->j_fs_dev != journal->j_dev) &&
- (journal->j_flags & JBD2_BARRIER))
+ if (journal->j_flags & JBD2_BARRIER)
blkdev_issue_flush(journal->j_fs_dev, GFP_KERNEL, NULL);
- if (!(journal->j_flags & JBD2_ABORT))
- jbd2_journal_update_sb_log_tail(journal);
+
+ __jbd2_update_log_tail(journal, first_tid, blocknr);
return 0;
}
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -340,7 +340,16 @@ void jbd2_journal_commit_transaction(jou
/* Do we need to erase the effects of a prior jbd2_journal_flush? */
if (journal->j_flags & JBD2_FLUSHED) {
jbd_debug(3, "super block updated\n");
- jbd2_journal_update_sb_log_tail(journal);
+ /*
+ * We hold j_checkpoint_mutex so tail cannot change under us.
+ * We don't need any special data guarantees for writing sb
+ * since journal is empty and it is ok for write to be
+ * flushed only with transaction commit.
+ */
+ jbd2_journal_update_sb_log_tail(journal,
+ journal->j_tail_sequence,
+ journal->j_tail,
+ WRITE_SYNC);
} else {
jbd_debug(3, "superblock not updated\n");
}
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -775,6 +775,85 @@ struct journal_head *jbd2_journal_get_de
return jbd2_journal_add_journal_head(bh);
}
+/*
+ * Return tid of the oldest transaction in the journal and block in the journal
+ * where the transaction starts.
+ *
+ * If the journal is now empty, return which will be the next transaction ID
+ * we will write and where will that transaction start.
+ *
+ * The return value is 0 if journal tail cannot be pushed any further, 1 if
+ * it can.
+ */
+int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
+ unsigned long *block)
+{
+ transaction_t *transaction;
+ int ret;
+
+ read_lock(&journal->j_state_lock);
+ spin_lock(&journal->j_list_lock);
+ transaction = journal->j_checkpoint_transactions;
+ if (transaction) {
+ *tid = transaction->t_tid;
+ *block = transaction->t_log_start;
+ } else if ((transaction = journal->j_committing_transaction) != NULL) {
+ *tid = transaction->t_tid;
+ *block = transaction->t_log_start;
+ } else if ((transaction = journal->j_running_transaction) != NULL) {
+ *tid = transaction->t_tid;
+ *block = journal->j_head;
+ } else {
+ *tid = journal->j_transaction_sequence;
+ *block = journal->j_head;
+ }
+ ret = tid_gt(*tid, journal->j_tail_sequence);
+ spin_unlock(&journal->j_list_lock);
+ read_unlock(&journal->j_state_lock);
+
+ return ret;
+}
+
+/*
+ * Update information in journal structure and in on disk journal superblock
+ * about log tail. This function does not check whether information passed in
+ * really pushes log tail further. It's responsibility of the caller to make
+ * sure provided log tail information is valid (e.g. by holding
+ * j_checkpoint_mutex all the time between computing log tail and calling this
+ * function as is the case with jbd2_cleanup_journal_tail()).
+ *
+ * Requires j_checkpoint_mutex
+ */
+void __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block)
+{
+ unsigned long freed;
+
+ BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
+
+ /*
+ * We cannot afford for write to remain in drive's caches since as
+ * soon as we update j_tail, next transaction can start reusing journal
+ * space and if we lose sb update during power failure we'd replay
+ * old transaction with possibly newly overwritten data.
+ */
+ jbd2_journal_update_sb_log_tail(journal, tid, block, WRITE_FUA);
+ write_lock(&journal->j_state_lock);
+ freed = block - journal->j_tail;
+ if (block < journal->j_tail)
+ freed += journal->j_last - journal->j_first;
+
+ trace_jbd2_update_log_tail(journal, tid, block, freed);
+ jbd_debug(1,
+ "Cleaning journal tail from %d to %d (offset %lu), "
+ "freeing %lu\n",
+ journal->j_tail_sequence, tid, block, freed);
+
+ journal->j_free += freed;
+ journal->j_tail_sequence = tid;
+ journal->j_tail = block;
+ write_unlock(&journal->j_state_lock);
+}
+
struct jbd2_stats_proc_session {
journal_t *journal;
struct transaction_stats_s *stats;
@@ -1156,16 +1235,28 @@ static int journal_reset(journal_t *jour
journal->j_errno);
journal->j_flags |= JBD2_FLUSHED;
} else {
- /* Add the dynamic fields and write it to disk. */
- jbd2_journal_update_sb_log_tail(journal);
+ /*
+ * Update log tail information. We use WRITE_FUA since new
+ * transaction will start reusing journal space and so we
+ * must make sure information about current log tail is on
+ * disk before that.
+ */
+ jbd2_journal_update_sb_log_tail(journal,
+ journal->j_tail_sequence,
+ journal->j_tail,
+ WRITE_FUA);
}
return jbd2_journal_start_thread(journal);
}
-static void jbd2_write_superblock(journal_t *journal)
+static void jbd2_write_superblock(journal_t *journal, int write_op)
{
struct buffer_head *bh = journal->j_sb_buffer;
+ int ret;
+ if (!(journal->j_flags & JBD2_BARRIER))
+ write_op &= ~(REQ_FUA | REQ_FLUSH);
+ lock_buffer(bh);
if (buffer_write_io_error(bh)) {
/*
* Oh, dear. A previous attempt to write the journal
@@ -1181,39 +1272,44 @@ static void jbd2_write_superblock(journa
clear_buffer_write_io_error(bh);
set_buffer_uptodate(bh);
}
-
- BUFFER_TRACE(bh, "marking dirty");
- mark_buffer_dirty(bh);
- sync_dirty_buffer(bh);
+ get_bh(bh);
+ bh->b_end_io = end_buffer_write_sync;
+ ret = submit_bh(write_op, bh);
+ wait_on_buffer(bh);
if (buffer_write_io_error(bh)) {
- printk(KERN_ERR "JBD2: I/O error detected "
- "when updating journal superblock for %s.\n",
- journal->j_devname);
clear_buffer_write_io_error(bh);
set_buffer_uptodate(bh);
+ ret = -EIO;
+ }
+ if (ret) {
+ printk(KERN_ERR "JBD2: Error %d detected when updating "
+ "journal superblock for %s.\n", ret,
+ journal->j_devname);
}
}
/**
* jbd2_journal_update_sb_log_tail() - Update log tail in journal sb on disk.
* @journal: The journal to update.
+ * @tail_tid: TID of the new transaction at the tail of the log
+ * @tail_block: The first block of the transaction at the tail of the log
+ * @write_op: With which operation should we write the journal sb
*
* Update a journal's superblock information about log tail and write it to
* disk, waiting for the IO to complete.
*/
-void jbd2_journal_update_sb_log_tail(journal_t *journal)
+void jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
+ unsigned long tail_block, int write_op)
{
journal_superblock_t *sb = journal->j_superblock;
- read_lock(&journal->j_state_lock);
- jbd_debug(1, "JBD2: updating superblock (start %ld, seq %d)\n",
- journal->j_tail, journal->j_tail_sequence);
+ jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
+ tail_block, tail_tid);
- sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
- sb->s_start = cpu_to_be32(journal->j_tail);
- read_unlock(&journal->j_state_lock);
+ sb->s_sequence = cpu_to_be32(tail_tid);
+ sb->s_start = cpu_to_be32(tail_block);
- jbd2_write_superblock(journal);
+ jbd2_write_superblock(journal, write_op);
/* Log is no longer empty */
write_lock(&journal->j_state_lock);
WARN_ON(!sb->s_sequence);
@@ -1240,7 +1336,7 @@ static void jbd2_mark_journal_empty(jour
sb->s_start = cpu_to_be32(0);
read_unlock(&journal->j_state_lock);
- jbd2_write_superblock(journal);
+ jbd2_write_superblock(journal, WRITE_FUA);
/* Log is no longer empty */
write_lock(&journal->j_state_lock);
@@ -1266,7 +1362,7 @@ static void jbd2_journal_update_sb_errno
sb->s_errno = cpu_to_be32(journal->j_errno);
read_unlock(&journal->j_state_lock);
- jbd2_write_superblock(journal);
+ jbd2_write_superblock(journal, WRITE_SYNC);
}
/*
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -21,6 +21,7 @@
#include <linux/jbd2.h>
#include <linux/errno.h>
#include <linux/crc32.h>
+#include <linux/blkdev.h>
#endif
/*
@@ -265,7 +266,9 @@ int jbd2_journal_recover(journal_t *jour
err2 = sync_blockdev(journal->j_fs_dev);
if (!err)
err = err2;
-
+ /* Make sure all replayed data is on permanent storage */
+ if (journal->j_flags & JBD2_BARRIER)
+ blkdev_issue_flush(journal->j_fs_dev, GFP_KERNEL, NULL);
return err;
}
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -972,6 +972,9 @@ extern void __journal_clean_data_list(tr
/* Log buffer allocation */
extern struct journal_head * jbd2_journal_get_descriptor_buffer(journal_t *);
int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
+int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
+ unsigned long *block);
+void __jbd2_update_log_tail(journal_t *journal, tid_t tid, unsigned long block);
/* Commit management */
extern void jbd2_journal_commit_transaction(journal_t *);
@@ -1083,7 +1086,8 @@ extern int jbd2_journal_destroy (j
extern int jbd2_journal_recover (journal_t *journal);
extern int jbd2_journal_wipe (journal_t *, int);
extern int jbd2_journal_skip_recovery (journal_t *);
-extern void jbd2_journal_update_sb_log_tail (journal_t *);
+extern void jbd2_journal_update_sb_log_tail (journal_t *, tid_t,
+ unsigned long, int);
extern void __jbd2_journal_abort_hard (journal_t *);
extern void jbd2_journal_abort (journal_t *, int);
extern int jbd2_journal_errno (journal_t *);
--- a/include/trace/events/jbd2.h
+++ b/include/trace/events/jbd2.h
@@ -200,7 +200,7 @@ TRACE_EVENT(jbd2_checkpoint_stats,
__entry->forced_to_close, __entry->written, __entry->dropped)
);
-TRACE_EVENT(jbd2_cleanup_journal_tail,
+TRACE_EVENT(jbd2_update_log_tail,
TP_PROTO(journal_t *journal, tid_t first_tid,
unsigned long block_nr, unsigned long freed),
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread