Hi Ju Hyung, This is the change on tools part. ;)
Thanks, On 2019/5/14 17:38, Ju Hyung Park wrote: > Hi Chao, > > I believe Jaegeuk already queued v2 for Linus, I think you should probably > make this as a separate patch. > > Thanks. > > On Tue, May 14, 2019, 6:35 PM Chao Yu <[email protected]> wrote: > >> For large_nat_bitmap feature, there is a design flaw: >> >> Previous: >> >> struct f2fs_checkpoint layout: >> +--------------------------+ 0x0000 >> | checkpoint_ver | >> | ...... | >> | checksum_offset |------+ >> | ...... | | >> | sit_nat_version_bitmap[] |<-----|-------+ >> | ...... | | | >> | checksum_value |<-----+ | >> +--------------------------+ 0x1000 | >> | | nat_bitmap + sit_bitmap >> | payload blocks | | >> | | | >> +--------------------------|<-------------+ >> >> Obviously, if nat_bitmap size + sit_bitmap size is larger than >> MAX_BITMAP_SIZE_IN_CKPT, nat_bitmap or sit_bitmap may overlap >> checkpoint checksum's position, once checkpoint() is triggered >> from kernel, nat or sit bitmap will be damaged by checksum field. >> >> In order to fix this, let's relocate checksum_value's position >> to the head of sit_nat_version_bitmap as below, then nat/sit >> bitmap and chksum value update will become safe. >> >> After: >> >> struct f2fs_checkpoint layout: >> +--------------------------+ 0x0000 >> | checkpoint_ver | >> | ...... | >> | checksum_offset |------+ >> | ...... | | >> | sit_nat_version_bitmap[] |<-----+ >> | ...... |<-------------+ >> | | | >> +--------------------------+ 0x1000 | >> | | nat_bitmap + sit_bitmap >> | payload blocks | | >> | | | >> +--------------------------|<-------------+ >> >> Related report and discussion: >> >> https://sourceforge.net/p/linux-f2fs/mailman/message/36642346/ >> >> In addition, during writing checkpoint, if large_nat_bitmap feature is >> enabled, we need to set CP_LARGE_NAT_BITMAP_FLAG flag in checkpoint. >> >> Reported-by: Park Ju Hyung <[email protected]> >> Signed-off-by: Chao Yu <[email protected]> >> --- >> v3: >> - if large_nat_bitmap is off, fix to configure checksum_offset to >> CP_CHKSUM_OFFSET. >> fsck/f2fs.h | 9 ++++++++- >> fsck/fsck.c | 37 +++++++++++++++++++++++++++++++++++++ >> fsck/fsck.h | 1 + >> fsck/main.c | 2 ++ >> fsck/mount.c | 9 ++++++++- >> fsck/resize.c | 5 +++++ >> include/f2fs_fs.h | 10 ++++++++-- >> mkfs/f2fs_format.c | 5 ++++- >> 8 files changed, 73 insertions(+), 5 deletions(-) >> >> diff --git a/fsck/f2fs.h b/fsck/f2fs.h >> index 93f01e5..4dc6698 100644 >> --- a/fsck/f2fs.h >> +++ b/fsck/f2fs.h >> @@ -270,9 +270,16 @@ static inline void *__bitmap_ptr(struct f2fs_sb_info >> *sbi, int flag) >> int offset; >> >> if (is_set_ckpt_flags(ckpt, CP_LARGE_NAT_BITMAP_FLAG)) { >> + unsigned int chksum_size = 0; >> + >> offset = (flag == SIT_BITMAP) ? >> le32_to_cpu(ckpt->nat_ver_bitmap_bytesize) : 0; >> - return &ckpt->sit_nat_version_bitmap + offset; >> + >> + if (le32_to_cpu(ckpt->checksum_offset) == >> + CP_MIN_CHKSUM_OFFSET) >> + chksum_size = sizeof(__le32); >> + >> + return &ckpt->sit_nat_version_bitmap + offset + >> chksum_size; >> } >> >> if (le32_to_cpu(F2FS_RAW_SUPER(sbi)->cp_payload) > 0) { >> diff --git a/fsck/fsck.c b/fsck/fsck.c >> index a8c8923..b5daeb4 100644 >> --- a/fsck/fsck.c >> +++ b/fsck/fsck.c >> @@ -1917,6 +1917,19 @@ int fsck_chk_meta(struct f2fs_sb_info *sbi) >> return 0; >> } >> >> +void fsck_chk_checkpoint(struct f2fs_sb_info *sbi) >> +{ >> + struct f2fs_checkpoint *cp = F2FS_CKPT(sbi); >> + >> + if (get_cp(ckpt_flags) & CP_LARGE_NAT_BITMAP_FLAG) { >> + if (get_cp(checksum_offset) != CP_MIN_CHKSUM_OFFSET) { >> + ASSERT_MSG("Deprecated layout of large_nat_bitmap, >> " >> + "chksum_offset:%u", >> get_cp(checksum_offset)); >> + c.fix_chksum = 1; >> + } >> + } >> +} >> + >> void fsck_init(struct f2fs_sb_info *sbi) >> { >> struct f2fs_fsck *fsck = F2FS_FSCK(sbi); >> @@ -2017,6 +2030,23 @@ static void flush_curseg_sit_entries(struct >> f2fs_sb_info *sbi) >> free(sit_blk); >> } >> >> +static void fix_checksum(struct f2fs_sb_info *sbi) >> +{ >> + struct f2fs_checkpoint *cp = F2FS_CKPT(sbi); >> + struct f2fs_nm_info *nm_i = NM_I(sbi); >> + struct sit_info *sit_i = SIT_I(sbi); >> + void *bitmap_offset; >> + >> + if (!c.fix_chksum) >> + return; >> + >> + bitmap_offset = cp->sit_nat_version_bitmap + sizeof(__le32); >> + >> + memcpy(bitmap_offset, nm_i->nat_bitmap, nm_i->bitmap_size); >> + memcpy(bitmap_offset + nm_i->bitmap_size, >> + sit_i->sit_bitmap, sit_i->bitmap_size); >> +} >> + >> static void fix_checkpoint(struct f2fs_sb_info *sbi) >> { >> struct f2fs_fsck *fsck = F2FS_FSCK(sbi); >> @@ -2038,6 +2068,12 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi) >> flags |= CP_TRIMMED_FLAG; >> if (is_set_ckpt_flags(cp, CP_DISABLED_FLAG)) >> flags |= CP_DISABLED_FLAG; >> + if (is_set_ckpt_flags(cp, CP_LARGE_NAT_BITMAP_FLAG)) { >> + flags |= CP_LARGE_NAT_BITMAP_FLAG; >> + set_cp(checksum_offset, CP_MIN_CHKSUM_OFFSET); >> + } else { >> + set_cp(checksum_offset, CP_CHKSUM_OFFSET); >> + } >> >> if (flags & CP_UMOUNT_FLAG) >> cp_blocks = 8; >> @@ -2717,6 +2753,7 @@ int fsck_verify(struct f2fs_sb_info *sbi) >> write_curseg_info(sbi); >> flush_curseg_sit_entries(sbi); >> } >> + fix_checksum(sbi); >> fix_checkpoint(sbi); >> } else if (is_set_ckpt_flags(cp, CP_FSCK_FLAG) || >> is_set_ckpt_flags(cp, CP_QUOTA_NEED_FSCK_FLAG)) { >> diff --git a/fsck/fsck.h b/fsck/fsck.h >> index cbd6e93..c8802b0 100644 >> --- a/fsck/fsck.h >> +++ b/fsck/fsck.h >> @@ -154,6 +154,7 @@ extern int fsck_chk_dentry_blk(struct f2fs_sb_info *, >> u32, struct child_info *, >> int, int); >> int fsck_chk_inline_dentries(struct f2fs_sb_info *, struct f2fs_node *, >> struct child_info *); >> +void fsck_chk_checkpoint(struct f2fs_sb_info *sbi); >> int fsck_chk_meta(struct f2fs_sb_info *sbi); >> int fsck_chk_curseg_info(struct f2fs_sb_info *); >> int convert_encrypted_name(unsigned char *, u32, unsigned char *, int); >> diff --git a/fsck/main.c b/fsck/main.c >> index 03076d9..afdfec9 100644 >> --- a/fsck/main.c >> +++ b/fsck/main.c >> @@ -616,6 +616,8 @@ static void do_fsck(struct f2fs_sb_info *sbi) >> c.fix_on = 1; >> } >> >> + fsck_chk_checkpoint(sbi); >> + >> fsck_chk_quota_node(sbi); >> >> /* Traverse all block recursively from root inode */ >> diff --git a/fsck/mount.c b/fsck/mount.c >> index 95c5357..5a0955e 100644 >> --- a/fsck/mount.c >> +++ b/fsck/mount.c >> @@ -774,7 +774,8 @@ static int verify_checksum_chksum(struct >> f2fs_checkpoint *cp) >> unsigned int chksum_offset = get_cp(checksum_offset); >> unsigned int crc, cal_crc; >> >> - if (chksum_offset > CP_CHKSUM_OFFSET) { >> + if (chksum_offset < CP_MIN_CHKSUM_OFFSET || >> + chksum_offset > CP_CHKSUM_OFFSET) { >> MSG(0, "\tInvalid CP CRC offset: %u\n", chksum_offset); >> return -1; >> } >> @@ -2372,6 +2373,12 @@ void write_checkpoint(struct f2fs_sb_info *sbi) >> flags |= CP_TRIMMED_FLAG; >> if (is_set_ckpt_flags(cp, CP_DISABLED_FLAG)) >> flags |= CP_DISABLED_FLAG; >> + if (is_set_ckpt_flags(cp, CP_LARGE_NAT_BITMAP_FLAG)) { >> + flags |= CP_LARGE_NAT_BITMAP_FLAG; >> + set_cp(checksum_offset, CP_MIN_CHKSUM_OFFSET); >> + } else { >> + set_cp(checksum_offset, CP_CHKSUM_OFFSET); >> + } >> >> set_cp(free_segment_count, get_free_segments(sbi)); >> set_cp(valid_block_count, sbi->total_valid_block_count); >> diff --git a/fsck/resize.c b/fsck/resize.c >> index 5537a73..fc563f2 100644 >> --- a/fsck/resize.c >> +++ b/fsck/resize.c >> @@ -514,6 +514,11 @@ static void rebuild_checkpoint(struct f2fs_sb_info >> *sbi, >> flags = update_nat_bits_flags(new_sb, cp, get_cp(ckpt_flags)); >> if (flags & CP_COMPACT_SUM_FLAG) >> flags &= ~CP_COMPACT_SUM_FLAG; >> + if (flags & CP_LARGE_NAT_BITMAP_FLAG) >> + set_cp(checksum_offset, CP_MIN_CHKSUM_OFFSET); >> + else >> + set_cp(checksum_offset, CP_CHKSUM_OFFSET); >> + >> set_cp(ckpt_flags, flags); >> >> memcpy(new_cp, cp, (unsigned char *)cp->sit_nat_version_bitmap - >> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h >> index e0a4cbf..84a4e55 100644 >> --- a/include/f2fs_fs.h >> +++ b/include/f2fs_fs.h >> @@ -382,6 +382,7 @@ struct f2fs_configuration { >> int ro; >> int preserve_limits; /* preserve quota limits */ >> int large_nat_bitmap; >> + int fix_chksum; /* fix old cp.chksum position */ >> __le32 feature; /* defined features */ >> >> /* mkfs parameters */ >> @@ -692,10 +693,15 @@ struct f2fs_checkpoint { >> unsigned char sit_nat_version_bitmap[1]; >> } __attribute__((packed)); >> >> +#define CP_BITMAP_OFFSET \ >> + (offsetof(struct f2fs_checkpoint, sit_nat_version_bitmap)) >> +#define CP_MIN_CHKSUM_OFFSET CP_BITMAP_OFFSET >> + >> +#define MIN_NAT_BITMAP_SIZE 64 >> #define MAX_SIT_BITMAP_SIZE_IN_CKPT \ >> - (CP_CHKSUM_OFFSET - sizeof(struct f2fs_checkpoint) + 1 - 64) >> + (CP_CHKSUM_OFFSET - CP_BITMAP_OFFSET - MIN_NAT_BITMAP_SIZE) >> #define MAX_BITMAP_SIZE_IN_CKPT \ >> - (CP_CHKSUM_OFFSET - sizeof(struct f2fs_checkpoint) + 1) >> + (CP_CHKSUM_OFFSET - CP_BITMAP_OFFSET) >> >> /* >> * For orphan inode management >> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c >> index ab8103c..ed27700 100644 >> --- a/mkfs/f2fs_format.c >> +++ b/mkfs/f2fs_format.c >> @@ -690,7 +690,10 @@ static int f2fs_write_check_point_pack(void) >> set_cp(nat_ver_bitmap_bytesize, ((get_sb(segment_count_nat) / 2) << >> get_sb(log_blocks_per_seg)) / 8); >> >> - set_cp(checksum_offset, CP_CHKSUM_OFFSET); >> + if (c.large_nat_bitmap) >> + set_cp(checksum_offset, CP_MIN_CHKSUM_OFFSET); >> + else >> + set_cp(checksum_offset, CP_CHKSUM_OFFSET); >> >> crc = f2fs_checkpoint_chksum(cp); >> *((__le32 *)((unsigned char *)cp + get_cp(checksum_offset))) = >> -- >> 2.18.0.rc1 >> >> >> >> _______________________________________________ >> Linux-f2fs-devel mailing list >> [email protected] >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel >> > _______________________________________________ Linux-f2fs-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
