| Message ID | 1240717765-16572-4-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sat, Apr 25, 2009 at 11:49:23PM -0400, Theodore Ts'o wrote: > The function ext4_mark_recovery_complete() is called from two call > paths: either (a) while mounting the filesystem, in which case there's > no danger of any other CPU calling write_super() until the mount is > completed, and (b) while remounting the filesystem read-write, in > which case the fs core has already locked the superblock, and in any > case write_super() wouldn't be called until the filesystem is > successfully changed from being mounted read-only to read-write. Currently ext4_remount releases/reqacquires lock_super around ext4_mark_recovery_complete, and unfortunately currently ->write_super can be called on a r/o filesystem (that's why we have the MS_RDONLY checks in all instance, I plan to clean that mess up). So for now I would keep that instance of lock_super, it'll also serve as documentation for the locking requirements once we pushed down lock_super to the filesystem in ->write_super and ->remount_fs so that it can eventually be replaced with an ext4-local lock. -- 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 Sun, Apr 26, 2009 at 03:07:14AM -0400, Christoph Hellwig wrote: > On Sat, Apr 25, 2009 at 11:49:23PM -0400, Theodore Ts'o wrote: > > The function ext4_mark_recovery_complete() is called from two call > > paths: either (a) while mounting the filesystem, in which case there's > > no danger of any other CPU calling write_super() until the mount is > > completed, and (b) while remounting the filesystem read-write, in > > which case the fs core has already locked the superblock, and in any > > case write_super() wouldn't be called until the filesystem is > > successfully changed from being mounted read-only to read-write. > > Currently ext4_remount releases/reqacquires lock_super around > ext4_mark_recovery_complete, and unfortunately currently ->write_super > can be called on a r/o filesystem (that's why we have the MS_RDONLY > checks in all instance, I plan to clean that mess up). That's true, but the patch also takes out the release/reacquire in in ext4_remount (which was particularly ugly, belch). So even if write_super gets called on an r/o filesystem (why?!?), we should be safe because remount will hold lock_super() throughout the entire remount operation. We could delay this cleanup until you clean the mess with write_super, but I don't think it would be harmful in removing the lock_super()/unlock_super() pair in ext4_mark_recovery_complete(), and the unlock_super()/lock_super() pair in ext4_remount before then. Am I missing something? - 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
On Sun, Apr 26, 2009 at 07:46:08AM -0400, Theodore Tso wrote: > That's true, but the patch also takes out the release/reacquire in in > ext4_remount (which was particularly ugly, belch). Sorry, missed the second hunk of the patch. > So even if > write_super gets called on an r/o filesystem (why?!?), No good reason really. Hopefully we'll sort all that out soon. > we should be > safe because remount will hold lock_super() throughout the entire > remount operation. > > We could delay this cleanup until you clean the mess with write_super, > but I don't think it would be harmful in removing the > lock_super()/unlock_super() pair in ext4_mark_recovery_complete(), and > the unlock_super()/lock_super() pair in ext4_remount before then. Am > I missing something? No, I was just missing the second hunk of the patch. -- 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
diff --git a/fs/ext4/super.c b/fs/ext4/super.c index e0b0c9f..176d43f 100644 --- a/fs/ext4/super.c +++ b/fs/ext4/super.c @@ -3198,14 +3198,12 @@ static void ext4_mark_recovery_complete(struct super_block *sb, if (jbd2_journal_flush(journal) < 0) goto out; - lock_super(sb); if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER) && sb->s_flags & MS_RDONLY) { EXT4_CLEAR_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_RECOVER); sb->s_dirt = 0; ext4_commit_super(sb, es, 1); } - unlock_super(sb); out: jbd2_journal_unlock_updates(journal); @@ -3438,15 +3436,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data) (sbi->s_mount_state & EXT4_VALID_FS)) es->s_state = cpu_to_le16(sbi->s_mount_state); - /* - * We have to unlock super so that we can wait for - * transactions. - */ - if (sbi->s_journal) { - unlock_super(sb); + if (sbi->s_journal) ext4_mark_recovery_complete(sb, es); - lock_super(sb); - } } else { int ret; if ((ret = EXT4_HAS_RO_COMPAT_FEATURE(sb,
The function ext4_mark_recovery_complete() is called from two call paths: either (a) while mounting the filesystem, in which case there's no danger of any other CPU calling write_super() until the mount is completed, and (b) while remounting the filesystem read-write, in which case the fs core has already locked the superblock, and in any case write_super() wouldn't be called until the filesystem is successfully changed from being mounted read-only to read-write. Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- fs/ext4/super.c | 11 +---------- 1 files changed, 1 insertions(+), 10 deletions(-)