Add a mutex that protects the ext2_sb_info from concurrent modifications.
This is a preparation for removing the BKL later.
Signed-off-by: Jan Blunck <[email protected]>
---
fs/ext2/inode.c | 2 ++
fs/ext2/super.c | 30 +++++++++++++++++++++++++++---
include/linux/ext2_fs_sb.h | 2 ++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c
index ade6340..057074e 100644
--- a/fs/ext2/inode.c
+++ b/fs/ext2/inode.c
@@ -1401,9 +1401,11 @@ int ext2_write_inode(struct inode *inode, int do_sync)
* created, add a flag to the superblock.
*/
lock_kernel();
+ mutex_lock(&EXT2_SB(sb)->s_mutex);
ext2_update_dynamic_rev(sb);
EXT2_SET_RO_COMPAT_FEATURE(sb,
EXT2_FEATURE_RO_COMPAT_LARGE_FILE);
+ mutex_unlock(&EXT2_SB(sb)->s_mutex);
unlock_kernel();
ext2_write_super(sb);
}
diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index 5af1775..d610eb9 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -84,6 +84,9 @@ void ext2_warning (struct super_block * sb, const char * function,
va_end(args);
}
+/*
+ * This must be called with sbi->s_mutex held.
+ */
void ext2_update_dynamic_rev(struct super_block *sb)
{
struct ext2_super_block *es = EXT2_SB(sb)->s_es;
@@ -117,8 +120,8 @@ static void ext2_put_super (struct super_block * sb)
lock_kernel();
- if (sb->s_dirt)
- ext2_write_super(sb);
+ if (sb->s_dirt && !(sb->s_flags & MS_RDONLY))
+ ext2_sync_fs(sb, 1);
ext2_xattr_put_super(sb);
if (!(sb->s_flags & MS_RDONLY)) {
@@ -207,6 +210,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;
+ mutex_lock(&sbi->s_mutex);
def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
if (sbi->s_sb_block != 1)
@@ -279,6 +283,7 @@ static int ext2_show_options(struct seq_file *seq, struct vfsmount *vfs)
if (!test_opt(sb, RESERVATION))
seq_puts(seq, ",noreservation");
+ mutex_unlock(&sbi->s_mutex);
return 0;
}
@@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
sbi->s_sb_block = sb_block;
/*
+ * mutex for protection of modifications of the superblock while being
+ * write out by ext2_write_super() or ext2_sync_fs().
+ */
+ mutex_init(&sbi->s_mutex);
+
+ /*
* See what the current blocksize for the device is, and
* use that as the blocksize. Otherwise (or if the blocksize
* is smaller than the default) use the default.
@@ -1122,8 +1133,9 @@ static void ext2_sync_super(struct super_block *sb, struct ext2_super_block *es)
* flags to 0. We need to set this flag to 0 since the fs
* may have been checked while mounted and e2fsck may have
* set s_state to EXT2_VALID_FS after some corrections.
+ *
+ * This must be called with sbi->s_mutex held.
*/
-
static int ext2_sync_fs(struct super_block *sb, int wait)
{
struct ext2_super_block *es = EXT2_SB(sb)->s_es;
@@ -1150,10 +1162,14 @@ static int ext2_sync_fs(struct super_block *sb, int wait)
void ext2_write_super(struct super_block *sb)
{
+ struct ext2_sb_info * sbi = EXT2_SB(sb);
+
+ mutex_lock(&sbi->s_mutex);
if (!(sb->s_flags & MS_RDONLY))
ext2_sync_fs(sb, 1);
else
sb->s_dirt = 0;
+ mutex_unlock(&sbi->s_mutex);
}
static int ext2_remount (struct super_block * sb, int * flags, char * data)
@@ -1166,6 +1182,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
int err;
lock_kernel();
+ mutex_lock(&sbi->s_mutex);
/* Store the old options */
old_sb_flags = sb->s_flags;
@@ -1203,12 +1220,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)) {
+ mutex_unlock(&sbi->s_mutex);
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)) {
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return 0;
}
@@ -1238,6 +1257,7 @@ static int ext2_remount (struct super_block * sb, int * flags, char * data)
sb->s_flags &= ~MS_RDONLY;
}
ext2_sync_super(sb, es);
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return 0;
restore_opts:
@@ -1245,6 +1265,7 @@ restore_opts:
sbi->s_resuid = old_opts.s_resuid;
sbi->s_resgid = old_opts.s_resgid;
sb->s_flags = old_sb_flags;
+ mutex_unlock(&sbi->s_mutex);
unlock_kernel();
return err;
}
@@ -1256,6 +1277,8 @@ static int ext2_statfs (struct dentry * dentry, struct kstatfs * buf)
struct ext2_super_block *es = sbi->s_es;
u64 fsid;
+ mutex_lock(&sbi->s_mutex);
+
if (test_opt (sb, MINIX_DF))
sbi->s_overhead_last = 0;
else if (sbi->s_blocks_last != le32_to_cpu(es->s_blocks_count)) {
@@ -1310,6 +1333,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;
+ mutex_unlock(&sbi->s_mutex);
return 0;
}
diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
index 1cdb663..8518361 100644
--- a/include/linux/ext2_fs_sb.h
+++ b/include/linux/ext2_fs_sb.h
@@ -106,6 +106,8 @@ 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;
+ /* protect against concurrent modifications of this structure */
+ struct mutex s_mutex;
};
static inline spinlock_t *
--
1.6.4.2
> @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> sbi->s_sb_block = sb_block;
>
> /*
> + * mutex for protection of modifications of the superblock while being
> + * write out by ext2_write_super() or ext2_sync_fs().
> + */
> + mutex_init(&sbi->s_mutex);
I didn't go over all the code paths in detail, but if you replace
the BKL with a mutex that is hold over a longer write-out sleep
period you potentially limit IO parallelism a lot.
In this case since the BKL didn't protect over the sleep anyways it might
be reasonable to drop it during the IO operation (and possibly if
you're sure nothing else sleeps use a spin lock)
-Andi
On Mon, Nov 02, Andi Kleen wrote:
> > @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > sbi->s_sb_block = sb_block;
> >
> > /*
> > + * mutex for protection of modifications of the superblock while being
> > + * write out by ext2_write_super() or ext2_sync_fs().
> > + */
> > + mutex_init(&sbi->s_mutex);
>
> I didn't go over all the code paths in detail, but if you replace
> the BKL with a mutex that is hold over a longer write-out sleep
> period you potentially limit IO parallelism a lot.
Right. I converted it to be a spinlock and unlock before calling
ext2_sync_super().
What do you think?
Thanks,
Jan
> Right. I converted it to be a spinlock and unlock before calling
> ext2_sync_super().
>
> What do you think?
Again didn't go through all the code paths, but it seems like a good
approach.
-Andi
On Mon 02-11-09 17:57:52, Jan Blunck wrote:
> On Mon, Nov 02, Andi Kleen wrote:
>
> > > @@ -762,6 +767,12 @@ static int ext2_fill_super(struct super_block *sb, void *data, int silent)
> > > sbi->s_sb_block = sb_block;
> > >
> > > /*
> > > + * mutex for protection of modifications of the superblock while being
> > > + * write out by ext2_write_super() or ext2_sync_fs().
> > > + */
> > > + mutex_init(&sbi->s_mutex);
> >
> > I didn't go over all the code paths in detail, but if you replace
> > the BKL with a mutex that is hold over a longer write-out sleep
> > period you potentially limit IO parallelism a lot.
>
> Right. I converted it to be a spinlock and unlock before calling
> ext2_sync_super().
>
> What do you think?
The patch is generally fine. I have just a few minor comments below:
> diff --git a/fs/ext2/super.c b/fs/ext2/super.c
> index 5af1775..70c326c 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);
> + /* drops sbi->s_lock */
> ext2_sync_super(sb, es);
I don't like this dropping of spinlock inside ext2_sync_super. Can we
just drop it here and retake it in ext2_sync_super? It's by far not a
performance critical path so it should not really matter.
> diff --git a/include/linux/ext2_fs_sb.h b/include/linux/ext2_fs_sb.h
> index 1cdb663..0d20278 100644
> --- a/include/linux/ext2_fs_sb.h
> +++ b/include/linux/ext2_fs_sb.h
> @@ -106,6 +106,8 @@ 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;
> + /* protect against concurrent modifications of this structure */
> + spinlock_t s_lock;
> };
As I'm reading the code s_lock protects some of the fieds but definitely
not all. I'd say it protects s_mount_state, s_blocks_last, s_overhead_last,
and a content of superblock's buffer pointed to by sbi->s_es. The rest just
either does not change during lifetime of the filesystem or has different
locks (either s_umount semaphore or other spinlocks).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR