Skip to contentSkip to conversation

f2fs-tools: don't finalize device early

PATCH In review Jaegeuk Kim 2 participants 3 messages Oct 14, 2017 – Oct 16, 2017

lore
Share & tools
lore

Jaegeuk Kim

author
#0 lore raw

PATCH 1/1 f2fs-tools: don't finalize device early

We should close the device by main body of each tools.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fsck/mount.c | 3 ---
 1 file changed, 3 deletions(-)
fsck/mount.c +0 -3
index 7a28314..c78314e 100644
@@ -1941,9 +1941,6 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	if (flags & CP_NAT_BITS_FLAG)
 		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
 
-	/* in case of sudden power off */
-	f2fs_finalize_device();
-
 	/* write the last cp */
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);
Signature · 6 lines
2.14.0.rc1.383.gd1ce394fe2-goog


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
#1 lore raw
Quoted 12 lines · Jaegeuk Kim

We should close the device by main body of each tools.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fsck/mount.c | 3 ---
 1 file changed, 3 deletions(-)
fsck/mount.c +0 -0
index 7a28314..c78314e 100644

fsck/mount.c void write_checkpoint(struct f2fs_sb_info *sbi) · line 1945

Quoted 5 lines
fsck/mount.c +0 -2
@@ -1941,9 +1941,6 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	if (flags & CP_NAT_BITS_FLAG)
 		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
 
-	/* in case of sudden power off */
-	f2fs_finalize_device();

IIRC, Yunlei wants to fsync previous data in checkpoint area before write last CP pack, in order to avoid checkpoint corruption in sudden-power-off case.

Shouldn't we use fsync(main-device-fd) here instead?

Thanks,

fsck/mount.c void write_checkpoint(struct f2fs_sb_info *sbi) · line 1946

Quoted 5 lines
fsck/mount.c +0 -1
-
 	/* write the last cp */
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot

author
#2 lore raw
Quoted 16 lines · Chao Yu
Quoted 15 lines · Jaegeuk Kim

We should close the device by main body of each tools.

Signed-off-by Jaegeuk Kim <[email protected]>
---
 fsck/mount.c | 3 ---
 1 file changed, 3 deletions(-)
fsck/mount.c +0 -0
index 7a28314..c78314e 100644
@@ -1941,9 +1941,6 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	if (flags & CP_NAT_BITS_FLAG)
 		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
Quoted 7 lines

fsck/mount.c void write_checkpoint(struct f2fs_sb_info *sbi) · line 1945

Quoted 2 lines
fsck/mount.c +0 -2
-	/* in case of sudden power off */
-	f2fs_finalize_device();

IIRC, Yunlei wants to fsync previous data in checkpoint area before write last CP pack, in order to avoid checkpoint corruption in sudden-power-off case.

Shouldn't we use fsync(main-device-fd) here instead?

Yunlei,

If it's correct, could you write a patch introducing a generic function to call fsync() only? I should have reviewed the original patch tho.

Thanks,

Quoted 8 lines

Thanks,

Quoted 5 lines
fsck/mount.c +0 -1
-
 	/* write the last cp */
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot