[PATCH 0/5] ext2: Preparation to remove BKL

[PATCH 0/5] ext2: Preparation to remove BKL

[Jan Blunck]

In this series I've collected the patches that prepare ext2 for BKL removal.
It consist mostly of cleanups and additionally introduces a spinlock to protect
some of the superblock's fields against concurrent access. I've addressed the
feedback kindly provided by Ogawa-san by moving the ext2_write_super() out of
ext2_setup_super().

These patches have been part of the BKL removal series that I have posted in
November 2009 already. Since this is more than just removing the usage of the
big lock I repost it separately for inclusion. This series, at least the last
patch that includes the s_lock, needs to be merged before Frederics bkl-removal
branch, if he merges the rest of my patches there.

Thanks,
Jan

Jan Blunck (5):
  ext2: Use ext2_clear_super_error() in ext2_sync_fs()
  ext2: Set the write time in ext2_sync_fs()
  ext2: Remove duplicate code from ext2_sync_fs()
  ext2: Move ext2_write_super() out of ext2_setup_super()
  ext2: Add ext2_sb_info s_lock spinlock

 fs/ext2/inode.c            |    2 +
 fs/ext2/super.c            |   63 +++++++++++++++++++++++++-------------------
 include/linux/ext2_fs_sb.h |    6 ++++
 3 files changed, 44 insertions(+), 27 deletions(-)

[PATCH 1/5] ext2: Use ext2_clear_super_error() in ext2_sync_fs()

[Jan Blunck]

ext2_sync_fs() used to duplicate the code from ext2_clear_super_error().

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/ext2/super.c |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 42e4a30..dbebbf0 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1120,8 +1120,8 @@ static void ext2_clear_super_error(struct super_block *sb)
 		 * be remapped.  Nothing we can do but to retry the
 		 * write and hope for the best.
 		 */
-		printk(KERN_ERR "EXT2-fs: %s previous I/O error to "
-		       "superblock detected", sb->s_id);
+		ext2_msg(sb, KERN_ERR,
+		       "previous I/O error to superblock detected\n");
 		clear_buffer_write_io_error(sbh);
 		set_buffer_uptodate(sbh);
 	}
@@ -1164,20 +1164,7 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
 
 	lock_kernel();
-	if (buffer_write_io_error(sbh)) {
-		/*
-		 * Oh, dear.  A previous attempt to write the
-		 * superblock failed.  This could happen because the
-		 * USB device was yanked out.  Or it could happen to
-		 * be a transient write error and maybe the block will
-		 * be remapped.  Nothing we can do but to retry the
-		 * write and hope for the best.
-		 */
-		ext2_msg(sb, KERN_ERR,
-		       "previous I/O error to superblock detected\n");
-		clear_buffer_write_io_error(sbh);
-		set_buffer_uptodate(sbh);
-	}
+	ext2_clear_super_error(sb);
 
 	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
 		ext2_debug("setting valid to 0\n");
-- 
1.6.4.2

[PATCH 2/5] ext2: Set the write time in ext2_sync_fs()

[Jan Blunck]

This is probably a typo since the write time should actually be updated by
ext2_sync_fs() instead of the mount time.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/ext2/super.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index dbebbf0..4a17ca3 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1173,7 +1173,7 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 			cpu_to_le32(ext2_count_free_blocks(sb));
 		es->s_free_inodes_count =
 			cpu_to_le32(ext2_count_free_inodes(sb));
-		es->s_mtime = cpu_to_le32(get_seconds());
+		es->s_wtime = cpu_to_le32(get_seconds());
 		ext2_sync_super(sb, es);
 	} else {
 		ext2_commit_super(sb, es);
-- 
1.6.4.2

[PATCH 3/5] ext2: Remove duplicate code from ext2_sync_fs()

[Jan Blunck]

Depending in the state (valid or unchecked) of the filesystem either
ext2_sync_super() or ext2_commit_super() is called. If the filesystem is
currently valid (it is checked), we first mark it unchecked and afterwards
duplicate the work that ext2_sync_super() is doing later. Therefore this
patch removes the duplicate code and calls ext2_sync_super() directly after
marking the filesystem unchecked.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/ext2/super.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 4a17ca3..40fc8d5 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1164,16 +1164,9 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
 
 	lock_kernel();
-	ext2_clear_super_error(sb);

[PATCH 4/5] ext2: Move ext2_write_super() out of ext2_setup_super()

[Jan Blunck]

Move ext2_write_super() out of ext2_setup_super() as a preparation for the
next patch that adds a new lock for superblock fields.

Signed-off-by: Jan Blunck <jblunck@suse.de>
---
 fs/ext2/super.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 40fc8d5..b01c491 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -606,7 +606,6 @@ static int ext2_setup_super (struct super_block * sb,
 	if (!le16_to_cpu(es->s_max_mnt_count))
 		es->s_max_mnt_count = cpu_to_le16(EXT2_DFL_MAX_MNT_COUNT);
 	le16_add_cpu(&es->s_mnt_count, 1);
-	ext2_write_super(sb);
 	if (test_opt (sb, DEBUG))
 		ext2_msg(sb, KERN_INFO, "%s, %s, bs=%lu, fs=%lu, gc=%lu, "
 			"bpg=%lu, ipg=%lu, mo=%04lx]",
@@ -1079,7 +1078,9 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 	if (EXT2_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL))
 		ext2_msg(sb, KERN_WARNING,
 			"warning: mounting ext3 filesystem as ext2");
-	ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
+	if (ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY))
+		sb->s_flags != MS_RDONLY;
+	ext2_write_super(sb);
 	return 0;
 
 cantfind_ext2:
@@ -1249,6 +1250,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		 */
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 		es->s_mtime = cpu_to_le32(get_seconds());
+		ext2_sync_super(sb, es);
 	} else {
 		__le32 ret = EXT2_HAS_RO_COMPAT_FEATURE(sb,
 					       ~EXT2_FEATURE_RO_COMPAT_SUPP);
@@ -1268,8 +1270,8 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		sbi->s_mount_state = le16_to_cpu(es->s_state);
 		if (!ext2_setup_super (sb, es, 0))
 			sb->s_flags &= ~MS_RDONLY;
+		ext2_write_super(sb);
 	}
-	ext2_sync_super(sb, es);
 	unlock_kernel();
 	return 0;
 restore_opts:
-- 
1.6.4.2

Re: [PATCH 4/5] ext2: Move ext2_write_super() out of ext2_setup_super()

[Jan Kara]

On Mon 12-04-10 22:41:44, Jan Blunck wrote:
> Move ext2_write_super() out of ext2_setup_super() as a preparation for the
> next patch that adds a new lock for superblock fields.
> 
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> ---
>  fs/ext2/super.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 40fc8d5..b01c491 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -606,7 +606,6 @@ static int ext2_setup_super (struct super_block * sb,
>  	if (!le16_to_cpu(es->s_max_mnt_count))
>  		es->s_max_mnt_count = cpu_to_le16(EXT2_DFL_MAX_MNT_COUNT);
>  	le16_add_cpu(&es->s_mnt_count, 1);
> -	ext2_write_super(sb);
>  	if (test_opt (sb, DEBUG))
>  		ext2_msg(sb, KERN_INFO, "%s, %s, bs=%lu, fs=%lu, gc=%lu, "
>  			"bpg=%lu, ipg=%lu, mo=%04lx]",
> @@ -1079,7 +1078,9 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
>  	if (EXT2_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL))
>  		ext2_msg(sb, KERN_WARNING,
>  			"warning: mounting ext3 filesystem as ext2");
> -	ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
> +	if (ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY))
> +		sb->s_flags != MS_RDONLY;
                            ^^ Should be |=, shouldn't it?

										Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 4/5] ext2: Move ext2_write_super() out of ext2_setup_super()

[Jan Blunck]

On Tue, Apr 13, Jan Kara wrote:

> On Mon 12-04-10 22:41:44, Jan Blunck wrote:
> > Move ext2_write_super() out of ext2_setup_super() as a preparation for the
> > next patch that adds a new lock for superblock fields.
> > 
> > Signed-off-by: Jan Blunck <jblunck@suse.de>
> > ---
> >  fs/ext2/super.c |    8 +++++---
> >  1 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > index 40fc8d5..b01c491 100644
> > --- a/fs/ext2/super.c
> > +++ b/fs/ext2/super.c
> > @@ -606,7 +606,6 @@ static int ext2_setup_super (struct super_block * sb,
> >  	if (!le16_to_cpu(es->s_max_mnt_count))
> >  		es->s_max_mnt_count = cpu_to_le16(EXT2_DFL_MAX_MNT_COUNT);
> >  	le16_add_cpu(&es->s_mnt_count, 1);
> > -	ext2_write_super(sb);
> >  	if (test_opt (sb, DEBUG))
> >  		ext2_msg(sb, KERN_INFO, "%s, %s, bs=%lu, fs=%lu, gc=%lu, "
> >  			"bpg=%lu, ipg=%lu, mo=%04lx]",
> > @@ -1079,7 +1078,9 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> >  	if (EXT2_HAS_COMPAT_FEATURE(sb, EXT3_FEATURE_COMPAT_HAS_JOURNAL))
> >  		ext2_msg(sb, KERN_WARNING,
> >  			"warning: mounting ext3 filesystem as ext2");
> > -	ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY);
> > +	if (ext2_setup_super (sb, es, sb->s_flags & MS_RDONLY))
> > +		sb->s_flags != MS_RDONLY;
>                             ^^ Should be |=, shouldn't it?
> 

Jan "Eagle Eye" Kara!!! Maybe its time for me to invest in new glasses ...

Thanks for spotting it,
Jan

[PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

[Jan Blunck]

Add a spinlock that protects against concurrent modifications of
s_mount_state, s_blocks_last, s_overhead_last and the content of the
superblock's buffer pointed to by sbi->s_es. This is a preparation patch
for removing the BKL from ext2 in the next patch.

Signed-off-by: Jan Blunck <jblunck@suse.de>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Jan Kara <jack@suse.cz>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/ext2/inode.c            |    2 ++
 fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
 include/linux/ext2_fs_sb.h |    6 ++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index fc13cc1..5d15442 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
 				* created, add a flag to the superblock.
 				*/
 				lock_kernel();
+				spin_lock(&EXT2_SB(sb)->s_lock);
 				ext2_update_dynamic_rev(sb);
 				EXT2_SET_RO_COMPAT_FEATURE(sb,
 					EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
+				spin_unlock(&EXT2_SB(sb)->s_lock);
 				unlock_kernel();
 				ext2_write_super(sb);
 			}
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index b01c491..34d7a62 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -52,8 +52,10 @@ void ext2_error (struct super_block * sb, const char * function,
 	struct ext2_super_block *es = sbi->s_es;
 
 	if (!(sb->s_flags & MS_RDONLY)) {
+		spin_lock(&sbi->s_lock);
 		sbi->s_mount_state |= EXT2_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT2_ERROR_FS);
+		spin_unlock(&sbi->s_lock);
 		ext2_sync_super(sb, es);
 	}
 
@@ -84,6 +86,9 @@ void ext2_msg(struct super_block *sb, const char *prefix,
 	va_end(args);
 }
 
+/*
+ * This must be called with sbi->s_lock held.
+ */
 void ext2_update_dynamic_rev(struct super_block *sb)
 {
 	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
@@ -124,7 +129,9 @@ static void ext2_put_super (struct super_block * sb)
 	if (!(sb->s_flags & MS_RDONLY)) {
 		struct ext2_super_block *es = sbi->s_es;
 
+		spin_lock(&sbi->s_lock);
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
+		spin_unlock(&sbi->s_lock);
 		ext2_sync_super(sb, es);
 	}
 	db_count = sbi->s_gdb_count;
@@ -209,6 +216,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
 	struct ext2_super_block *es = sbi->s_es;
 	unsigned long def_mount_opts;
 
+	spin_lock(&sbi->s_lock);
 	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
 
 	if (sbi->s_sb_block != 1)
@@ -281,6 +289,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
 	if (!test_opt(sb, RESERVATION))
 		seq_puts(seq, ",noreservation");
 
+	spin_unlock(&sbi->s_lock);
 	return 0;
 }
 
@@ -766,6 +775,8 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_fs_info = sbi;
 	sbi->s_sb_block = sb_block;
 
+	spin_lock_init(&sbi->s_lock);
+
 	/*
 	 * See what the current blocksize for the device is, and
 	 * use that as the blocksize.  Otherwise (or if the blocksize
@@ -1137,12 +1148,16 @@ static void ext2_commit_super (struct super_block * sb,
 	sb->s_dirt = 0;
 }
 
-static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
+static void ext2_sync_super(struct super_block *sb,
+			    struct ext2_super_block *es)
 {
 	ext2_clear_super_error(sb);
+	spin_lock(&EXT2_SB(sb)->s_lock);
 	es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb));
 	es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb));
 	es->s_wtime = cpu_to_le32(get_seconds());
+	/* unlock before we do IO */
+	spin_unlock(&EXT2_SB(sb)->s_lock);
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
 	sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
 	sb->s_dirt = 0;
@@ -1158,19 +1173,22 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
  * may have been checked while mounted and e2fsck may have
  * set s_state to EXT2_VALID_FS after some corrections.
  */
-
 static int ext2_sync_fs(struct super_block *sb, int wait)
 {
+	struct ext2_sb_info *sbi = EXT2_SB(sb);
 	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
 	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
 
 	lock_kernel();
+	spin_lock(&sbi->s_lock);
 	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
 		ext2_debug("setting valid to 0\n");
 		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
+		spin_unlock(&sbi->s_lock);
 		ext2_sync_super(sb, es);
 	} else {
 		ext2_commit_super(sb, es);
+		spin_unlock(&sbi->s_lock);
 	}
 	sb->s_dirt = 0;
 	unlock_kernel();
@@ -1197,6 +1215,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 	int err;
 
 	lock_kernel();
+	spin_lock(&sbi->s_lock);
 
 	/* Store the old options */
 	old_sb_flags = sb->s_flags;
@@ -1235,12 +1254,14 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		sbi->s_mount_opt |= old_mount_opt & EXT2_MOUNT_XIP;
 	}
 	if ((*flags & MS_RDONLY) == (sb->s_flags & MS_RDONLY)) {
+		spin_unlock(&sbi->s_lock);
 		unlock_kernel();
 		return 0;
 	}
 	if (*flags & MS_RDONLY) {
 		if (le16_to_cpu(es->s_state) & EXT2_VALID_FS ||
 		    !(sbi->s_mount_state & EXT2_VALID_FS)) {
+			spin_unlock(&sbi->s_lock);
 			unlock_kernel();
 			return 0;
 		}
@@ -1250,6 +1271,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		 */
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
 		es->s_mtime = cpu_to_le32(get_seconds());
+		spin_unlock(&sbi->s_lock);
 		ext2_sync_super(sb, es);
 	} else {
 		__le32 ret = EXT2_HAS_RO_COMPAT_FEATURE(sb,
@@ -1270,6 +1292,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		sbi->s_mount_state = le16_to_cpu(es->s_state);
 		if (!ext2_setup_super (sb, es, 0))
 			sb->s_flags &= ~MS_RDONLY;
+		spin_unlock(&sbi->s_lock);
 		ext2_write_super(sb);
 	}
 	unlock_kernel();
@@ -1279,6 +1302,7 @@ restore_opts:
 	sbi->s_resuid = old_opts.s_resuid;
 	sbi->s_resgid = old_opts.s_resgid;
 	sb->s_flags = old_sb_flags;
+	spin_unlock(&sbi->s_lock);
 	unlock_kernel();
 	return err;
 }
@@ -1290,6 +1314,8 @@ static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)
 	struct ext2_super_block *es = sbi->s_es;
 	u64 fsid;
 
+	spin_lock(&sbi->s_lock);
+
 	if (test_opt (sb, MINIX_DF))
 		sbi->s_overhead_last = 0;
 	else if (sbi->s_blocks_last != le32_to_cpu(es->s_blocks_count)) {
@@ -1344,6 +1370,7 @@ static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)
 	       le64_to_cpup((void *)es->s_uuid + sizeof(u64));
 	buf->f_fsid.val[0] = fsid & 0xFFFFFFFFUL;
 	buf->f_fsid.val[1] = (fsid >> 32) & 0xFFFFFFFFUL;
+	spin_unlock(&sbi->s_lock);
 	return 0;
 }
 
diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
index 1cdb663..dbc5934 100644
--- a/include/linux/ext2_fs_sb.h
+++ b/include/linux/ext2_fs_sb.h
@@ -106,6 +106,12 @@ struct ext2_sb_info {
 	spinlock_t s_rsv_window_lock;
 	struct rb_root s_rsv_window_root;
 	struct ext2_reserve_window_node s_rsv_window_head;
+	/*
+	 * protects against concurrent modifications of s_mount_state,
+	 * s_blocks_last, s_overhead_last and the content of superblock's
+	 * buffer pointed to by sbi->s_es.
+	 */
+	spinlock_t s_lock;
 };
 
 static inline spinlock_t *
-- 
1.6.4.2

Re: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

[Jan Kara]

On Mon 12-04-10 22:41:45, Jan Blunck wrote:
> Add a spinlock that protects against concurrent modifications of
> s_mount_state, s_blocks_last, s_overhead_last and the content of the
> superblock's buffer pointed to by sbi->s_es. This is a preparation patch
> for removing the BKL from ext2 in the next patch.
> 
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
>  fs/ext2/inode.c            |    2 ++
>  fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
>  include/linux/ext2_fs_sb.h |    6 ++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
> 
>diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
>index fc13cc1..5d15442 100644
>--- a/fs/ext2/inode.c
>+++ b/fs/ext2/inode.c
>@@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
>                               * created, add a flag to the superblock.
>                               */
>                               lock_kernel();
>+                              spin_lock(&EXT2_SB(sb)->s_lock);
>                               ext2_update_dynamic_rev(sb);
>                               EXT2_SET_RO_COMPAT_FEATURE(sb,
>                                       EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
>+                              spin_unlock(&EXT2_SB(sb)->s_lock);
>                               unlock_kernel();
>                               ext2_write_super(sb);
>                       }
  Looking at this - probably we should protect by this lock also setting of
a feature in ext2_xattr_update_super_block(). It's an unrelated bugfix but
when we are already doing the bugfixing & cleanups in this area...

> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index b01c491..34d7a62 100644
> --- a/fs/ext2/super.c
> +++ b/fs/ext2/super.c
> @@ -209,6 +216,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  	struct ext2_super_block *es = sbi->s_es;
>  	unsigned long def_mount_opts;
>  
> +	spin_lock(&sbi->s_lock);
>  	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
>  
>  	if (sbi->s_sb_block != 1)
> @@ -281,6 +289,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  	if (!test_opt(sb, RESERVATION))
>  		seq_puts(seq, ",noreservation");
>  
> +	spin_unlock(&sbi->s_lock);
>  	return 0;
>  }
  Why exactly do you have in the above? Probably because of consistent
view of mount options? You should comment about that in the changelo and
especially at the lock declaration in ext2_fs.h.

> @@ -1158,19 +1173,22 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
>   * may have been checked while mounted and e2fsck may have
>   * set s_state to EXT2_VALID_FS after some corrections.
>   */
> -
>  static int ext2_sync_fs(struct super_block *sb, int wait)
>  {
> +	struct ext2_sb_info *sbi = EXT2_SB(sb);
>  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
>  	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
>  
>  	lock_kernel();
> +	spin_lock(&sbi->s_lock);
>  	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
>  		ext2_debug("setting valid to 0\n");
>  		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
> +		spin_unlock(&sbi->s_lock);
>  		ext2_sync_super(sb, es);
>  	} else {
>  		ext2_commit_super(sb, es);
> +		spin_unlock(&sbi->s_lock);
  Could you please fold in ext2_commit_super? It's used only here and it's
name looks a bit scary to be called under the spinlock...

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

[Jan Blunck]

On Tue, Apr 13, Jan Kara wrote:

> On Mon 12-04-10 22:41:45, Jan Blunck wrote:
> > Add a spinlock that protects against concurrent modifications of
> > s_mount_state, s_blocks_last, s_overhead_last and the content of the
> > superblock's buffer pointed to by sbi->s_es. This is a preparation patch
> > for removing the BKL from ext2 in the next patch.
> > 
> > Signed-off-by: Jan Blunck <jblunck@suse.de>
> > Cc: Andi Kleen <andi@firstfloor.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> > ---
> >  fs/ext2/inode.c            |    2 ++
> >  fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
> >  include/linux/ext2_fs_sb.h |    6 ++++++
> >  3 files changed, 37 insertions(+), 2 deletions(-)
> > 
> >diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> >index fc13cc1..5d15442 100644
> >--- a/fs/ext2/inode.c
> >+++ b/fs/ext2/inode.c
> >@@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
> >                               * created, add a flag to the superblock.
> >                               */
> >                               lock_kernel();
> >+                              spin_lock(&EXT2_SB(sb)->s_lock);
> >                               ext2_update_dynamic_rev(sb);
> >                               EXT2_SET_RO_COMPAT_FEATURE(sb,
> >                                       EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> >+                              spin_unlock(&EXT2_SB(sb)->s_lock);
> >                               unlock_kernel();
> >                               ext2_write_super(sb);
> >                       }
>   Looking at this - probably we should protect by this lock also setting of
> a feature in ext2_xattr_update_super_block(). It's an unrelated bugfix but
> when we are already doing the bugfixing & cleanups in this area...
> 
> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> > index b01c491..34d7a62 100644
> > --- a/fs/ext2/super.c
> > +++ b/fs/ext2/super.c
> > @@ -209,6 +216,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
> >  	struct ext2_super_block *es = sbi->s_es;
> >  	unsigned long def_mount_opts;
> >  
> > +	spin_lock(&sbi->s_lock);
> >  	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
> >  
> >  	if (sbi->s_sb_block != 1)
> > @@ -281,6 +289,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
> >  	if (!test_opt(sb, RESERVATION))
> >  		seq_puts(seq, ",noreservation");
> >  
> > +	spin_unlock(&sbi->s_lock);
> >  	return 0;
> >  }
>   Why exactly do you have in the above? Probably because of consistent
> view of mount options? You should comment about that in the changelo and
> especially at the lock declaration in ext2_fs.h.
> 
> > @@ -1158,19 +1173,22 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
> >   * may have been checked while mounted and e2fsck may have
> >   * set s_state to EXT2_VALID_FS after some corrections.
> >   */
> > -
> >  static int ext2_sync_fs(struct super_block *sb, int wait)
> >  {
> > +	struct ext2_sb_info *sbi = EXT2_SB(sb);
> >  	struct ext2_super_block *es = EXT2_SB(sb)->s_es;
> >  	struct buffer_head *sbh = EXT2_SB(sb)->s_sbh;
> >  
> >  	lock_kernel();
> > +	spin_lock(&sbi->s_lock);
> >  	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
> >  		ext2_debug("setting valid to 0\n");
> >  		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
> > +		spin_unlock(&sbi->s_lock);
> >  		ext2_sync_super(sb, es);
> >  	} else {
> >  		ext2_commit_super(sb, es);
> > +		spin_unlock(&sbi->s_lock);
>   Could you please fold in ext2_commit_super? It's used only here and it's
> name looks a bit scary to be called under the spinlock...

Right. Is there actually a reason why we don't update the s_free_blocks_count
and s_free_inodes_count when we found the filesystem did not have the
EXT2_VALID_FS set? Otherwise I could use ext2_sync_super() in both and make it
honor the wait flag by just calling sync_dirty_buffer() when it is necessary.

What do you think?

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 40fc8d5..8187061 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -39,7 +39,7 @@
 #include "xip.h"
 
 static void ext2_sync_super(struct super_block *sb,
-			    struct ext2_super_block *es);
+			    struct ext2_super_block *es, int wait);
 static int ext2_remount (struct super_block * sb, int * flags, char * data);
 static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf);
 static int ext2_sync_fs(struct super_block *sb, int wait);
@@ -54,7 +54,7 @@ void ext2_error (struct super_block * sb, const char * function,
 	if (!(sb->s_flags & MS_RDONLY)) {
 		sbi->s_mount_state |= EXT2_ERROR_FS;
 		es->s_state |= cpu_to_le16(EXT2_ERROR_FS);
-		ext2_sync_super(sb, es);
+		ext2_sync_super(sb, es, 1);
 	}
 
 	va_start(args, fmt);
@@ -125,7 +125,7 @@ static void ext2_put_super (struct super_block * sb)
 		struct ext2_super_block *es = sbi->s_es;
 
 		es->s_state = cpu_to_le16(sbi->s_mount_state);
-		ext2_sync_super(sb, es);
+		ext2_sync_super(sb, es, 1);
 	}
 	db_count = sbi->s_gdb_count;
 	for (i = 0; i < db_count; i++)
@@ -1127,23 +1127,16 @@ static void ext2_clear_super_error(struct super_block *sb)
 	}
 }
 
-static void ext2_commit_super (struct super_block * sb,
-			       struct ext2_super_block * es)
-{
-	ext2_clear_super_error(sb);
-	es->s_wtime = cpu_to_le32(get_seconds());
-	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
-	sb->s_dirt = 0;
-}
-
-static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
+static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es,
+			    int wait)
 {
 	ext2_clear_super_error(sb);
 	es->s_free_blocks_count = cpu_to_le32(ext2_count_free_blocks(sb));
 	es->s_free_inodes_count = cpu_to_le32(ext2_count_free_inodes(sb));
 	es->s_wtime = cpu_to_le32(get_seconds());
 	mark_buffer_dirty(EXT2_SB(sb)->s_sbh);
-	sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
+	if (wait)
+		sync_dirty_buffer(EXT2_SB(sb)->s_sbh);
 	sb->s_dirt = 0;
 }
 
@@ -1167,11 +1160,8 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
 	if (es->s_state & cpu_to_le16(EXT2_VALID_FS)) {
 		ext2_debug("setting valid to 0\n");
 		es->s_state &= cpu_to_le16(~EXT2_VALID_FS);
-		ext2_sync_super(sb, es);
-	} else {
-		ext2_commit_super(sb, es);
 	}
-	sb->s_dirt = 0;
+	ext2_sync_super(sb, es, wait);
 	unlock_kernel();
 
 	return 0;
@@ -1269,7 +1259,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
 		if (!ext2_setup_super (sb, es, 0))
 			sb->s_flags &= ~MS_RDONLY;
 	}
-	ext2_sync_super(sb, es);
+	ext2_sync_super(sb, es, 1);
 	unlock_kernel();
 	return 0;
 restore_opts:

Re: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

[Jaswinder Singh Rajput]

Hello,

On Tue, Apr 13, 2010 at 2:11 AM, Jan Blunck <jblunck@suse.de> wrote:
> Add a spinlock that protects against concurrent modifications of
> s_mount_state, s_blocks_last, s_overhead_last and the content of the
> superblock's buffer pointed to by sbi->s_es. This is a preparation patch
> for removing the BKL from ext2 in the next patch.
>
> Signed-off-by: Jan Blunck <jblunck@suse.de>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
>  fs/ext2/inode.c            |    2 ++
>  fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
>  include/linux/ext2_fs_sb.h |    6 ++++++
>  3 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> index fc13cc1..5d15442 100644
> --- a/fs/ext2/inode.c
> +++ b/fs/ext2/inode.c
> @@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
>                                * created, add a flag to the superblock.
>                                */
>                                lock_kernel();
> +                               spin_lock(&EXT2_SB(sb)->s_lock);
>                                ext2_update_dynamic_rev(sb);
>                                EXT2_SET_RO_COMPAT_FEATURE(sb,
>                                        EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> +                               spin_unlock(&EXT2_SB(sb)->s_lock);
>                                unlock_kernel();
>                                ext2_write_super(sb);

Do we need both locks (kernel lock and spin lock)

Thanks,
--
JSR
--
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

Re: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock

[Jan Blunck]

On Wed, Apr 14, Jaswinder Singh Rajput wrote:

> Hello,
> 
> On Tue, Apr 13, 2010 at 2:11 AM, Jan Blunck <jblunck@suse.de> wrote:
> > Add a spinlock that protects against concurrent modifications of
> > s_mount_state, s_blocks_last, s_overhead_last and the content of the
> > superblock's buffer pointed to by sbi->s_es. This is a preparation patch
> > for removing the BKL from ext2 in the next patch.
> >
> > Signed-off-by: Jan Blunck <jblunck@suse.de>
> > Cc: Andi Kleen <andi@firstfloor.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> > ---
> >  fs/ext2/inode.c            |    2 ++
> >  fs/ext2/super.c            |   31 +++++++++++++++++++++++++++++--
> >  include/linux/ext2_fs_sb.h |    6 ++++++
> >  3 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
> > index fc13cc1..5d15442 100644
> > --- a/fs/ext2/inode.c
> > +++ b/fs/ext2/inode.c
> > @@ -1407,9 +1407,11 @@ static int __ext2_write_inode(struct inode *inode, int do_sync)
> >                                * created, add a flag to the superblock.
> >                                */
> >                                lock_kernel();
> > +                               spin_lock(&EXT2_SB(sb)->s_lock);
> >                                ext2_update_dynamic_rev(sb);
> >                                EXT2_SET_RO_COMPAT_FEATURE(sb,
> >                                        EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
> > +                               spin_unlock(&EXT2_SB(sb)->s_lock);
> >                                unlock_kernel();
> >                                ext2_write_super(sb);
> 
> Do we need both locks (kernel lock and spin lock)
> 

The BKL is removed in a separate patch. First I though it should get merged
through Frederic's tree but since the removal does not depend on the pushdown
from do_new_mount() I guess it is safe to add here.

Thanks,
Jan
--
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

Re: [PATCH 0/5] ext2: Preparation to remove BKL

[Frederic Weisbecker]

On Mon, Apr 12, 2010 at 10:41:40PM +0200, Jan Blunck wrote:
> In this series I've collected the patches that prepare ext2 for BKL removal.
> It consist mostly of cleanups and additionally introduces a spinlock to protect
> some of the superblock's fields against concurrent access. I've addressed the
> feedback kindly provided by Ogawa-san by moving the ext2_write_super() out of
> ext2_setup_super().
> 
> These patches have been part of the BKL removal series that I have posted in
> November 2009 already. Since this is more than just removing the usage of the
> big lock I repost it separately for inclusion. This series, at least the last
> patch that includes the s_lock, needs to be merged before Frederics bkl-removal
> branch, if he merges the rest of my patches there.


It looks like this is all about .35 material.

This is going to be hard to have a separate tree to pushdown/remove
the bkl in the mount path if it depends on the ext2 tree.

What about putting that with the fs bkl removal tree? Would that conflict
with other changes in the ext2 tree?

Thanks.

Re: [PATCH 0/5] ext2: Preparation to remove BKL

[Arnd Bergmann]

On Monday 12 April 2010, Frederic Weisbecker wrote:
> It looks like this is all about .35 material.
> 
> This is going to be hard to have a separate tree to pushdown/remove
> the bkl in the mount path if it depends on the ext2 tree.

The pushdown can be in a separate tree that can be merged independently,
just the final patch removing it in do_new_mount needs to wait
for everything else to get merged first. We have some other patches,
in particular the one that makes CONFIG_BKL modular that have to
wait for other patches and should probably just be applied separately
rather than merged from a bisectable git tree.

	Arnd

Re: [PATCH 0/5] ext2: Preparation to remove BKL

[Frederic Weisbecker]

On Tue, Apr 13, 2010 at 11:31:54AM +0200, Arnd Bergmann wrote:
> On Monday 12 April 2010, Frederic Weisbecker wrote:
> > It looks like this is all about .35 material.
> > 
> > This is going to be hard to have a separate tree to pushdown/remove
> > the bkl in the mount path if it depends on the ext2 tree.
> 
> The pushdown can be in a separate tree that can be merged independently,
> just the final patch removing it in do_new_mount needs to wait
> for everything else to get merged first.


Right.



> We have some other patches,
> in particular the one that makes CONFIG_BKL modular that have to
> wait for other patches and should probably just be applied separately
> rather than merged from a bisectable git tree.
> 
> 	Arnd


Ok, no problem then.

Thanks.