* [PATCH] ext4_freeze: don't return to userspace with a mutex held
@ 2010-03-29 22:34 Eric Sandeen
2010-04-04 20:52 ` tytso
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2010-03-29 22:34 UTC (permalink / raw)
To: ext4 development
This is for RH bug 568503 -
snapshot bug: lock held when returning to user space
ext4_freeze() does jbd2_journal_lock_updates() which takes
the j_barrier mutex, and then we return to userspace. The
kernel does not like this:
================================================
[ BUG: lock held when returning to user space! ]
------------------------------------------------
lvcreate/1075 is leaving the kernel with locks still held!
1 lock held by lvcreate/1075:
#0: (&journal->j_barrier){+.+...}, at: [<ffffffff811c6214>]
jbd2_journal_lock_updates+0xe1/0xf0
I don't -think- we need to do this; by now we should have s_frozen
set, and nobody else should be coming down the pipe to get to
the journal. However, just to be on the safe side, I added
a couple of vfs_check_frozen() calls in ext4 functions which will
arrive at start_this_handle(), which should ensure that we never
get any journal traffic generated while frozen.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
(ext3 will need similar changes if this patch passes
muster on review
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index e14d22c..00d09f5 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -241,6 +241,7 @@ handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
if (sb->s_flags & MS_RDONLY)
return ERR_PTR(-EROFS);
+ vfs_check_frozen(sb, SB_FREEZE_WRITE);
/* Special case here: if the journal has aborted behind our
* backs (eg. EIO in the commit thread), then we still need to
* take the FS itself readonly cleanly. */
@@ -3485,8 +3486,10 @@ int ext4_force_commit(struct super_block *sb)
return 0;
journal = EXT4_SB(sb)->s_journal;
- if (journal)
+ if (journal) {
+ vfs_check_frozen(sb, SB_FREEZE_WRITE);
ret = ext4_journal_force_commit(journal);
+ }
return ret;
}
@@ -3535,18 +3538,16 @@ static int ext4_freeze(struct super_block *sb)
* the journal.
*/
error = jbd2_journal_flush(journal);
- if (error < 0) {
- out:
- jbd2_journal_unlock_updates(journal);
- return error;
- }
+ if (error < 0)
+ goto out;
/* Journal blocked and flushed, clear needs_recovery flag. */
EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
error = ext4_commit_super(sb, 1);
- if (error)
- goto out;
- return 0;
+out:
+ /* we rely on s_frozen to stop further updates */
+ jbd2_journal_unlock_updates(EXT4_SB(sb)->s_journal);
+ return error;
}
/*
@@ -3563,7 +3564,6 @@ static int ext4_unfreeze(struct super_block *sb)
EXT4_SET_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER);
ext4_commit_super(sb, 1);
unlock_super(sb);
- jbd2_journal_unlock_updates(EXT4_SB(sb)->s_journal);
return 0;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ext4_freeze: don't return to userspace with a mutex held
2010-03-29 22:34 [PATCH] ext4_freeze: don't return to userspace with a mutex held Eric Sandeen
@ 2010-04-04 20:52 ` tytso
2010-04-04 21:03 ` tytso
2010-04-04 21:23 ` Eric Sandeen
0 siblings, 2 replies; 6+ messages in thread
From: tytso @ 2010-04-04 20:52 UTC (permalink / raw)
To: Eric Sandeen; +Cc: ext4 development
On Mon, Mar 29, 2010 at 05:34:43PM -0500, Eric Sandeen wrote:
>
> I don't -think- we need to do this; by now we should have s_frozen
> set, and nobody else should be coming down the pipe to get to
> the journal. However, just to be on the safe side, I added
> a couple of vfs_check_frozen() calls in ext4 functions which will
> arrive at start_this_handle(), which should ensure that we never
> get any journal traffic generated while frozen.
Um, I think the addition of vfs_check_frozen(), esp. to
ext4_journal_start_sb() is absolutely necessary. What else do we have
to prevent filesystem modifications from going to the file systme
layer? I didn't see anything in the VFS layer that checks s_frozen;
am I missing something?
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4_freeze: don't return to userspace with a mutex held
2010-04-04 20:52 ` tytso
@ 2010-04-04 21:03 ` tytso
2010-04-04 21:23 ` Eric Sandeen
1 sibling, 0 replies; 6+ messages in thread
From: tytso @ 2010-04-04 21:03 UTC (permalink / raw)
To: Eric Sandeen; +Cc: ext4 development
On Sun, Apr 04, 2010 at 04:52:37PM -0400, tytso@MIT.EDU wrote:
> On Mon, Mar 29, 2010 at 05:34:43PM -0500, Eric Sandeen wrote:
> >
> > I don't -think- we need to do this; by now we should have s_frozen
> > set, and nobody else should be coming down the pipe to get to
> > the journal. However, just to be on the safe side, I added
> > a couple of vfs_check_frozen() calls in ext4 functions which will
> > arrive at start_this_handle(), which should ensure that we never
> > get any journal traffic generated while frozen.
>
> Um, I think the addition of vfs_check_frozen(), esp. to
> ext4_journal_start_sb() is absolutely necessary. What else do we have
> to prevent filesystem modifications from going to the file systme
> layer? I didn't see anything in the VFS layer that checks s_frozen;
> am I missing something?
Added to the ext4 patch queue, with the following patch comment:
ext4: don't return to userspace after freezing the fs with a mutex held
ext4_freeze() used jbd2_journal_lock_updates() which takes
the j_barrier mutex, and then returns to userspace. The
kernel does not like this:
================================================
[ BUG: lock held when returning to user space! ]
------------------------------------------------
lvcreate/1075 is leaving the kernel with locks still held!
1 lock held by lvcreate/1075:
#0: (&journal->j_barrier){+.+...}, at: [<ffffffff811c6214>]
jbd2_journal_lock_updates+0xe1/0xf0
Use vfs_check_frozen() added to ext4_journal_start_sb() and
ext4_force_commit() instead.
Addresses-Red-Hat-Bugzilla: #568503
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4_freeze: don't return to userspace with a mutex held
2010-04-04 20:52 ` tytso
2010-04-04 21:03 ` tytso
@ 2010-04-04 21:23 ` Eric Sandeen
2010-04-04 21:56 ` Theodore Tso
1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2010-04-04 21:23 UTC (permalink / raw)
To: tytso; +Cc: ext4 development
tytso@mit.edu wrote:
> On Mon, Mar 29, 2010 at 05:34:43PM -0500, Eric Sandeen wrote:
>> I don't -think- we need to do this; by now we should have s_frozen
>> set, and nobody else should be coming down the pipe to get to
>> the journal. However, just to be on the safe side, I added
>> a couple of vfs_check_frozen() calls in ext4 functions which will
>> arrive at start_this_handle(), which should ensure that we never
>> get any journal traffic generated while frozen.
>
> Um, I think the addition of vfs_check_frozen(), esp. to
> ext4_journal_start_sb() is absolutely necessary. What else do we have
> to prevent filesystem modifications from going to the file systme
> layer? I didn't see anything in the VFS layer that checks s_frozen;
> am I missing something?
>
> - Ted
Well, there is __generic_file_aio_write doing vfs_check_frozen, but
I thought there was more at the vfs level to stop things from getting
to the filesystem... *shrug* I see you put the patch in as sent,
it sounds right to me.
Thanks,
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4_freeze: don't return to userspace with a mutex held
2010-04-04 21:23 ` Eric Sandeen
@ 2010-04-04 21:56 ` Theodore Tso
2010-04-05 0:43 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Theodore Tso @ 2010-04-04 21:56 UTC (permalink / raw)
To: Eric Sandeen; +Cc: ext4 development
On Apr 4, 2010, at 5:23 PM, Eric Sandeen wrote:
>
> Well, there is __generic_file_aio_write doing vfs_check_frozen, but
> I thought there was more at the vfs level to stop things from getting
> to the filesystem... *shrug* I see you put the patch in as sent,
> it sounds right to me.
Yeah, it looks safe but it would be nice to do some testing to make sure there's nothing we missed.
I'm also vaguely worried whether the right thing will happen in no-journal mode, but it looks like it's better than what we had before (which clearly would have never worked in no journal mode), so I decided to let it pass.
Testing the freeze functionality definitely needs to be done before we trust what happens in no journal mode, for sure.
-- Ted
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4_freeze: don't return to userspace with a mutex held
2010-04-04 21:56 ` Theodore Tso
@ 2010-04-05 0:43 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2010-04-05 0:43 UTC (permalink / raw)
To: Theodore Tso; +Cc: ext4 development
Theodore Tso wrote:
> On Apr 4, 2010, at 5:23 PM, Eric Sandeen wrote:
>> Well, there is __generic_file_aio_write doing vfs_check_frozen, but
>> I thought there was more at the vfs level to stop things from
>> getting to the filesystem... *shrug* I see you put the patch in as
>> sent, it sounds right to me.
>
> Yeah, it looks safe but it would be nice to do some testing to make
> sure there's nothing we missed.
>
> I'm also vaguely worried whether the right thing will happen in
> no-journal mode, but it looks like it's better than what we had
> before (which clearly would have never worked in no journal mode), so
> I decided to let it pass.
I did go back up the callchain from where the old lock should have
caught it, so I think I've plugged the same holes that were protected
before.
> Testing the freeze functionality definitely needs to be done before
> we trust what happens in no journal mode, for sure.
Oh good point, hadn't even thought about nojournal mode & freeze.
Always nice to "accidentally" fix things ;)
-Eric
>
> -- Ted
>
> -- To unsubscribe from this list: send the line "unsubscribe
> linux-ext4" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-05 0:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-29 22:34 [PATCH] ext4_freeze: don't return to userspace with a mutex held Eric Sandeen
2010-04-04 20:52 ` tytso
2010-04-04 21:03 ` tytso
2010-04-04 21:23 ` Eric Sandeen
2010-04-04 21:56 ` Theodore Tso
2010-04-05 0:43 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).