From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zerg.uk ([108.61.210.227]:55847 "EHLO zerg.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755837AbbI0OzM (ORCPT ); Sun, 27 Sep 2015 10:55:12 -0400 From: Bartosz Kwitniewski To: Ben Hutchings Reply-To: zerg2000@astral.org.pl Cc: stable , Jan Kara Subject: Re: [PATCH 3.2.x] jbd2: add mutex_lock on j_checkpoint_mutex in jbd2_journal_flush Date: Sun, 27 Sep 2015 16:47:06 +0200 Message-ID: <4349260.T86D35KoAB@zealot> In-Reply-To: <1443361449.2004.20.camel@decadent.org.uk> References: <1668256.Geort4rEnn@zealot> <1443361449.2004.20.camel@decadent.org.uk> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart5084046.glkZRHJ5D2"; micalg="pgp-sha1"; protocol="application/pgp-signature" Sender: stable-owner@vger.kernel.org List-ID: --nextPart5084046.glkZRHJ5D2 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="us-ascii" 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 locki= ng of > > j_checkpoint_mutex in jbd2_journal_flush was not backported from up= stream. >=20 > Oops. >=20 > > 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 > >=20 > > Signed-off-by: Bartosz Kwitniewski > > Cc: stable@vger.kernel.org > > --- > > --- fs/jbd2/journal.c.orig> =09> 2015-08-12 16:33:24.000000000 +020= 0 > > +++ fs/jbd2/journal.c> =09> 2015-09-06 00:57:56.890894891 +0200 > > @@ -1828,10 +1828,13 @@ int jbd2_journal_flush(journal_t *journa > >=20 > > > =09> if (is_journal_aborted(journal)) > > > =09>=20 > > > =09> =09> return -EIO; > >=20 > > +> =09> mutex_lock(&journal->j_checkpoint_mutex); > >=20 > > > =09> if (!err) { > > > =09>=20 > > > =09> =09> err =3D jbd2_cleanup_journal_tail(journal); > >=20 > > -> =09> =09> if (err < 0) > > +> =09> =09> if (err < 0) { > > +> =09> =09> =09> mutex_unlock(&journal->j_checkpoint_mutex); > >=20 > > > =09> =09> =09> goto out; > >=20 > > +> =09> =09> } > >=20 > > > =09> =09> err =3D 0; > > > =09>=20 > > > =09> } > >=20 > > @@ -1841,6 +1844,7 @@ int jbd2_journal_flush(journal_t *journa > >=20 > > > =09> * commits of data to the journal will restore the current > > > =09> * s_start value. */ > > > =09>=20 > > > =09> jbd2_mark_journal_empty(journal); > >=20 > > +> =09> mutex_unlock(&journal->j_checkpoint_mutex); > >=20 > > > =09> write_lock(&journal->j_state_lock); > > > =09> J_ASSERT(!journal->j_running_transaction); > > > =09> J_ASSERT(!journal->j_committing_transaction); >=20 > Why is it sufficient to add locking of j_checkpoint_mutex only in thi= s > one function? >=20 > Shouldn't I cherry-pick commits 24bcc89c7e7c ("jbd2: split updating o= f > journal superblock and marking journal empty") and a78bb11d7acd ("jbd= 2: > protect all log tail updates with j_checkpoint_mutex") as well? >=20 > Ben. Hello! That's right, locking of j_checkpoint_mutex only in jbd2_journal_flush(= ) is not=20 enough. I have rushed with this patch without doing a deeper analysis, = I'm=20 sorry. =2D-=20 Bartosz Kwitniewski --nextPart5084046.glkZRHJ5D2 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEABECAAYFAlYIAX0ACgkQKtyxGvWngncOEgCfR0egaXruuV58uT54yR7Yrsu8 ImEAn1f2nyMaE6B+Z40nnzGYje16OTXn =AX8w -----END PGP SIGNATURE----- --nextPart5084046.glkZRHJ5D2--