On Sunday, September 27, 2015 02:44:09 PM 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@xxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > --- 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") and a78bb11d7acd ("jbd2:
> protect all log tail updates with j_checkpoint_mutex") as well?
>
> Ben.
Hello!
That's right, locking of j_checkpoint_mutex only in jbd2_journal_flush() is not
enough. I have rushed with this patch without doing a deeper analysis, I'm
sorry.
--
Bartosz Kwitniewski
Attachment:
signature.asc
Description: This is a digitally signed message part.