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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
