Re: [PATCH] nilfs2: sync super blokcs in turns

[Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>]

Hi,
On Thu, 27 May 2010 16:27:22 +0900, Jiro SEKIBA wrote:
> Hi,
> 
> This is a proposed patch to sync super blocks by turn.
> It still require recovery action when super root is not found,

Arh, right.  We need some extension to retry the search with the older
super block.  It looks a bit complicate.

Ok, I'll take a moment to solve this issue.

> but it works at least to mount with valid fs with the patch:
> 
>  nilfs2: use checkpoint number instead of timestamp to select super block
>  9f6c75b4c354939f0a8aefc19eb6a9334ef58a89
> 
> This will sync super blocks by turn instead of syncing duplicate
> super blocks at the time.  This will help searching valid super root when
> super block is written into disk before log is written, which is happen when
> barrier-less block devices are unmounted uncleanly.
> In the stiation, old super block likely points valid log.
> 
> Signed-off-by: Jiro SEKIBA <jir-hfpbi5WX9J54Eiagz67IpQ@public.gmane.org>
> ---
>  fs/nilfs2/nilfs.h     |    2 +-
>  fs/nilfs2/segment.c   |    3 +--
>  fs/nilfs2/super.c     |   32 ++++++++++++++++----------------
>  fs/nilfs2/the_nilfs.c |    2 +-
>  4 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/nilfs2/nilfs.h b/fs/nilfs2/nilfs.h
> index 8723e5b..ec644c5 100644
> --- a/fs/nilfs2/nilfs.h
> +++ b/fs/nilfs2/nilfs.h
> @@ -270,7 +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_commit_super(struct nilfs_sb_info *, int);
> +extern int nilfs_commit_super(struct nilfs_sb_info *);
>  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..b48de1f 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2490,8 +2490,7 @@ 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_commit_super(sbi);
>  			up_write(&nilfs->ns_sem);
>  		}
>  	}
> diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
> index 48145f5..4894e07 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -101,7 +101,7 @@ void nilfs_error(struct super_block *sb, const char *function,
>  			nilfs->ns_mount_state |= NILFS_ERROR_FS;
>  			nilfs->ns_sbp[0]->s_state |=
>  				cpu_to_le16(NILFS_ERROR_FS);
> -			nilfs_commit_super(sbi, 1);
> +			nilfs_commit_super(sbi);
>  		}
>  		up_write(&nilfs->ns_sem);
>  
> @@ -200,7 +200,7 @@ static void nilfs_clear_inode(struct inode *inode)
>  	nilfs_btnode_cache_clear(&ii->i_btnode_cache);
>  }
>  
> -static int nilfs_sync_super(struct nilfs_sb_info *sbi, int dupsb)
> +static int nilfs_sync_super(struct nilfs_sb_info *sbi)
>  {
>  	struct the_nilfs *nilfs = sbi->s_nilfs;
>  	int err;
> @@ -226,6 +226,9 @@ static int nilfs_sync_super(struct nilfs_sb_info *sbi, int dupsb)
>  		printk(KERN_ERR
>  		       "NILFS: unable to write superblock (err=%d)\n", err);
>  		if (err == -EIO && nilfs->ns_sbh[1]) {
> +			memcpy(nilfs->ns_sbp[1], nilfs->ns_sbp[0],
> +			       nilfs->ns_sbsize);
> +			nilfs->ns_sbwtime[1] = nilfs->ns_sbwtime[0];
>  			nilfs_fall_back_super_block(nilfs);
>  			goto retry;
>  		}
> @@ -240,11 +243,12 @@ static int nilfs_sync_super(struct nilfs_sb_info *sbi, int dupsb)
>  
>  		/* update GC protection for recent segments */
>  		if (nilfs->ns_sbh[1]) {
> +			int flip_bits = (nilfs->ns_cno & 0x0FL);
>  			sbp = NULL;
> -			if (dupsb) {
> -				set_buffer_dirty(nilfs->ns_sbh[1]);
> -				if (!sync_dirty_buffer(nilfs->ns_sbh[1]))
> -					sbp = nilfs->ns_sbp[1];
> +			/* flip super block 9 to 7 ratio */
> +			if ((flip_bits == 0x08) || flip_bits == 0x0F) {
> +				sbp = nilfs->ns_sbp[1];
> +				nilfs_swap_super_block(nilfs);
>  			}
>  		}
>  		if (sbp) {
> @@ -257,7 +261,7 @@ 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_commit_super(struct nilfs_sb_info *sbi)
>  {
>  	struct the_nilfs *nilfs = sbi->s_nilfs;
>  	struct nilfs_super_block **sbp = nilfs->ns_sbp;
> @@ -294,12 +298,8 @@ int nilfs_commit_super(struct nilfs_sb_info *sbi, int dupsb)
>  	sbp[0]->s_sum = cpu_to_le32(crc32_le(nilfs->ns_crc_seed,
>  					     (unsigned char *)sbp[0],
>  					     nilfs->ns_sbsize));
> -	if (dupsb && sbp[1]) {
> -		memcpy(sbp[1], sbp[0], nilfs->ns_sbsize);
> -		nilfs->ns_sbwtime[1] = t;
> -	}
>  	clear_nilfs_sb_dirty(nilfs);
> -	return nilfs_sync_super(sbi, dupsb);
> +	return nilfs_sync_super(sbi);
>  }
>  
>  static void nilfs_put_super(struct super_block *sb)
> @@ -314,7 +314,7 @@ 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);
> -		nilfs_commit_super(sbi, 1);
> +		nilfs_commit_super(sbi);
>  		up_write(&nilfs->ns_sem);
>  	}
>  	down_write(&nilfs->ns_super_sem);
> @@ -343,7 +343,7 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
>  
>  	down_write(&nilfs->ns_sem);
>  	if (nilfs_sb_dirty(nilfs))
> -		nilfs_commit_super(sbi, 1);
> +		nilfs_commit_super(sbi);
>  	up_write(&nilfs->ns_sem);
>  
>  	return err;
> @@ -657,7 +657,7 @@ static int nilfs_setup_super(struct nilfs_sb_info *sbi)
>  	sbp->s_mnt_count = cpu_to_le16(mnt_count + 1);
>  	sbp->s_state = cpu_to_le16(le16_to_cpu(sbp->s_state) & ~NILFS_VALID_FS);
>  	sbp->s_mtime = cpu_to_le64(get_seconds());
> -	return nilfs_commit_super(sbi, 1);
> +	return nilfs_commit_super(sbi);
>  }
>  
>  struct nilfs_super_block *nilfs_read_super_block(struct super_block *sb,
> @@ -901,7 +901,7 @@ static int nilfs_remount(struct super_block *sb, int *flags, char *data)
>  		    (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);
> +		nilfs_commit_super(sbi);
>  		up_write(&nilfs->ns_sem);
>  	} else {
>  		/*
> diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
> index a756168..f779363 100644
> --- a/fs/nilfs2/the_nilfs.c
> +++ b/fs/nilfs2/the_nilfs.c
> @@ -327,7 +327,7 @@ int load_nilfs(struct the_nilfs *nilfs, struct nilfs_sb_info *sbi)
>  	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_commit_super(sbi);
>  	up_write(&nilfs->ns_sem);
>  
>  	if (err) {
> -- 
> 1.5.6.5


Well, the basic idea looks OK.

But, this patch has an issue.

When the filesystem is mounted, nilfs updates super blocks to drop
their VALID_FS flags, and this state continues until the partition
will be unmounted.

The point is that the checkpoint number does not change when the
VALID_FS flags are set or unset.

If the system goes down while the VALID_FS flag on the secondary super
block is still on, it may prevent roll-forward recovery because the
VALID_FS flag forces to skip the recovery.

So, we still have to drop the VALID_FS flag together from both super
blocks.

Or, we may as well stop using the VALID_FS flag.  Nilfs can know
whether the recovery is needed or not by scanning logs without the
VALID_FS flag.

> +			/* flip super block 9 to 7 ratio */
> +			if ((flip_bits == 0x08) || flip_bits == 0x0F) {
> +				sbp = nilfs->ns_sbp[1];
> +				nilfs_swap_super_block(nilfs);
>  			}

This twist looks well thought out.  But, one comment.  From the coding
style viewpoint, it should be as follows:

> +			if (flip_bits == 0x08 || flip_bits == 0x0F) {

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