| Message ID | BANLkTikqLrAyVaULk5UQsM7TzGk73ihsKg@mail.gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2011-04-04, at 9:47 PM, Yongqiang Yang wrote: > handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) > { > + handle = ext4_journal_current_handle(); > + > + /* > + * Before vfs_check_frozen(), the current handle should be allowed > + * to finish, otherwise deadlock would happen when the filesystem > + * is freezed && active handles are not stopped. > + */ > + if (!journal) { > + if (!handle) > + vfs_check_frozen(sb, SB_FREEZE_TRANS); > + return ext4_get_nojournal(); > } > + > + if (!handle) > + vfs_check_frozen(sb, SB_FREEZE_TRANS); This is probably easier to read by putting the !handle check/code together, with a minor fixup to the comment as well: /* * If a handle has already been started, it should be allowed to * finish, otherwise deadlock could happen between freeze and * truncate due to the restart of the journal handle if the * filesystem is frozen and active handles are not stopped. */ if (!handle) { vfs_check_frozen(sb, SB_FREEZE_TRANS); if (!journal) return ext4_get_nojournal(); } /* * 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. */ if (is_journal_aborted(journal)) { ext4_abort(sb, "Detected aborted journal"); return ERR_PTR(-EROFS); } return jbd2_journal_start(journal, nblocks); } > /* > @@ -4131,6 +4149,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait) > /* > * LVM calls this function before a (read-only) snapshot is created. This > * gives us a chance to flush the journal completely and mark the fs clean. > + * > + * Note that only this function cannot bring a filesystem to be in a clean > + * state independently, because ext4 prevents a new handle from being started > + * by @sb->s_frozen, which stays in an upper layer. It thus needs help from > + * the upper layer. > */ > static int ext4_freeze(struct super_block *sb) > { > -- > 1.7.4 > -- > 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 Cheers, Andreas -- 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
On Tue 05-04-11 15:47:25, Yongqiang Yang wrote: > v0->v1: > check s_frozen in nojournal case only if there is no an active handle. > get reference to an active handle by jbd2_journal_start(). > > ext4_journal_start_sb() should not prevent an active handle from being > started due to s_frozen. Otherwise, deadlock is easy to happen, below > is a situation. > > ================================================ > freeze | truncate > ================================================ > | ext4_ext_truncate() > freeze_super() | starts a handle > sets s_frozen | > | ext4_ext_truncate() > | holds i_data_sem > ext4_freeze() | > waits for updates | > | ext4_free_blocks() > | calls dquot_free_block() > | > | dquot_free_blocks() > | calls ext4_dirty_inode() > | > | ext4_dirty_inode() > | trys to start an active > | handle > | > | block due to s_frozen > ================================================ > > Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> > Reported-by: Amir Goldstein <amir73il@users.sf.net> > Reviewed-by: Jan Kara <jack@suse.cz> The usual workflow is that you add 'Reviewed-by' tag to the patch only after you send a version of a patch the person agrees to and explicitely says you can add a tag - for example I write something like "You can add Reviewed-by: Jan Kara <jack@suse.cz>" when I agree to the tag being added. > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index ccfa686..53920bd 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle) > * journal_end calls result in the superblock being marked dirty, so > * that sync() will call the filesystem's write_super callback if > * appropriate. > + * > + * To avoid j_barrier hold in userspace when a user calls freeze(), > + * ext4 prevents a new handle from being started by s_frozen, which > + * is in an upper layer. > */ > handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) > { > journal_t *journal; > + handle_t *handle; > > if (sb->s_flags & MS_RDONLY) > return ERR_PTR(-EROFS); > > - vfs_check_frozen(sb, SB_FREEZE_TRANS); > - /* 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. */ > journal = EXT4_SB(sb)->s_journal; > - if (journal) { > - if (is_journal_aborted(journal)) { > - ext4_abort(sb, "Detected aborted journal"); > - return ERR_PTR(-EROFS); > - } > - return jbd2_journal_start(journal, nblocks); > + handle = ext4_journal_current_handle(); > + > + /* > + * Before vfs_check_frozen(), the current handle should be allowed > + * to finish, otherwise deadlock would happen when the filesystem > + * is freezed && active handles are not stopped. > + */ > + if (!journal) { > + if (!handle) > + vfs_check_frozen(sb, SB_FREEZE_TRANS); > + return ext4_get_nojournal(); > } > - return ext4_get_nojournal(); > + > + if (!handle) > + vfs_check_frozen(sb, SB_FREEZE_TRANS); This test is the same for 'nojournal' case so you can move it before the !journal test. Otherwise the patch looks OK to me. So now you can add Reviewed-by: Jan Kara <jack@suse.cz> line :) Honza
On Wed, Apr 6, 2011 at 6:26 AM, Jan Kara <jack@suse.cz> wrote: > On Tue 05-04-11 15:47:25, Yongqiang Yang wrote: >> v0->v1: >> check s_frozen in nojournal case only if there is no an active handle. >> get reference to an active handle by jbd2_journal_start(). >> >> ext4_journal_start_sb() should not prevent an active handle from being >> started due to s_frozen. Otherwise, deadlock is easy to happen, below >> is a situation. >> >> ================================================ >> freeze | truncate >> ================================================ >> | ext4_ext_truncate() >> freeze_super() | starts a handle >> sets s_frozen | >> | ext4_ext_truncate() >> | holds i_data_sem >> ext4_freeze() | >> waits for updates | >> | ext4_free_blocks() >> | calls dquot_free_block() >> | >> | dquot_free_blocks() >> | calls ext4_dirty_inode() >> | >> | ext4_dirty_inode() >> | trys to start an active >> | handle >> | >> | block due to s_frozen >> ================================================ >> >> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> >> Reported-by: Amir Goldstein <amir73il@users.sf.net> >> Reviewed-by: Jan Kara <jack@suse.cz> > The usual workflow is that you add 'Reviewed-by' tag to the patch only > after you send a version of a patch the person agrees to and explicitely > says you can add a tag - for example I write something like "You can add > Reviewed-by: Jan Kara <jack@suse.cz>" > when I agree to the tag being added. I see. Thank you. > >> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >> index ccfa686..53920bd 100644 >> --- a/fs/ext4/super.c >> +++ b/fs/ext4/super.c >> @@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle) >> * journal_end calls result in the superblock being marked dirty, so >> * that sync() will call the filesystem's write_super callback if >> * appropriate. >> + * >> + * To avoid j_barrier hold in userspace when a user calls freeze(), >> + * ext4 prevents a new handle from being started by s_frozen, which >> + * is in an upper layer. >> */ >> handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks) >> { >> journal_t *journal; >> + handle_t *handle; >> >> if (sb->s_flags & MS_RDONLY) >> return ERR_PTR(-EROFS); >> >> - vfs_check_frozen(sb, SB_FREEZE_TRANS); >> - /* 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. */ >> journal = EXT4_SB(sb)->s_journal; >> - if (journal) { >> - if (is_journal_aborted(journal)) { >> - ext4_abort(sb, "Detected aborted journal"); >> - return ERR_PTR(-EROFS); >> - } >> - return jbd2_journal_start(journal, nblocks); >> + handle = ext4_journal_current_handle(); >> + >> + /* >> + * Before vfs_check_frozen(), the current handle should be allowed >> + * to finish, otherwise deadlock would happen when the filesystem >> + * is freezed && active handles are not stopped. >> + */ >> + if (!journal) { >> + if (!handle) >> + vfs_check_frozen(sb, SB_FREEZE_TRANS); >> + return ext4_get_nojournal(); >> } >> - return ext4_get_nojournal(); >> + >> + if (!handle) >> + vfs_check_frozen(sb, SB_FREEZE_TRANS); > This test is the same for 'nojournal' case so you can move it before the > !journal test. Otherwise the patch looks OK to me. So now you can add > Reviewed-by: Jan Kara <jack@suse.cz> > line :) > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR >
================================================
freeze | truncate
================================================
| ext4_ext_truncate()
freeze_super() | starts a handle
sets s_frozen |
| ext4_ext_truncate()
| holds i_data_sem
ext4_freeze() |
waits for updates |
| ext4_free_blocks()
| calls dquot_free_block()
|
| dquot_free_blocks()
| calls ext4_dirty_inode()
|
| ext4_dirty_inode()
| trys to start an active
| handle
|
| block due to s_frozen
================================================
Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com>
Reported-by: Amir Goldstein <amir73il@users.sf.net>
Reviewed-by: Jan Kara <jack@suse.cz>
---
fs/ext4/super.c | 45 ++++++++++++++++++++++++++++++++++-----------
1 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index ccfa686..53920bd 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -242,27 +242,45 @@ static void ext4_put_nojournal(handle_t *handle)
* journal_end calls result in the superblock being marked dirty, so
* that sync() will call the filesystem's write_super callback if
* appropriate.
+ *
+ * To avoid j_barrier hold in userspace when a user calls freeze(),
+ * ext4 prevents a new handle from being started by s_frozen, which
+ * is in an upper layer.
*/
handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks)
{
journal_t *journal;
+ handle_t *handle;
if (sb->s_flags & MS_RDONLY)
return ERR_PTR(-EROFS);
- vfs_check_frozen(sb, SB_FREEZE_TRANS);
- /* 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. */
journal = EXT4_SB(sb)->s_journal;
- if (journal) {
- if (is_journal_aborted(journal)) {
- ext4_abort(sb, "Detected aborted journal");
- return ERR_PTR(-EROFS);
- }
- return jbd2_journal_start(journal, nblocks);
+ handle = ext4_journal_current_handle();
+
+ /*
+ * Before vfs_check_frozen(), the current handle should be allowed
+ * to finish, otherwise deadlock would happen when the filesystem
+ * is freezed && active handles are not stopped.
+ */
+ if (!journal) {
+ if (!handle)
+ vfs_check_frozen(sb, SB_FREEZE_TRANS);
+ return ext4_get_nojournal();
}
- return ext4_get_nojournal();
+
+ if (!handle)
+ vfs_check_frozen(sb, SB_FREEZE_TRANS);
+ /*
+ * 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.
+ */
+ if (is_journal_aborted(journal)) {
+ ext4_abort(sb, "Detected aborted journal");
+ return ERR_PTR(-EROFS);
+ }
+ return jbd2_journal_start(journal, nblocks);
}
/*
@@ -4131,6 +4149,11 @@ static int ext4_sync_fs(struct super_block *sb, int wait)
/*
* LVM calls this function before a (read-only) snapshot is created. This
* gives us a chance to flush the journal completely and mark the fs clean.
+ *
+ * Note that only this function cannot bring a filesystem to be in a clean
+ * state independently, because ext4 prevents a new handle from being started
+ * by @sb->s_frozen, which stays in an upper layer. It thus needs help from
+ * the upper layer.
*/
static int ext4_freeze(struct super_block *sb)
{