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

[Jan Kara <jack@suse.cz>]

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") 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.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR