lkml.org 
[lkml]   [2010]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 5/5] ext2: Add ext2_sb_info s_lock spinlock
    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:

    \
     
     \ /
      Last update: 2010-04-14 10:23    [W:0.067 / U:0.372 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site