Hi, thank you for the comments. I'll revise it.
At Sun, 06 Jun 2010 20:12:05 +0900 (JST), Ryusuke Konishi wrote: > > Hi, > On Sun, 6 Jun 2010 07:19:08 +0900, Jiro SEKIBA wrote: > > This function checks validity of super block pointer. > > If first super block is invalid, it will swap the super blocks. > > The function should be called before any super block information updates. > > Caller must obtain nilfs->ns_sem. > > > > Signed-off-by: Jiro SEKIBA <[email protected]> > > Thank you for the post. > > I'll comment inline. > > > --- > > fs/nilfs2/nilfs.h | 1 + > > fs/nilfs2/segment.c | 6 +++- > > fs/nilfs2/super.c | 58 > > ++++++++++++++++++++++++++++++++++-------------- > > fs/nilfs2/the_nilfs.c | 9 +++++-- > > 4 files changed, 52 insertions(+), 22 deletions(-) > > > > diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h > > index 8723e5b..c8edffd 100644 > > --- a/fs/nilfs2/nilfs.h > > +++ b/fs/nilfs2/nilfs.h > > @@ -270,6 +270,7 @@ extern struct nilfs_super_block * > > nilfs_read_super_block(struct super_block *, u64, int, struct buffer_head > > **); > > extern int nilfs_store_magic_and_option(struct super_block *, > > struct nilfs_super_block *, char *); > > +extern int nilfs_prepare_super(struct nilfs_sb_info *); > > extern int nilfs_commit_super(struct nilfs_sb_info *, int); > > extern int nilfs_attach_checkpoint(struct nilfs_sb_info *, __u64); > > extern void nilfs_detach_checkpoint(struct nilfs_sb_info *); > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c > > index 6a7dbd8..72f779f 100644 > > --- a/fs/nilfs2/segment.c > > +++ b/fs/nilfs2/segment.c > > @@ -2490,8 +2490,10 @@ static int nilfs_segctor_construct(struct > > nilfs_sc_info *sci, int mode) > > if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) && > > nilfs_discontinued(nilfs)) { > > down_write(&nilfs->ns_sem); > > - err = nilfs_commit_super( > > - sbi, nilfs_altsb_need_update(nilfs)); > > + err = nilfs_prepare_super(sbi); > > + if (likely(!err)) > > + err = nilfs_commit_super( > > + sbi, nilfs_altsb_need_update(nilfs)); > > up_write(&nilfs->ns_sem); > > } > > } > > diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c > > index 48145f5..312b34a 100644 > > --- a/fs/nilfs2/super.c > > +++ b/fs/nilfs2/super.c > > @@ -97,7 +97,8 @@ void nilfs_error(struct super_block *sb, const char > > *function, > > struct the_nilfs *nilfs = sbi->s_nilfs; > > > > down_write(&nilfs->ns_sem); > > - if (!(nilfs->ns_mount_state & NILFS_ERROR_FS)) { > > + if (!(nilfs->ns_mount_state & NILFS_ERROR_FS) && > > + !nilfs_prepare_super(sbi)) { > > nilfs->ns_mount_state |= NILFS_ERROR_FS; > > nilfs->ns_sbp[0]->s_state |= > > cpu_to_le16(NILFS_ERROR_FS); > > @@ -257,14 +258,10 @@ static int nilfs_sync_super(struct nilfs_sb_info > > *sbi, int dupsb) > > return err; > > } > > > > -int nilfs_commit_super(struct nilfs_sb_info *sbi, int dupsb) > > +int nilfs_prepare_super(struct nilfs_sb_info *sbi) > > { > > struct the_nilfs *nilfs = sbi->s_nilfs; > > struct nilfs_super_block **sbp = nilfs->ns_sbp; > > - sector_t nfreeblocks; > > - time_t t; > > - int err; > > - > > /* nilfs->sem must be locked by the caller. */ > > This comment is copying a typo. Please use precise one Wow, OK I'll fix it. It's the original comment, though :). > (i.e. "nilfs->ns_sem"). And, do not omit an empty line just below the > local variable declarations. > > > if (sbp[0]->s_magic != NILFS_SUPER_MAGIC) { > > if (sbp[1] && sbp[1]->s_magic == NILFS_SUPER_MAGIC) > > @@ -275,6 +272,18 @@ int nilfs_commit_super(struct nilfs_sb_info *sbi, int > > dupsb) > > return -EIO; > > } > > } > > + return 0; > > +} > > + > > +int nilfs_commit_super(struct nilfs_sb_info *sbi, int dupsb) > > +{ > > + struct the_nilfs *nilfs = sbi->s_nilfs; > > + struct nilfs_super_block **sbp = nilfs->ns_sbp; > > + sector_t nfreeblocks; > > + time_t t; > > + int err; > > + > > + /* nilfs->sem must be locked by the caller. */ > > err = nilfs_count_free_blocks(nilfs, &nfreeblocks); > > if (unlikely(err)) { > > printk(KERN_ERR "NILFS: failed to count free blocks\n"); > > @@ -314,7 +323,8 @@ static void nilfs_put_super(struct super_block *sb) > > if (!(sb->s_flags & MS_RDONLY)) { > > down_write(&nilfs->ns_sem); > > nilfs->ns_sbp[0]->s_state = cpu_to_le16(nilfs->ns_mount_state); > > This change is not protected by the nilfs_{prepare,commit}_super pair. > > > - nilfs_commit_super(sbi, 1); > > + if (likely(!nilfs_prepare_super(sbi))) > > + nilfs_commit_super(sbi, 1); > > up_write(&nilfs->ns_sem); > > } > > down_write(&nilfs->ns_super_sem); > > @@ -342,7 +352,7 @@ static int nilfs_sync_fs(struct super_block *sb, int > > wait) > > err = nilfs_construct_segment(sb); > > > > down_write(&nilfs->ns_sem); > > - if (nilfs_sb_dirty(nilfs)) > > + if (nilfs_sb_dirty(nilfs) && likely(!nilfs_prepare_super(sbi))) > > nilfs_commit_super(sbi, 1); > > up_write(&nilfs->ns_sem); > > > > @@ -637,9 +647,20 @@ nilfs_set_default_options(struct nilfs_sb_info *sbi, > > static int nilfs_setup_super(struct nilfs_sb_info *sbi) > > { > > struct the_nilfs *nilfs = sbi->s_nilfs; > > - struct nilfs_super_block *sbp = nilfs->ns_sbp[0]; > > - int max_mnt_count = le16_to_cpu(sbp->s_max_mnt_count); > > - int mnt_count = le16_to_cpu(sbp->s_mnt_count); > > + struct nilfs_super_block *sbp; > > + int max_mnt_count; > > + int mnt_count; > > + int err; > > + > > + err = nilfs_prepare_super(sbi); > > + > > This empty line should be removed. > > > + if (unlikely(err)) > > + return err; > > This usage of unlikely macro is superfluous since gcc generally > considers conditions to blocks that return unlikely. > > It's still seen in places, but I hope they will be rewritten whenever > possible. > > > + > > + sbp = nilfs->ns_sbp[0]; > > + max_mnt_count = le16_to_cpu(sbp->s_max_mnt_count); > > + mnt_count = le16_to_cpu(sbp->s_mnt_count); > > + > > > > /* nilfs->sem must be locked by the caller. */ > > if (nilfs->ns_mount_state & NILFS_ERROR_FS) { > > @@ -896,12 +917,15 @@ static int nilfs_remount(struct super_block *sb, int > > *flags, char *data) > > * the RDONLY flag and then mark the partition as valid again. > > */ > > down_write(&nilfs->ns_sem); > > - sbp = nilfs->ns_sbp[0]; > > - if (!(sbp->s_state & le16_to_cpu(NILFS_VALID_FS)) && > > - (nilfs->ns_mount_state & NILFS_VALID_FS)) > > - sbp->s_state = cpu_to_le16(nilfs->ns_mount_state); > > - sbp->s_mtime = cpu_to_le64(get_seconds()); > > - nilfs_commit_super(sbi, 1); > > + if (likely(!nilfs_prepare_super(sbi))) { > > + sbp = nilfs->ns_sbp[0]; > > + if (!(sbp->s_state & le16_to_cpu(NILFS_VALID_FS)) && > > + (nilfs->ns_mount_state & NILFS_VALID_FS)) > > + sbp->s_state = > > + cpu_to_le16(nilfs->ns_mount_state); > > + sbp->s_mtime = cpu_to_le64(get_seconds()); > > + nilfs_commit_super(sbi, 1); > > + } > > How about letting nilfs_prepare_super return sbp instead of a status > code ? > > It seems fine if the reference of nilfs->ns_sbp[0] is hidden from the > callers. Ah, OK. I was thinking the way, but wanted to get return code. But actually prepare_super only returns EIO for error, so we can use NULL in case it got EIO. And caller can set code as EIO in the case. much better. thanks, regards, > > up_write(&nilfs->ns_sem); > > } else { > > /* > > diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c > > index a756168..636cefe 100644 > > --- a/fs/nilfs2/the_nilfs.c > > +++ b/fs/nilfs2/the_nilfs.c > > @@ -325,9 +325,12 @@ int load_nilfs(struct the_nilfs *nilfs, struct > > nilfs_sb_info *sbi) > > goto failed_unload; > > > > down_write(&nilfs->ns_sem); > > - nilfs->ns_mount_state |= NILFS_VALID_FS; > > - nilfs->ns_sbp[0]->s_state = cpu_to_le16(nilfs->ns_mount_state); > > - err = nilfs_commit_super(sbi, 1); > > + err = nilfs_prepare_super(sbi); > > + if (likely(!err)) { > > + nilfs->ns_mount_state |= NILFS_VALID_FS; > > + nilfs->ns_sbp[0]->s_state = cpu_to_le16(nilfs->ns_mount_state); > > + err = nilfs_commit_super(sbi, 1); > > + } > > up_write(&nilfs->ns_sem); > > > > if (err) { > > -- > > 1.5.6.5 > > Thanks, > Ryusuke Konishi > -- > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in > the body of a message to [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Jiro SEKIBA <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
