[PATCH 2/2] ->write_super lock_super pushdown

[PATCH 2/2] ->write_super lock_super pushdown

[Christoph Hellwig]

Only four filesystems have both a ->write_super method and use lock_super
internally: ext4, fat, sysv, ufs.  Push down lock_super into these
filesystems and remove it from the caller.  Add a get_fs_excl/put_fs_excl
pair to sync_super for now to keep the current behaviour in that
respect.  Calling it a second time from write_super is fine as it's
only checked for beeing non-zero.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/ext4/super.c
===================================================================
--- vfs-2.6.orig/fs/ext4/super.c	2009-05-06 21:35:40.923658961 +0200
+++ vfs-2.6/fs/ext4/super.c	2009-05-06 21:37:07.337661434 +0200
@@ -66,6 +66,7 @@ static int ext4_remount(struct super_blo
 static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf);
 static int ext4_unfreeze(struct super_block *sb);
 static void ext4_write_super(struct super_block *sb);
+static void ext4_write_super_locked(struct super_block *sb);
 static int ext4_freeze(struct super_block *sb);
 
 
@@ -566,7 +567,7 @@ static void ext4_put_super(struct super_
 	lock_super(sb);
 	lock_kernel();
 	if (sb->s_dirt)
-		ext4_write_super(sb);
+		ext4_write_super_locked(sb);
 
 	ext4_mb_release(sb);
 	ext4_ext_release(sb);
@@ -3273,7 +3274,7 @@ int ext4_force_commit(struct super_block
  * point.  (We can probably nuke this function altogether, and remove
  * any mention to sb->s_dirt in all of fs/ext4; eventual cleanup...)
  */
-static void ext4_write_super(struct super_block *sb)
+static void ext4_write_super_locked(struct super_block *sb)
 {
 	if (EXT4_SB(sb)->s_journal) {
 		if (mutex_trylock(&sb->s_lock) != 0)
@@ -3284,6 +3285,13 @@ static void ext4_write_super(struct supe
 	}
 }
 
+static void ext4_write_super(struct super_block *sb)
+{
+	lock_super(sb);
+	ext4_write_super_locked(sb);
+	unlock_super(sb);
+}
+
 static int ext4_sync_fs(struct super_block *sb, int wait)
 {
 	int ret = 0;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2009-05-06 21:37:16.737659384 +0200
+++ vfs-2.6/fs/fat/inode.c	2009-05-06 21:37:53.358661652 +0200
@@ -441,8 +441,10 @@ static void fat_clear_inode(struct inode
 
 static void fat_write_super(struct super_block *sb)
 {
+	lock_super(sb);
 	sb->s_dirt = 0;
 	fat_clusters_flush(sb);
+	unlock_super(sb);
 }
 
 static void fat_put_super(struct super_block *sb)
Index: vfs-2.6/fs/super.c
===================================================================
--- vfs-2.6.orig/fs/super.c	2009-05-06 21:41:00.805784126 +0200
+++ vfs-2.6/fs/super.c	2009-05-06 21:41:57.191661325 +0200
@@ -421,7 +421,6 @@ restart:
 			spin_unlock(&sb_lock);
 
 			down_read(&sb->s_umount);
-			lock_super(sb);
 			if (sb->s_root) {
 				/*
 				 * Avoid loops if a filesystem left s_dirt
@@ -433,10 +432,12 @@ restart:
 				if (sb->s_flags & MS_RDONLY)
 					sb->s_dirt = 0;
 
-				if (sb->s_dirt)
+				if (sb->s_dirt) {
+					get_fs_excl();
 					sb->s_op->write_super(sb);
+					put_fs_excl();
+				}
 			}
-			unlock_super(sb);
 			up_read(&sb->s_umount);
 
 			spin_lock(&sb_lock);
Index: vfs-2.6/fs/sysv/inode.c
===================================================================
--- vfs-2.6.orig/fs/sysv/inode.c	2009-05-06 21:39:21.263682879 +0200
+++ vfs-2.6/fs/sysv/inode.c	2009-05-06 21:39:58.219661498 +0200
@@ -37,6 +37,7 @@ static void sysv_write_super(struct supe
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 	unsigned long time = get_seconds(), old_time;
 
+	lock_super(sb);
 	lock_kernel();
 	/*
 	 * If we are going to write out the super block,
@@ -53,6 +54,7 @@ static void sysv_write_super(struct supe
 
 	sb->s_dirt = 0;
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
Index: vfs-2.6/fs/ufs/super.c
===================================================================
--- vfs-2.6.orig/fs/ufs/super.c	2009-05-06 21:40:01.440659714 +0200
+++ vfs-2.6/fs/ufs/super.c	2009-05-06 21:40:33.502786591 +0200
@@ -1131,6 +1131,7 @@ static void ufs_write_super(struct super
 	struct ufs_super_block_third * usb3;
 	unsigned flags;
 
+	lock_super(sb);
 	lock_kernel();
 	UFSD("ENTER\n");
 	flags = UFS_SB(sb)->s_flags;
@@ -1149,6 +1150,7 @@ static void ufs_write_super(struct super
 	sb->s_dirt = 0;
 	UFSD("EXIT\n");
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static void ufs_put_super(struct super_block *sb)

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Christoph Hellwig]

On Wed, May 06, 2009 at 10:16:31PM +0200, Christoph Hellwig wrote:
> Only four filesystems have both a ->write_super method and use lock_super
> internally: ext4, fat, sysv, ufs.  Push down lock_super into these
> filesystems and remove it from the caller.  Add a get_fs_excl/put_fs_excl
> pair to sync_super for now to keep the current behaviour in that
> respect.  Calling it a second time from write_super is fine as it's
> only checked for beeing non-zero.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Actually the patch lost two hunks due to missing quilt adds.  Here's the
complete one:


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/ext4/super.c
===================================================================
--- vfs-2.6.orig/fs/ext4/super.c	2009-05-06 22:23:00.016784345 +0200
+++ vfs-2.6/fs/ext4/super.c	2009-05-06 22:26:09.741784086 +0200
@@ -66,6 +66,7 @@ static int ext4_remount(struct super_blo
 static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf);
 static int ext4_unfreeze(struct super_block *sb);
 static void ext4_write_super(struct super_block *sb);
+static void ext4_write_super_locked(struct super_block *sb);
 static int ext4_freeze(struct super_block *sb);
 
 
@@ -566,7 +567,7 @@ static void ext4_put_super(struct super_
 	lock_super(sb);
 	lock_kernel();
 	if (sb->s_dirt)
-		ext4_write_super(sb);
+		ext4_write_super_locked(sb);
 
 	ext4_mb_release(sb);
 	ext4_ext_release(sb);
@@ -3273,17 +3274,22 @@ int ext4_force_commit(struct super_block
  * point.  (We can probably nuke this function altogether, and remove
  * any mention to sb->s_dirt in all of fs/ext4; eventual cleanup...)
  */
-static void ext4_write_super(struct super_block *sb)
+static void ext4_write_super_locked(struct super_block *sb)
 {
 	if (EXT4_SB(sb)->s_journal) {
-		if (mutex_trylock(&sb->s_lock) != 0)
-			BUG();
 		sb->s_dirt = 0;
 	} else {
 		ext4_commit_super(sb, EXT4_SB(sb)->s_es, 1);
 	}
 }
 
+static void ext4_write_super(struct super_block *sb)
+{
+	lock_super(sb);
+	ext4_write_super_locked(sb);
+	unlock_super(sb);
+}
+
 static int ext4_sync_fs(struct super_block *sb, int wait)
 {
 	int ret = 0;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2009-05-06 22:23:00.021784213 +0200
+++ vfs-2.6/fs/fat/inode.c	2009-05-06 22:26:09.741784086 +0200
@@ -441,8 +441,10 @@ static void fat_clear_inode(struct inode
 
 static void fat_write_super(struct super_block *sb)
 {
+	lock_super(sb);
 	sb->s_dirt = 0;
 	fat_clusters_flush(sb);
+	unlock_super(sb);
 }
 
 static void fat_put_super(struct super_block *sb)
Index: vfs-2.6/fs/super.c
===================================================================
--- vfs-2.6.orig/fs/super.c	2009-05-06 22:23:00.025787586 +0200
+++ vfs-2.6/fs/super.c	2009-05-06 22:26:09.742784073 +0200
@@ -421,7 +421,6 @@ restart:
 			spin_unlock(&sb_lock);
 
 			down_read(&sb->s_umount);
-			lock_super(sb);
 			if (sb->s_root) {
 				/*
 				 * Avoid loops if a filesystem left s_dirt
@@ -433,10 +432,12 @@ restart:
 				if (sb->s_flags & MS_RDONLY)
 					sb->s_dirt = 0;
 
-				if (sb->s_dirt)
+				if (sb->s_dirt) {
+					get_fs_excl();
 					sb->s_op->write_super(sb);
+					put_fs_excl();
+				}
 			}
-			unlock_super(sb);
 			up_read(&sb->s_umount);
 
 			spin_lock(&sb_lock);
Index: vfs-2.6/fs/sysv/inode.c
===================================================================
--- vfs-2.6.orig/fs/sysv/inode.c	2009-05-06 22:23:00.059784161 +0200
+++ vfs-2.6/fs/sysv/inode.c	2009-05-06 22:26:09.743783851 +0200
@@ -37,6 +37,7 @@ static void sysv_write_super(struct supe
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 	unsigned long time = get_seconds(), old_time;
 
+	lock_super(sb);
 	lock_kernel();
 	/*
 	 * If we are going to write out the super block,
@@ -53,6 +54,7 @@ static void sysv_write_super(struct supe
 
 	sb->s_dirt = 0;
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
Index: vfs-2.6/fs/ufs/super.c
===================================================================
--- vfs-2.6.orig/fs/ufs/super.c	2009-05-06 22:23:00.065784017 +0200
+++ vfs-2.6/fs/ufs/super.c	2009-05-06 22:26:09.743783851 +0200
@@ -1131,6 +1131,7 @@ static void ufs_write_super(struct super
 	struct ufs_super_block_third * usb3;
 	unsigned flags;
 
+	lock_super(sb);
 	lock_kernel();
 	UFSD("ENTER\n");
 	flags = UFS_SB(sb)->s_flags;
@@ -1149,6 +1150,7 @@ static void ufs_write_super(struct super
 	sb->s_dirt = 0;
 	UFSD("EXIT\n");
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static void ufs_put_super(struct super_block *sb)
Index: vfs-2.6/fs/sync.c
===================================================================
--- vfs-2.6.orig/fs/sync.c	2009-05-06 22:28:49.598694594 +0200
+++ vfs-2.6/fs/sync.c	2009-05-06 22:29:48.860661341 +0200
@@ -31,10 +31,11 @@ static int __sync_filesystem(struct supe
 	else
 		sync_quota_sb(sb, -1);
 	sync_inodes_sb(sb, wait);
-	lock_super(sb);
-	if (sb->s_dirt && sb->s_op->write_super)
+	if (sb->s_dirt && sb->s_op->write_super) {
+		get_fs_excl();
 		sb->s_op->write_super(sb);
-	unlock_super(sb);
+		put_fs_excl();
+	}
 	if (sb->s_op->sync_fs)
 		sb->s_op->sync_fs(sb, wait);
 	return __sync_blockdev(sb->s_bdev, wait);
@@ -162,10 +163,11 @@ int file_fsync(struct file *filp, struct
 
 	/* sync the superblock to buffers */
 	sb = inode->i_sb;
-	lock_super(sb);
-	if (sb->s_dirt && sb->s_op->write_super)
+	if (sb->s_dirt && sb->s_op->write_super) {
+		get_fs_excl();
 		sb->s_op->write_super(sb);
-	unlock_super(sb);
+		put_fs_excl();
+	}
 
 	/* .. finally sync the buffers to disk */
 	err = sync_blockdev(sb->s_bdev);

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Al Viro]

On Wed, May 06, 2009 at 10:36:18PM +0200, Christoph Hellwig wrote:
> On Wed, May 06, 2009 at 10:16:31PM +0200, Christoph Hellwig wrote:
> > Only four filesystems have both a ->write_super method and use lock_super
> > internally: ext4, fat, sysv, ufs.  Push down lock_super into these
> > filesystems and remove it from the caller.  Add a get_fs_excl/put_fs_excl
> > pair to sync_super for now to keep the current behaviour in that
> > respect.  Calling it a second time from write_super is fine as it's
> > only checked for beeing non-zero.
> > 
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Actually the patch lost two hunks due to missing quilt adds.  Here's the
> complete one:

That's wrong.  Even if fs does *not* use lock_super(), you can't get
rid of it in ->write_super().  The thing is, right now calls of
foo_write_super() are serialized by s_lock.  For all filesystems that
have ->write_super(), whether they use s_lock elsewhere or not.
Lose lock_super() in the caller and you'll need some replacement in
the method.

IOW, you are missing
	* affs
	* bfs
	* exofs
	* ext2
	* hfs
	* hfsplus
	* jffs2
	* nilfs2
	* reiserfs
	* xfs
in the list.  Some of those might be fine with several calls of ->write_super()
in parallel, but that needs per-fs review for all of those.

NAK in the current form.

[PATCH 2/2] ->write_super lock_super pushdown

[Christoph Hellwig]

Push down lock_super into ->write_super instances and remove it from the
caller.

Following filesystem don't need ->s_lock in ->write_super and are skipped:

 * bfs, nilfs2 - no other uses of s_lock and have internal locks in
	->write_super
 * ext2 - uses BKL in ext2_write_super and has internal calls without s_lock
 * reiserfs - no other uses of s_lock as has reiserfs_write_lock (BKL) in
 	->write_super
 * xfs - no other uses of s_lock and uses internal lock (buffer lock on
	superblock buffer) to serialize ->write_super.  Also xfs_fs_write_super 
	is superflous and will go away in the next merge window


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: vfs-2.6/fs/ext4/super.c
===================================================================
--- vfs-2.6.orig/fs/ext4/super.c	2009-05-11 23:26:24.844787452 +0200
+++ vfs-2.6/fs/ext4/super.c	2009-05-11 23:28:12.925808532 +0200
@@ -66,6 +66,7 @@ static int ext4_remount(struct super_blo
 static int ext4_statfs(struct dentry *dentry, struct kstatfs *buf);
 static int ext4_unfreeze(struct super_block *sb);
 static void ext4_write_super(struct super_block *sb);
+static void ext4_write_super_locked(struct super_block *sb);
 static int ext4_freeze(struct super_block *sb);
 
 
@@ -566,7 +567,7 @@ static void ext4_put_super(struct super_
 	lock_super(sb);
 	lock_kernel();
 	if (sb->s_dirt)
-		ext4_write_super(sb);
+		ext4_write_super_locked(sb);
 
 	ext4_mb_release(sb);
 	ext4_ext_release(sb);
@@ -3273,7 +3274,7 @@ int ext4_force_commit(struct super_block
  * point.  (We can probably nuke this function altogether, and remove
  * any mention to sb->s_dirt in all of fs/ext4; eventual cleanup...)
  */
-static void ext4_write_super(struct super_block *sb)
+static void ext4_write_super_locked(struct super_block *sb)
 {
 	if (EXT4_SB(sb)->s_journal) {
 		if (mutex_trylock(&sb->s_lock) != 0)
@@ -3284,6 +3285,13 @@ static void ext4_write_super(struct supe
 	}
 }
 
+static void ext4_write_super(struct super_block *sb)
+{
+	lock_super(sb);
+	ext4_write_super_locked(sb);
+	unlock_super(sb);
+}
+
 static int ext4_sync_fs(struct super_block *sb, int wait)
 {
 	int ret = 0;
Index: vfs-2.6/fs/fat/inode.c
===================================================================
--- vfs-2.6.orig/fs/fat/inode.c	2009-05-11 23:26:24.862786880 +0200
+++ vfs-2.6/fs/fat/inode.c	2009-05-11 23:28:12.925808532 +0200
@@ -441,10 +441,12 @@ static void fat_clear_inode(struct inode
 
 static void fat_write_super(struct super_block *sb)
 {
+	lock_super(sb);
 	sb->s_dirt = 0;
 
 	if (!(sb->s_flags & MS_RDONLY))
 		fat_clusters_flush(sb);
+	unlock_super(sb);
 }
 
 static void fat_put_super(struct super_block *sb)
Index: vfs-2.6/fs/super.c
===================================================================
--- vfs-2.6.orig/fs/super.c	2009-05-11 23:26:24.882820644 +0200
+++ vfs-2.6/fs/super.c	2009-05-11 23:28:12.926784214 +0200
@@ -420,10 +420,8 @@ restart:
 			spin_unlock(&sb_lock);
 
 			down_read(&sb->s_umount);
-			lock_super(sb);
 			if (sb->s_root && sb->s_dirt)
 				sb->s_op->write_super(sb);
-			unlock_super(sb);
 			up_read(&sb->s_umount);
 
 			spin_lock(&sb_lock);
Index: vfs-2.6/fs/sysv/inode.c
===================================================================
--- vfs-2.6.orig/fs/sysv/inode.c	2009-05-11 23:26:24.907820404 +0200
+++ vfs-2.6/fs/sysv/inode.c	2009-05-11 23:28:12.926784214 +0200
@@ -37,6 +37,7 @@ static void sysv_write_super(struct supe
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 	unsigned long time = get_seconds(), old_time;
 
+	lock_super(sb);
 	lock_kernel();
 	if (sb->s_flags & MS_RDONLY)
 		goto clean;
@@ -56,6 +57,7 @@ static void sysv_write_super(struct supe
 clean:
 	sb->s_dirt = 0;
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
Index: vfs-2.6/fs/ufs/super.c
===================================================================
--- vfs-2.6.orig/fs/ufs/super.c	2009-05-11 23:26:24.931820805 +0200
+++ vfs-2.6/fs/ufs/super.c	2009-05-11 23:28:12.927812907 +0200
@@ -1131,6 +1131,7 @@ static void ufs_write_super(struct super
 	struct ufs_super_block_third * usb3;
 	unsigned flags;
 
+	lock_super(sb);
 	lock_kernel();
 	UFSD("ENTER\n");
 	flags = UFS_SB(sb)->s_flags;
@@ -1150,6 +1151,7 @@ static void ufs_write_super(struct super
 	sb->s_dirt = 0;
 	UFSD("EXIT\n");
 	unlock_kernel();
+	unlock_super(sb);
 }
 
 static void ufs_put_super(struct super_block *sb)
Index: vfs-2.6/fs/affs/super.c
===================================================================
--- vfs-2.6.orig/fs/affs/super.c	2009-05-11 23:26:24.949786640 +0200
+++ vfs-2.6/fs/affs/super.c	2009-05-11 23:28:12.928818062 +0200
@@ -54,6 +54,7 @@ affs_write_super(struct super_block *sb)
 	int clean = 2;
 	struct affs_sb_info *sbi = AFFS_SB(sb);
 
+	lock_super(sb);
 	if (!(sb->s_flags & MS_RDONLY)) {
 		//	if (sbi->s_bitmap[i].bm_bh) {
 		//		if (buffer_dirty(sbi->s_bitmap[i].bm_bh)) {
@@ -66,6 +67,7 @@ affs_write_super(struct super_block *sb)
 		sb->s_dirt = !clean;	/* redo until bitmap synced */
 	} else
 		sb->s_dirt = 0;
+	unlock_super(sb);
 
 	pr_debug("AFFS: write_super() at %lu, clean=%d\n", get_seconds(), clean);
 }
Index: vfs-2.6/fs/exofs/super.c
===================================================================
--- vfs-2.6.orig/fs/exofs/super.c	2009-05-11 23:26:24.971786995 +0200
+++ vfs-2.6/fs/exofs/super.c	2009-05-11 23:28:12.929808342 +0200
@@ -214,6 +214,7 @@ static void exofs_write_super(struct sup
 		return;
 	}
 
+	lock_super(sb);
 	lock_kernel();
 	sbi = sb->s_fs_info;
 	fscb->s_nextid = cpu_to_le64(sbi->s_nextid);
@@ -246,6 +247,7 @@ out:
 	if (or)
 		osd_end_request(or);
 	unlock_kernel();
+	unlock_super(sb);
 	kfree(fscb);
 }
 
Index: vfs-2.6/fs/hfs/super.c
===================================================================
--- vfs-2.6.orig/fs/hfs/super.c	2009-05-11 23:26:24.990787179 +0200
+++ vfs-2.6/fs/hfs/super.c	2009-05-11 23:28:12.930808120 +0200
@@ -49,11 +49,13 @@ MODULE_LICENSE("GPL");
  */
 static void hfs_write_super(struct super_block *sb)
 {
+	lock_super(sb);
 	sb->s_dirt = 0;
-	if (sb->s_flags & MS_RDONLY)
-		return;
+
 	/* sync everything to the buffers */
-	hfs_mdb_commit(sb);
+	if (!(sb->s_flags & MS_RDONLY))
+		hfs_mdb_commit(sb);
+	unlock_super(sb);
 }
 
 /*
Index: vfs-2.6/fs/hfsplus/super.c
===================================================================
--- vfs-2.6.orig/fs/hfsplus/super.c	2009-05-11 23:26:25.008788283 +0200
+++ vfs-2.6/fs/hfsplus/super.c	2009-05-11 23:28:12.930808120 +0200
@@ -157,10 +157,12 @@ static void hfsplus_write_super(struct s
 	struct hfsplus_vh *vhdr = HFSPLUS_SB(sb).s_vhdr;
 
 	dprint(DBG_SUPER, "hfsplus_write_super\n");
+
+	lock_super(sb);
 	sb->s_dirt = 0;
 	if (sb->s_flags & MS_RDONLY)
 		/* warn? */
-		return;
+		goto out;
 
 	vhdr->free_blocks = cpu_to_be32(HFSPLUS_SB(sb).free_blocks);
 	vhdr->next_alloc = cpu_to_be32(HFSPLUS_SB(sb).next_alloc);
@@ -192,6 +194,8 @@ static void hfsplus_write_super(struct s
 		}
 		HFSPLUS_SB(sb).flags &= ~HFSPLUS_SB_WRITEBACKUP;
 	}
+ out:
+	unlock_super(sb);
 }
 
 static void hfsplus_put_super(struct super_block *sb)
Index: vfs-2.6/fs/jffs2/super.c
===================================================================
--- vfs-2.6.orig/fs/jffs2/super.c	2009-05-11 23:26:25.035786621 +0200
+++ vfs-2.6/fs/jffs2/super.c	2009-05-11 23:28:12.931808177 +0200
@@ -56,15 +56,18 @@ static void jffs2_i_init_once(void *foo)
 static void jffs2_write_super(struct super_block *sb)
 {
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(sb);
+
+	lock_super(sb);
 	sb->s_dirt = 0;
 
-	if (sb->s_flags & MS_RDONLY)
-		return;
+	if (!(sb->s_flags & MS_RDONLY)) {
+		D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
+		jffs2_garbage_collect_trigger(c);
+		jffs2_erase_pending_blocks(c, 0);
+		jffs2_flush_wbuf_gc(c, 0);
+	}
 
-	D1(printk(KERN_DEBUG "jffs2_write_super()\n"));
-	jffs2_garbage_collect_trigger(c);
-	jffs2_erase_pending_blocks(c, 0);
-	jffs2_flush_wbuf_gc(c, 0);
+	unlock_super(sb);
 }
 
 static int jffs2_sync_fs(struct super_block *sb, int wait)
Index: vfs-2.6/fs/sync.c
===================================================================
--- vfs-2.6.orig/fs/sync.c	2009-05-11 23:28:41.043659008 +0200
+++ vfs-2.6/fs/sync.c	2009-05-11 23:29:04.202661387 +0200
@@ -33,10 +33,8 @@ static int __sync_filesystem(struct supe
 	else
 		sync_quota_sb(sb, -1);
 	sync_inodes_sb(sb, wait);
-	lock_super(sb);
 	if (sb->s_dirt && sb->s_op->write_super)
 		sb->s_op->write_super(sb);
-	unlock_super(sb);
 	if (sb->s_op->sync_fs)
 		sb->s_op->sync_fs(sb, wait);
 	return __sync_blockdev(sb->s_bdev, wait);
@@ -164,10 +162,8 @@ int file_fsync(struct file *filp, struct
 
 	/* sync the superblock to buffers */
 	sb = inode->i_sb;
-	lock_super(sb);
 	if (sb->s_dirt && sb->s_op->write_super)
 		sb->s_op->write_super(sb);
-	unlock_super(sb);
 
 	/* .. finally sync the buffers to disk */
 	err = sync_blockdev(sb->s_bdev);

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Al Viro]

On Mon, May 11, 2009 at 11:35:03PM +0200, Christoph Hellwig wrote:
> 
> Push down lock_super into ->write_super instances and remove it from the
> caller.
> 
> Following filesystem don't need ->s_lock in ->write_super and are skipped:
> 
>  * bfs, nilfs2 - no other uses of s_lock and have internal locks in
> 	->write_super
>  * ext2 - uses BKL in ext2_write_super and has internal calls without s_lock
>  * reiserfs - no other uses of s_lock as has reiserfs_write_lock (BKL) in
>  	->write_super

Ehh...  I'm not at all sure that reiserfs one is OK, but we are not exposing
any races that didn't exist before ;-/

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Boaz Harrosh]

On 05/12/2009 12:35 AM, Christoph Hellwig wrote:
> Push down lock_super into ->write_super instances and remove it from the
> caller.
> 
> Following filesystem don't need ->s_lock in ->write_super and are skipped:
> 
>  * bfs, nilfs2 - no other uses of s_lock and have internal locks in
> 	->write_super
>  * ext2 - uses BKL in ext2_write_super and has internal calls without s_lock
>  * reiserfs - no other uses of s_lock as has reiserfs_write_lock (BKL) in
>  	->write_super
>  * xfs - no other uses of s_lock and uses internal lock (buffer lock on
> 	superblock buffer) to serialize ->write_super.  Also xfs_fs_write_super 
> 	is superflous and will go away in the next merge window
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

Ack-by Boaz Harrosh <bharrosh@panasas.com>

<snip>
> ===================================================================
> --- vfs-2.6.orig/fs/exofs/super.c	2009-05-11 23:26:24.971786995 +0200
> +++ vfs-2.6/fs/exofs/super.c	2009-05-11 23:28:12.929808342 +0200
> @@ -214,6 +214,7 @@ static void exofs_write_super(struct sup
>  		return;
>  	}
>  
> +	lock_super(sb);
>  	lock_kernel();
>  	sbi = sb->s_fs_info;
>  	fscb->s_nextid = cpu_to_le64(sbi->s_nextid);
> @@ -246,6 +247,7 @@ out:
>  	if (or)
>  		osd_end_request(or);
>  	unlock_kernel();
> +	unlock_super(sb);
>  	kfree(fscb);
>  }
>  

Please I have a question about this?

lock_super():
  I do not see any other lock_super() in exofs, so all this might "lock" is
  race against itself, right? Should I make sure that concurrent
  exofs_write_super are protected some other way and remove this?

lock_kernel();
  What is that used for? What should I check so this can be removed?

Sorry for the novice-ness ;-)

Thanks
Boaz

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Christoph Hellwig]

On Tue, May 12, 2009 at 01:10:10PM +0300, Boaz Harrosh wrote:
> Please I have a question about this?
> 
> lock_super():
>   I do not see any other lock_super() in exofs, so all this might "lock" is
>   race against itself, right?

Yes.

>   Should I make sure that concurrent
>   exofs_write_super are protected some other way and remove this?

Yes.

> 
> lock_kernel();
>   What is that used for? What should I check so this can be removed?

lock kernel is the big kernel lock as in the very first lock that was
added when Linux grew SMP support.  If you filesystem does internal
locking you most likely don't need this one at all.  The only superblock
method still called with the BKL in the vfs tree is remount, but it
will most likely be gone before 2.6.31, too.  After that you can do
a quick audit if there was anything it protected against and remove it.

Re: [PATCH 2/2] ->write_super lock_super pushdown

[Al Viro]

On Tue, May 12, 2009 at 03:25:04PM +0200, Christoph Hellwig wrote:

> lock kernel is the big kernel lock as in the very first lock that was
> added when Linux grew SMP support.  If you filesystem does internal
> locking you most likely don't need this one at all.  The only superblock
> method still called with the BKL in the vfs tree is remount, but it
> will most likely be gone before 2.6.31, too.  After that you can do
> a quick audit if there was anything it protected against and remove it.

Don't forget that ->get_sb() on mount(2) is also under BKL.  Not a
superblock method, for obvious reasons.