ext3: crossport ext3 sb_lock changes
cover ext3: crossport ext3 sb_lock changes
I ran into a deadlock in ext3 resize due to orphan list manipulation and filesystem resizing both wanting to grab the sb_lock, and stepping on each other.
This patch series backports 4 ext4 commits to ext3, pretty trivial changes to un-overload the superblock lock.
Thanks,
-Eric
PATCH 1/4 ext3: Remove outdated comment about lock_super()
From: Theodore Ts'o <[email protected]>
ext3_fill_super() is no longer called by read_super(), and it is no longer called with the superblock locked. The unlock_super()/lock_super() is no longer present, so this comment is entirely superfluous.
Crossport of 32ed5058ce90024efcd811254b4b1de0468099df
--- fs/ext3/super.c | 9 +-------- 1 files changed, 1 insertions(+), 8 deletions(-)
fs/ext3/super.c +1 -8
@@ -1974,14 +1974,7 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent) }ext3_setup_super (sb, es, sb->s_flags &MS_RDONLY ); -/* -* akpm: core read_super() calls in here with the superblock locked. -* That deadlocks, because orphan cleanup needs to lock the superblock -* in numerous places. Here we just pop the lock - it's relatively -* harmless, because we are now ready to accept write_super() requests, -* and aviro says that's the only reason for hanging onto the -* superblock lock. -*/ +EXT3_SB (sb)->s_mount_state |=EXT3_ORPHAN_FS ;ext3_orphan_cleanup (sb, es);EXT3_SB (sb)->s_mount_state &= ~EXT3_ORPHAN_FS ;
-- 1.6.0.6
PATCH 2/4 ext3: ext3_mark_recovery_complete() doesn't need to use lock_super
In reply to Eric SandeenFrom: Theodore Ts'o <[email protected]>
The function ext3_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. This also allows us to take out a very vile unlock_super()/lock_super() pair in ext3_remount().
Crossport of a63c9eb2ce6f5028da90f282798232c4f398ceb8
--- fs/ext3/super.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
fs/ext3/super.c +0 -8
@@ -2352,13 +2352,11 @@ static void ext3_mark_recovery_complete(struct super_block * sb,if (journal_flush (journal) <0 ) goto out; -lock_super (sb);if (EXT3_HAS_INCOMPAT_FEATURE (sb,EXT3_FEATURE_INCOMPAT_RECOVER ) && sb->s_flags &MS_RDONLY ) {EXT3_CLEAR_INCOMPAT_FEATURE (sb,EXT3_FEATURE_INCOMPAT_RECOVER );ext3_commit_super (sb, es,1 ); } -unlock_super (sb); out:journal_unlock_updates (journal);
@@ -2550,13 +2548,7 @@ static int ext3_remount (struct super_block * sb, int * flags, char * data) (sbi->s_mount_state & EXT3_VALID_FS)) es->s_state =cpu_to_le16 (sbi->s_mount_state); -/* -* We have to unlock super so that we can wait for -* transactions. -*/ -unlock_super (sb);ext3_mark_recovery_complete (sb, es); -lock_super (sb); } else { __le32 ret; if ((ret = EXT3_HAS_RO_COMPAT_FEATURE(sb,
-- 1.6.0.6
PATCH 3/4 ext3: Replace lock/unlock_super() with an explicit lock for the orphan list
In reply to Eric SandeenFrom: Theodore Ts'o <[email protected]>
Use a separate lock to protect the orphan list, so we can stop overloading the use of lock_super().
Crossport of 3b9d4ed26680771295d904a6b83e88e620780893
--- fs/ext3/namei.c | 20 +++++++++++--------- fs/ext3/super.c | 1 + include/linux/ext3_fs_sb.h | 1 + 3 files changed, 13 insertions(+), 9 deletions(-)
fs/ext3/namei.c +11 -9
@@ -1920,7 +1920,7 @@ int ext3_orphan_add(handle_t *handle, struct inode *inode)struct ext3_iloc iloc;int err =0 , rc; -lock_super (sb); +mutex_lock (&EXT3_SB (sb)->s_orphan_lock);if (!list_empty (&EXT3_I (inode)->i_orphan)) goto out_unlock;
@@ -1929,9 +1929,13 @@ int ext3_orphan_add(handle_t *handle, struct inode *inode)/* @@@ FIXME: Observation from aviro: * I think I can trigger J_ASSERT in ext3_orphan_add(). We block -* here (on lock_super()), so race with ext3_link() which might bump +* here (on s_orphan_lock), so race with ext3_link() which might bump * ->i_nlink. For, say it, character device. Not a regular file, * not a directory, not a symlink and ->i_nlink > 0. +* +* tytso, 4/25/2009: I'm not sure how that could happen; +* shouldn't the fs core protect us from these sort of +* unlink()/link() races? */ J_ASSERT ((S_ISREG (inode->i_mode) ||S_ISDIR (inode->i_mode) ||S_ISLNK (inode->i_mode)) || inode->i_nlink ==0 );
@@ -1968,7 +1972,7 @@ int ext3_orphan_add(handle_t *handle, struct inode *inode)jbd_debug (4 ,"orphan inode %lu will point to %d\n" , inode->i_ino,NEXT_ORPHAN (inode)); out_unlock: -unlock_super (sb); +mutex_unlock (&EXT3_SB (sb)->s_orphan_lock);ext3_std_error (inode->i_sb, err);return err; }
@@ -1986,11 +1990,9 @@ int ext3_orphan_del(handle_t *handle, struct inode *inode)struct ext3_iloc iloc;int err =0 ; -lock_super (inode->i_sb); -if (list_empty (&ei->i_orphan)) { -unlock_super (inode->i_sb); -return 0 ; - } +mutex_lock (&EXT3_SB (inode->i_sb)->s_orphan_lock); +if (list_empty (&ei->i_orphan)) + goto out; ino_next =NEXT_ORPHAN (inode); prev = ei->i_orphan.prev;
@@ -2040,7 +2042,7 @@ int ext3_orphan_del(handle_t *handle, struct inode *inode) out_err:ext3_std_error (inode->i_sb, err); out: -unlock_super (inode->i_sb); +mutex_unlock (&EXT3_SB (inode->i_sb)->s_orphan_lock);return err; out_brelse:
fs/ext3/super.c +1 -0
@@ -1890,6 +1890,7 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent) sb->dq_op = &ext3_quota_operations;#endif INIT_LIST_HEAD (&sbi->s_orphan);/* unlinked but open files */ +mutex_init (&sbi->s_orphan_lock); sb->s_root =NULL ;
include/linux/ext3_fs_sb.h +1 -0
@@ -72,6 +72,7 @@ struct ext3_sb_info {struct inode * s_journal_inode;struct journal_s * s_journal;struct list_head s_orphan; +struct mutex s_orphan_lock;unsigned long s_commit_interval;struct block_device *journal_bdev;#ifdef CONFIG_JBD_DEBUG
-- 1.6.0.6
PATCH 4/4 ext3: Replace lock/unlock_super() with an explicit lock for resizing
In reply to Eric SandeenFrom: Theodore Ts'o <[email protected]>
Use a separate lock to protect s_groups_count and the other block group descriptors which get changed via an on-line resize operation, so we can stop overloading the use of lock_super().
Crossport of 32ed5058ce90024efcd811254b4b1de0468099df
--- fs/ext3/resize.c | 35 ++++++++++++++++++----------------- fs/ext3/super.c | 1 + include/linux/ext3_fs_sb.h | 1 + 3 files changed, 20 insertions(+), 17 deletions(-)
fs/ext3/resize.c +18 -17
@@ -209,7 +209,7 @@ static int setup_new_group_blocks(struct super_block *sb,if (IS_ERR (handle))return PTR_ERR (handle); -lock_super (sb); +mutex_lock (&sbi->s_resize_lock);if (input->group != sbi->s_groups_count) { err = -EBUSY ; goto exit_journal;
@@ -324,7 +324,7 @@ exit_bh:brelse (bh); exit_journal: -unlock_super (sb); +mutex_unlock (&sbi->s_resize_lock);if ((err2 =ext3_journal_stop (handle)) && !err) err = err2;
@@ -662,11 +662,12 @@ exit_free:* important part is that the new block and inode counts are in the backup * superblocks, and the location of the new group metadata in the GDT backups. * -* We do not need lock_super() for this, because these blocks are not -* otherwise touched by the filesystem code when it is mounted. We don't -* need to worry about last changing from sbi->s_groups_count, because the -* worst that can happen is that we do not copy the full number of backups -* at this time. The resize which changed s_groups_count will backup again. +* We do not need take the s_resize_lock for this, because these +* blocks are not otherwise touched by the filesystem code when it is +* mounted. We don't need to worry about last changing from +* sbi->s_groups_count, because the worst that can happen is that we +* do not copy the full number of backups at this time. The resize +* which changed s_groups_count will backup again. */ static void update_backups (struct super_block *sb,int blk_off,char *data,int size)
@@ -825,7 +826,7 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input) goto exit_put; } -lock_super (sb); +mutex_lock (&sbi->s_resize_lock);if (input->group != sbi->s_groups_count) {ext3_warning (sb, __func__,"multiple resizers run on filesystem!" );
@@ -856,7 +857,7 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input)/* * OK, now we've set up the new group. Time to make it active. * -* Current kernels don't lock all allocations via lock_super(), +* We do not lock all allocations via s_resize_lock * so we have to be safe wrt. concurrent accesses the group * data. So we need to be careful to set all of the relevant * group descriptor data etc. *before* we enable the group.
@@ -900,12 +901,12 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input)* * The precise rules we use are: * -* * Writers of s_groups_count *must* hold lock_super +* * Writers of s_groups_count *must* hold s_resize_lock * AND * * Writers must perform a smp_wmb() after updating all dependent * data and before modifying the groups count * -* * Readers must hold lock_super() over the access +* * Readers must hold s_resize_lock over the access * OR * * Readers must perform an smp_rmb() after reading the groups count * and before reading any dependent data.
@@ -936,7 +937,7 @@ int ext3_group_add(struct super_block *sb, struct ext3_new_group_data *input)ext3_journal_dirty_metadata (handle, sbi->s_sbh); exit_journal: -unlock_super (sb); +mutex_unlock (&sbi->s_resize_lock);if ((err2 =ext3_journal_stop (handle)) && !err) err = err2;if (!err) {
@@ -973,7 +974,7 @@ int ext3_group_extend(struct super_block *sb, struct ext3_super_block *es,/* We don't need to worry about locking wrt other resizers just * yet: we're going to revalidate es->s_blocks_count after -* taking lock_super() below. */ +* taking the s_resize_lock below. */ o_blocks_count =le32_to_cpu (es->s_blocks_count); o_groups_count =EXT3_SB (sb)->s_groups_count;
@@ -1045,11 +1046,11 @@ int ext3_group_extend(struct super_block *sb, struct ext3_super_block *es, goto exit_put; } -lock_super (sb); +mutex_lock (&EXT3_SB (sb)->s_resize_lock);if (o_blocks_count !=le32_to_cpu (es->s_blocks_count)) {ext3_warning (sb, __func__,"multiple resizers run on filesystem!" ); -unlock_super (sb); +mutex_unlock (&EXT3_SB (sb)->s_resize_lock);ext3_journal_stop (handle); err = -EBUSY ; goto exit_put;
@@ -1059,13 +1060,13 @@ int ext3_group_extend(struct super_block *sb, struct ext3_super_block *es, EXT3_SB(sb)->s_sbh))) {ext3_warning (sb, __func__,"error %d on journal write access" , err); -unlock_super (sb); +mutex_unlock (&EXT3_SB (sb)->s_resize_lock);ext3_journal_stop (handle); goto exit_put; } es->s_blocks_count =cpu_to_le32 (o_blocks_count + add);ext3_journal_dirty_metadata (handle,EXT3_SB (sb)->s_sbh); -unlock_super (sb); +mutex_unlock (&EXT3_SB (sb)->s_resize_lock);ext3_debug ("freeing blocks %lu through " E3FSBLK "\n" , o_blocks_count, o_blocks_count + add);ext3_free_blocks_sb (handle, sb, o_blocks_count, add, &freed_blocks);
fs/ext3/super.c +1 -0
@@ -1891,6 +1891,7 @@ static int ext3_fill_super (struct super_block *sb, void *data, int silent)#endif INIT_LIST_HEAD (&sbi->s_orphan);/* unlinked but open files */ mutex_init (&sbi->s_orphan_lock); +mutex_init (&sbi->s_resize_lock); sb->s_root =NULL ;
include/linux/ext3_fs_sb.h +1 -0
@@ -73,6 +73,7 @@ struct ext3_sb_info {struct journal_s * s_journal;struct list_head s_orphan;struct mutex s_orphan_lock; +struct mutex s_resize_lock;unsigned long s_commit_interval;struct block_device *journal_bdev;#ifdef CONFIG_JBD_DEBUG
-- 1.6.0.6