Re: [PATCH 1/2] nilfs2: introduce nilfs_prepare_super

[Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>]

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 <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> 
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html