Hi Chao, The new logs are at http://arter97.com/f2fs/v3
The first run spits out 60MB worth of log and the second run spits out 170MB worth of log and the third run now returns everything's ok. Returning ok after 2nd run is good news, but the user would expect everything to be fixed with just the first run of fsck'ing. Is this expected? I'll test the new kernel soon. Thanks. On Tue, May 14, 2019 at 6:54 PM Ju Hyung Park <[email protected]> wrote: > > Ah, sorry. I typed too soon. > > I'll make sure to test this sooner than later. > Sorry for the delay. > > Thanks :) > > On Tue, May 14, 2019, 6:43 PM Chao Yu <[email protected]> wrote: >> >> 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
