Skip to contentSkip to conversation

f2fs: introduce get_checkpoint_version for cleanup

PATCH Superseded Tiezhu Yang 2 participants 3 messages Sep 25, 2016 – Sep 26, 2016 F2FS file system

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Tiezhu Yang

author
#0 lore raw

PATCH 1/1 f2fs: introduce get_checkpoint_version for cleanup

There exists almost same codes when get the value of pre_version and cur_version in function validate_checkpoint, this patch adds get_checkpoint_version to clean up redundant codes.

Signed-off-by Tiezhu Yang <[email protected]>
---
 fs/f2fs/checkpoint.c | 52 ++++++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 24 deletions(-)
fs/f2fs/checkpoint.c +28 -24
index a655d75..facede2 100644
@@ -666,45 +666,49 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
 	}
 }
 
-static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
-				block_t cp_addr, unsigned long long *version)
+static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
+			struct f2fs_checkpoint *cp_block, struct page *cp_page,
+			unsigned long long *version)
 {
-	struct page *cp_page_1, *cp_page_2 = NULL;
 	unsigned long blk_size = sbi->blocksize;
-	struct f2fs_checkpoint *cp_block;
-	unsigned long long cur_version = 0, pre_version = 0;
-	size_t crc_offset;
+	size_t crc_offset = 0;
 	__u32 crc = 0;
 
-	/* Read the 1st cp block in this CP pack */
-	cp_page_1 = get_meta_page(sbi, cp_addr);
+	cp_page = get_meta_page(sbi, cp_addr);
+	cp_block = (struct f2fs_checkpoint *)page_address(cp_page);
 
-	/* get the version number */
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_1);
 	crc_offset = le32_to_cpu(cp_block->checksum_offset);
 	if (crc_offset >= blk_size)
-		goto invalid_cp1;
+		return -1;
 
 	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
 	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
-		goto invalid_cp1;
+		return -1;
 
-	pre_version = cur_cp_version(cp_block);
+	*version = cur_cp_version(cp_block);
+	return 0;
+}
 
-	/* Read the 2nd cp block in this CP pack */
-	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
-	cp_page_2 = get_meta_page(sbi, cp_addr);
+static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
+				block_t cp_addr, unsigned long long *version)
+{
+	struct page *cp_page_1 = NULL, *cp_page_2 = NULL;
+	struct f2fs_checkpoint *cp_block = NULL;
+	unsigned long long cur_version = 0, pre_version = 0;
+	int ret = 0;
 
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_2);
-	crc_offset = le32_to_cpu(cp_block->checksum_offset);
-	if (crc_offset >= blk_size)
-		goto invalid_cp2;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_1, version);
+	if (ret == -1)
+		goto invalid_cp1;
+	pre_version = *version;
 
-	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
-	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
+	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_2, version);
+	if (ret == -1)
 		goto invalid_cp2;
-
-	cur_version = cur_cp_version(cp_block);
+	cur_version = *version;
 
 	if (cur_version == pre_version) {
 		*version = cur_version;
Signature · 2 lines
1.8.3.1
------------------------------------------------------------------------------
maintainer
#1 lore raw

Hi Tiezhu,

Quoted 32 lines · Tiezhu Yang

There exists almost same codes when get the value of pre_version and cur_version in function validate_checkpoint, this patch adds get_checkpoint_version to clean up redundant codes.

Signed-off-by Tiezhu Yang <[email protected]>
---
 fs/f2fs/checkpoint.c | 52 ++++++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 24 deletions(-)
fs/f2fs/checkpoint.c +4 -7
index a655d75..facede2 100644
@@ -666,45 +666,49 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
 	}
 }
 
-static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
-				block_t cp_addr, unsigned long long *version)
+static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
+			struct f2fs_checkpoint *cp_block, struct page *cp_page,
+			unsigned long long *version)
 {
-	struct page *cp_page_1, *cp_page_2 = NULL;
 	unsigned long blk_size = sbi->blocksize;
-	struct f2fs_checkpoint *cp_block;
-	unsigned long long cur_version = 0, pre_version = 0;
-	size_t crc_offset;
+	size_t crc_offset = 0;
 	__u32 crc = 0;
 
-	/* Read the 1st cp block in this CP pack */

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 682

Quoted 10 lines
fs/f2fs/checkpoint.c +3 -4
-	cp_page_1 = get_meta_page(sbi, cp_addr);
+	cp_page = get_meta_page(sbi, cp_addr);
+	cp_block = (struct f2fs_checkpoint *)page_address(cp_page);
 
-	/* get the version number */
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_1);
 	crc_offset = le32_to_cpu(cp_block->checksum_offset);
 	if (crc_offset >= blk_size)
-		goto invalid_cp1;
+		return -1;

How about using -EINVAL with a kernel message?

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 686

Quoted 5 lines
fs/f2fs/checkpoint.c +1 -1
 	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
 	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
-		goto invalid_cp1;
+		return -1;

ditto.

Quoted 5 lines
fs/f2fs/checkpoint.c +3 -1
-	pre_version = cur_cp_version(cp_block);
+	*version = cur_cp_version(cp_block);
+	return 0;
+}

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 698

Quoted 10 lines
fs/f2fs/checkpoint.c +7 -3
-	/* Read the 2nd cp block in this CP pack */
-	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
-	cp_page_2 = get_meta_page(sbi, cp_addr);
+static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
+				block_t cp_addr, unsigned long long *version)
+{
+	struct page *cp_page_1 = NULL, *cp_page_2 = NULL;
+	struct f2fs_checkpoint *cp_block = NULL;
+	unsigned long long cur_version = 0, pre_version = 0;
+	int ret = 0;

	int err;

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 702

Quoted 8 lines
fs/f2fs/checkpoint.c +3 -4
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_2);
-	crc_offset = le32_to_cpu(cp_block->checksum_offset);
-	if (crc_offset >= blk_size)
-		goto invalid_cp2;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_1, version);
+	if (ret == -1)

if (err)

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 709

Quoted 9 lines
fs/f2fs/checkpoint.c +6 -2
+		goto invalid_cp1;
+	pre_version = *version;
 
-	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
-	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
+	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_2, version);
+	if (ret == -1)

if (err)

Quoted 9 lines
fs/f2fs/checkpoint.c +1 -2
 		goto invalid_cp2;
-
-	cur_version = cur_cp_version(cp_block);
+	cur_version = *version;
 
 	if (cur_version == pre_version) {
 		*version = cur_version;
Signature · 1 line
1.8.3.1
author
#2 lore raw
Quoted 39 lines · Jaegeuk Kim

Hi Tiezhu,

Quoted 36 lines · Tiezhu Yang

There exists almost same codes when get the value of pre_version and cur_version in function validate_checkpoint, this patch adds get_checkpoint_version to clean up redundant codes.

Signed-off-by Tiezhu Yang <[email protected]>
---
 fs/f2fs/checkpoint.c | 52 ++++++++++++++++++++++++++++------------------------
 1 file changed, 28 insertions(+), 24 deletions(-)
fs/f2fs/checkpoint.c +6 -8
index a655d75..facede2 100644
@@ -666,45 +666,49 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
 	}
 }
 
-static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
-				block_t cp_addr, unsigned long long *version)
+static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
+			struct f2fs_checkpoint *cp_block, struct page *cp_page,
+			unsigned long long *version)
 {
-	struct page *cp_page_1, *cp_page_2 = NULL;
 	unsigned long blk_size = sbi->blocksize;
-	struct f2fs_checkpoint *cp_block;
-	unsigned long long cur_version = 0, pre_version = 0;
-	size_t crc_offset;
+	size_t crc_offset = 0;
 	__u32 crc = 0;
 
-	/* Read the 1st cp block in this CP pack */
-	cp_page_1 = get_meta_page(sbi, cp_addr);
+	cp_page = get_meta_page(sbi, cp_addr);
+	cp_block = (struct f2fs_checkpoint *)page_address(cp_page);
Quoted 8 lines

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 682

Quoted 6 lines
fs/f2fs/checkpoint.c +1 -3
-	/* get the version number */
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_1);
 	crc_offset = le32_to_cpu(cp_block->checksum_offset);
 	if (crc_offset >= blk_size)
-		goto invalid_cp1;
+		return -1;

How about using -EINVAL with a kernel message?

OK, thanks for your suggestion, I will send a v2 patch.

Thanks,

Quoted 55 lines

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 686

Quoted 6 lines
fs/f2fs/checkpoint.c +1 -1
 	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
 	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
-		goto invalid_cp1;
+		return -1;

ditto.

Quoted 5 lines
fs/f2fs/checkpoint.c +3 -1
-	pre_version = cur_cp_version(cp_block);
+	*version = cur_cp_version(cp_block);
+	return 0;
+}

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 698

Quoted 10 lines
fs/f2fs/checkpoint.c +7 -3
-	/* Read the 2nd cp block in this CP pack */
-	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
-	cp_page_2 = get_meta_page(sbi, cp_addr);
+static struct page *validate_checkpoint(struct f2fs_sb_info *sbi,
+				block_t cp_addr, unsigned long long *version)
+{
+	struct page *cp_page_1 = NULL, *cp_page_2 = NULL;
+	struct f2fs_checkpoint *cp_block = NULL;
+	unsigned long long cur_version = 0, pre_version = 0;
+	int ret = 0;

	int err;

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 702

Quoted 8 lines
fs/f2fs/checkpoint.c +3 -4
-	cp_block = (struct f2fs_checkpoint *)page_address(cp_page_2);
-	crc_offset = le32_to_cpu(cp_block->checksum_offset);
-	if (crc_offset >= blk_size)
-		goto invalid_cp2;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_1, version);
+	if (ret == -1)

if (err)

fs/f2fs/checkpoint.c static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk) · line 709

Quoted 9 lines
fs/f2fs/checkpoint.c +6 -2
+		goto invalid_cp1;
+	pre_version = *version;
 
-	crc = le32_to_cpu(*((__le32 *)((unsigned char *)cp_block + crc_offset)));
-	if (!f2fs_crc_valid(sbi, crc, cp_block, crc_offset))
+	cp_addr += le32_to_cpu(cp_block->cp_pack_total_block_count) - 1;
+	ret = get_checkpoint_version(sbi, cp_addr, cp_block,
+					cp_page_2, version);
+	if (ret == -1)

if (err)

Quoted 5 lines
fs/f2fs/checkpoint.c +1 -2
 		goto invalid_cp2;
-
-	cur_version = cur_cp_version(cp_block);
+	cur_version = *version;
Quoted 4 lines
Quoted 4 lines
fs/f2fs/checkpoint.c +0 -0
 	if (cur_version == pre_version) {
 		*version = cur_version;
Signature · 1 line
1.8.3.1

------------------------------------------------------------------------------