Re: [PATCH 3.2.x] jbd2: add mutex_lock on j_checkpoint_mutex in jbd2_journal_flush

[Bartosz Kwitniewski <zerg2000@astral.org.pl>]

[-- Attachment #1: Type: text/plain, Size: 3060 bytes --]

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@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") 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 #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 181 bytes --]