All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] fsck.f2fs: write checkpoint out of place first
@ 2018-07-24 15:28 Weichao Guo
  2018-07-26 11:52 ` Chao Yu
  0 siblings, 1 reply; 3+ messages in thread
From: Weichao Guo @ 2018-07-24 15:28 UTC (permalink / raw)
  To: jaegeuk, chao; +Cc: miaoxie, linux-f2fs-devel

We may encounter both checkpoints invalid in such a case:
1. write checkpoint A, B, C;
2. sudden power-cut during write checkpoint D;
3. fsck changes the total block count of checkpoint C;
4. sudden power-cut during fsck write checkpoint C in place

 ---------           ---------
|  ver C  |         |  ver D  |
|   ...   |         |   ...   |
| content |         | content |
|   ...   |         |   ...   |
|  ver C  |         |         |
|  ver A  |         |  ver B  |
 ---------           ---------

As the total # of checkpoint C is changed, an old cp block
like ver A or an invalid cp block may be referenced.
To avoid both checkpoints invalid, and considering fsck should
not update the checkpoint version, fsck could write checkpoint
out of place first and then write checkpoint in place. This
makes sure the file system is fixed by fsck and at least one
of the two checkpoints is valid.

Signed-off-by: Weichao Guo <guoweichao@huawei.com>
---
 fsck/fsck.c  | 13 ++++++++++++-
 fsck/mount.c | 13 ++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fsck/fsck.c b/fsck/fsck.c
index 91c8529..28320d5 100644
--- a/fsck/fsck.c
+++ b/fsck/fsck.c
@@ -1954,6 +1954,7 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
 	u32 i;
 	int ret;
 	u_int32_t crc = 0;
+	int phase = 0;
 
 	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
 		orphan_blks = __start_sum_addr(sbi) - 1;
@@ -1975,9 +1976,11 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
 	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
 
 	cp_blk_no = get_sb(cp_blkaddr);
-	if (sbi->cur_cp == 2)
+	/* write checkpoint with current version out of place first */
+	if (sbi->cur_cp == 1)
 		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
 
+next_step:
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);
 
@@ -2002,6 +2005,14 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
 	/* Write nat bits */
 	if (flags & CP_NAT_BITS_FLAG)
 		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
+
+	if (phase == 0) {
+		cp_blk_no = get_sb(cp_blkaddr);
+		if (sbi->cur_cp == 2)
+			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
+		phase++;
+		goto next_step;
+	}
 }
 
 int check_curseg_offset(struct f2fs_sb_info *sbi)
diff --git a/fsck/mount.c b/fsck/mount.c
index e5574c5..8d7a9bb 100644
--- a/fsck/mount.c
+++ b/fsck/mount.c
@@ -2088,6 +2088,7 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	u32 flags = CP_UMOUNT_FLAG;
 	int i, ret;
 	u_int32_t crc = 0;
+	int phase = 0;
 
 	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
 		orphan_blks = __start_sum_addr(sbi) - 1;
@@ -2105,9 +2106,11 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
 
 	cp_blk_no = get_sb(cp_blkaddr);
-	if (sbi->cur_cp == 2)
+	/* write checkpoint with current version out of place first */
+	if (sbi->cur_cp == 1)
 		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
 
+next_step:
 	/* write the first cp */
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);
@@ -2142,6 +2145,14 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
 	/* write the last cp */
 	ret = dev_write_block(cp, cp_blk_no++);
 	ASSERT(ret >= 0);
+
+	if (phase == 0) {
+		cp_blk_no = get_sb(cp_blkaddr);
+		if (sbi->cur_cp == 2)
+			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
+		phase++;
+		goto next_step;
+	}
 }
 
 void build_nat_area_bitmap(struct f2fs_sb_info *sbi)
-- 
2.10.1


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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH RFC] fsck.f2fs: write checkpoint out of place first
  2018-07-24 15:28 [PATCH RFC] fsck.f2fs: write checkpoint out of place first Weichao Guo
@ 2018-07-26 11:52 ` Chao Yu
  2018-07-26 12:49   ` guoweichao
  0 siblings, 1 reply; 3+ messages in thread
From: Chao Yu @ 2018-07-26 11:52 UTC (permalink / raw)
  To: Weichao Guo, jaegeuk, chao; +Cc: miaoxie, linux-f2fs-devel

On 2018/7/24 23:28, Weichao Guo wrote:
> We may encounter both checkpoints invalid in such a case:
> 1. write checkpoint A, B, C;
> 2. sudden power-cut during write checkpoint D;
> 3. fsck changes the total block count of checkpoint C;
> 4. sudden power-cut during fsck write checkpoint C in place
> 
>  ---------           ---------
> |  ver C  |         |  ver D  |
> |   ...   |         |   ...   |
> | content |         | content |
> |   ...   |         |   ...   |
> |  ver C  |         |         |
> |  ver A  |         |  ver B  |
>  ---------           ---------
> 
> As the total # of checkpoint C is changed, an old cp block
> like ver A or an invalid cp block may be referenced.
> To avoid both checkpoints invalid, and considering fsck should
> not update the checkpoint version, fsck could write checkpoint
> out of place first and then write checkpoint in place. This
> makes sure the file system is fixed by fsck and at least one
> of the two checkpoints is valid.
> 
> Signed-off-by: Weichao Guo <guoweichao@huawei.com>
> ---
>  fsck/fsck.c  | 13 ++++++++++++-
>  fsck/mount.c | 13 ++++++++++++-
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/fsck/fsck.c b/fsck/fsck.c
> index 91c8529..28320d5 100644
> --- a/fsck/fsck.c
> +++ b/fsck/fsck.c
> @@ -1954,6 +1954,7 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>  	u32 i;
>  	int ret;
>  	u_int32_t crc = 0;
> +	int phase = 0;
>  
>  	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
>  		orphan_blks = __start_sum_addr(sbi) - 1;
> @@ -1975,9 +1976,11 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>  	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
>  
>  	cp_blk_no = get_sb(cp_blkaddr);
> -	if (sbi->cur_cp == 2)
> +	/* write checkpoint with current version out of place first */
> +	if (sbi->cur_cp == 1)
>  		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>  
> +next_step:
>  	ret = dev_write_block(cp, cp_blk_no++);
>  	ASSERT(ret >= 0);
>  
> @@ -2002,6 +2005,14 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>  	/* Write nat bits */
>  	if (flags & CP_NAT_BITS_FLAG)
>  		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
> +
> +	if (phase == 0) {
> +		cp_blk_no = get_sb(cp_blkaddr);
> +		if (sbi->cur_cp == 2)
> +			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
> +		phase++;
> +		goto next_step;

Before goes to second round, we need to keep all cached data being persistent,
so additional fsync() is needed?

Since we are trying to implement OPU style checkpoint() in f2fs-tools, can you
check whether all caller of {fix,write}_checkpoint() need such logic?

Thanks,

> +	}
>  }
>  
>  int check_curseg_offset(struct f2fs_sb_info *sbi)
> diff --git a/fsck/mount.c b/fsck/mount.c
> index e5574c5..8d7a9bb 100644
> --- a/fsck/mount.c
> +++ b/fsck/mount.c
> @@ -2088,6 +2088,7 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>  	u32 flags = CP_UMOUNT_FLAG;
>  	int i, ret;
>  	u_int32_t crc = 0;
> +	int phase = 0;
>  
>  	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
>  		orphan_blks = __start_sum_addr(sbi) - 1;
> @@ -2105,9 +2106,11 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>  	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
>  
>  	cp_blk_no = get_sb(cp_blkaddr);
> -	if (sbi->cur_cp == 2)
> +	/* write checkpoint with current version out of place first */
> +	if (sbi->cur_cp == 1)
>  		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>  
> +next_step:
>  	/* write the first cp */
>  	ret = dev_write_block(cp, cp_blk_no++);
>  	ASSERT(ret >= 0);
> @@ -2142,6 +2145,14 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>  	/* write the last cp */
>  	ret = dev_write_block(cp, cp_blk_no++);
>  	ASSERT(ret >= 0);
> +
> +	if (phase == 0) {
> +		cp_blk_no = get_sb(cp_blkaddr);
> +		if (sbi->cur_cp == 2)
> +			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
> +		phase++;
> +		goto next_step;
> +	}
>  }
>  
>  void build_nat_area_bitmap(struct f2fs_sb_info *sbi)
> 


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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH RFC] fsck.f2fs: write checkpoint out of place first
  2018-07-26 11:52 ` Chao Yu
@ 2018-07-26 12:49   ` guoweichao
  0 siblings, 0 replies; 3+ messages in thread
From: guoweichao @ 2018-07-26 12:49 UTC (permalink / raw)
  To: Chao Yu, jaegeuk, chao; +Cc: miaoxie, linux-f2fs-devel



On 2018/7/26 19:52, Chao Yu wrote:
> On 2018/7/24 23:28, Weichao Guo wrote:
>> We may encounter both checkpoints invalid in such a case:
>> 1. write checkpoint A, B, C;
>> 2. sudden power-cut during write checkpoint D;
>> 3. fsck changes the total block count of checkpoint C;
>> 4. sudden power-cut during fsck write checkpoint C in place
>>
>>  ---------           ---------
>> |  ver C  |         |  ver D  |
>> |   ...   |         |   ...   |
>> | content |         | content |
>> |   ...   |         |   ...   |
>> |  ver C  |         |         |
>> |  ver A  |         |  ver B  |
>>  ---------           ---------
>>
>> As the total # of checkpoint C is changed, an old cp block
>> like ver A or an invalid cp block may be referenced.
>> To avoid both checkpoints invalid, and considering fsck should
>> not update the checkpoint version, fsck could write checkpoint
>> out of place first and then write checkpoint in place. This
>> makes sure the file system is fixed by fsck and at least one
>> of the two checkpoints is valid.
>>
>> Signed-off-by: Weichao Guo <guoweichao@huawei.com>
>> ---
>>  fsck/fsck.c  | 13 ++++++++++++-
>>  fsck/mount.c | 13 ++++++++++++-
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/fsck/fsck.c b/fsck/fsck.c
>> index 91c8529..28320d5 100644
>> --- a/fsck/fsck.c
>> +++ b/fsck/fsck.c
>> @@ -1954,6 +1954,7 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>>  	u32 i;
>>  	int ret;
>>  	u_int32_t crc = 0;
>> +	int phase = 0;
>>  
>>  	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
>>  		orphan_blks = __start_sum_addr(sbi) - 1;
>> @@ -1975,9 +1976,11 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>>  	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
>>  
>>  	cp_blk_no = get_sb(cp_blkaddr);
>> -	if (sbi->cur_cp == 2)
>> +	/* write checkpoint with current version out of place first */
>> +	if (sbi->cur_cp == 1)
>>  		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>>  
>> +next_step:
>>  	ret = dev_write_block(cp, cp_blk_no++);
>>  	ASSERT(ret >= 0);
>>  
>> @@ -2002,6 +2005,14 @@ static void fix_checkpoint(struct f2fs_sb_info *sbi)
>>  	/* Write nat bits */
>>  	if (flags & CP_NAT_BITS_FLAG)
>>  		write_nat_bits(sbi, sb, cp, sbi->cur_cp);
>> +
>> +	if (phase == 0) {
>> +		cp_blk_no = get_sb(cp_blkaddr);
>> +		if (sbi->cur_cp == 2)
>> +			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>> +		phase++;
>> +		goto next_step;
> 
> Before goes to second round, we need to keep all cached data being persistent,
> so additional fsync() is needed?
Yes.
> 
> Since we are trying to implement OPU style checkpoint() in f2fs-tools, can you
> check whether all caller of {fix,write}_checkpoint() need such logic?
OK, I will resend a revised version of this patch.

Thanks,

> 
> Thanks,
> 
>> +	}
>>  }
>>  
>>  int check_curseg_offset(struct f2fs_sb_info *sbi)
>> diff --git a/fsck/mount.c b/fsck/mount.c
>> index e5574c5..8d7a9bb 100644
>> --- a/fsck/mount.c
>> +++ b/fsck/mount.c
>> @@ -2088,6 +2088,7 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>>  	u32 flags = CP_UMOUNT_FLAG;
>>  	int i, ret;
>>  	u_int32_t crc = 0;
>> +	int phase = 0;
>>  
>>  	if (is_set_ckpt_flags(cp, CP_ORPHAN_PRESENT_FLAG)) {
>>  		orphan_blks = __start_sum_addr(sbi) - 1;
>> @@ -2105,9 +2106,11 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>>  	*((__le32 *)((unsigned char *)cp + CHECKSUM_OFFSET)) = cpu_to_le32(crc);
>>  
>>  	cp_blk_no = get_sb(cp_blkaddr);
>> -	if (sbi->cur_cp == 2)
>> +	/* write checkpoint with current version out of place first */
>> +	if (sbi->cur_cp == 1)
>>  		cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>>  
>> +next_step:
>>  	/* write the first cp */
>>  	ret = dev_write_block(cp, cp_blk_no++);
>>  	ASSERT(ret >= 0);
>> @@ -2142,6 +2145,14 @@ void write_checkpoint(struct f2fs_sb_info *sbi)
>>  	/* write the last cp */
>>  	ret = dev_write_block(cp, cp_blk_no++);
>>  	ASSERT(ret >= 0);
>> +
>> +	if (phase == 0) {
>> +		cp_blk_no = get_sb(cp_blkaddr);
>> +		if (sbi->cur_cp == 2)
>> +			cp_blk_no += 1 << get_sb(log_blocks_per_seg);
>> +		phase++;
>> +		goto next_step;
>> +	}
>>  }
>>  
>>  void build_nat_area_bitmap(struct f2fs_sb_info *sbi)
>>
> 
> 
> .
> 


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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-07-26 12:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-24 15:28 [PATCH RFC] fsck.f2fs: write checkpoint out of place first Weichao Guo
2018-07-26 11:52 ` Chao Yu
2018-07-26 12:49   ` guoweichao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.