linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] f2fs: check segment type before recover data
@ 2017-12-30  7:42 Yunlong Song
  2018-01-02  6:49 ` Chao Yu
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Yunlong Song @ 2017-12-30  7:42 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

In some case, the node blocks has wrong blkaddr whose segment type is
NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
the xattr range. Since fsck.f2fs does not check the recovery nodes, this
will cause __f2fs_replace_block change the curseg of node and do the
update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
result, when recovery process write checkpoint and sync nodes, the
next_blkoff of curseg is used in the segment bit map, then it will
cause f2fs_bug_on. So let's check the segment type before recover data,
and stop recover if it is not in DATA segment.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/recovery.c | 3 ++-
 fs/f2fs/segment.h  | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index 7d63faf..e8fee4a 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -478,7 +478,8 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
 		}
 
 		/* dest is valid block, try to recover from src to dest */
-		if (is_valid_blkaddr(sbi, dest, META_POR)) {
+		if (is_valid_blkaddr(sbi, dest, META_POR) &&
+			is_data_blkaddr(sbi, dest)) {
 
 			if (src == NULL_ADDR) {
 				err = reserve_new_block(&dn);
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 71a2aaa..5c5a215 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -115,6 +115,9 @@
 #define SECTOR_TO_BLOCK(sectors)					\
 	((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK)
 
+#define is_data_blkaddr(sbi, blkaddr)	\
+	(IS_DATASEG(get_seg_entry(sbi, GET_SEGNO(sbi, blkaddr))->type))
+
 /*
  * indicate a block allocation direction: RIGHT and LEFT.
  * RIGHT means allocating new sections towards the end of volume.
-- 
1.8.5.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] f2fs: check segment type before recover data
  2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
@ 2018-01-02  6:49 ` Chao Yu
  2018-01-02 11:02   ` Yunlong Song
  2018-01-03 20:04 ` Jaegeuk Kim
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2018-01-02  6:49 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, chao, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

On 2017/12/30 15:42, Yunlong Song wrote:
> In some case, the node blocks has wrong blkaddr whose segment type is

You mean *data block* has wrong blkaddr whose segment type is NODE?

> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
> will cause __f2fs_replace_block change the curseg of node and do the
> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a

Do you mean the root cause is that __f2fs_replace_block didn't update
next_blkoff?

> result, when recovery process write checkpoint and sync nodes, the
> next_blkoff of curseg is used in the segment bit map, then it will
> cause f2fs_bug_on. So let's check the segment type before recover data,
> and stop recover if it is not in DATA segment.

Sorry, I can't catch the whole cause and effect from you description, if
possible, could you give an example?

Thanks,

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/recovery.c | 3 ++-
>  fs/f2fs/segment.h  | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 7d63faf..e8fee4a 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -478,7 +478,8 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  		}
>  
>  		/* dest is valid block, try to recover from src to dest */
> -		if (is_valid_blkaddr(sbi, dest, META_POR)) {
> +		if (is_valid_blkaddr(sbi, dest, META_POR) &&
> +			is_data_blkaddr(sbi, dest)) {
>  
>  			if (src == NULL_ADDR) {
>  				err = reserve_new_block(&dn);
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 71a2aaa..5c5a215 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -115,6 +115,9 @@
>  #define SECTOR_TO_BLOCK(sectors)					\
>  	((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK)
>  
> +#define is_data_blkaddr(sbi, blkaddr)	\
> +	(IS_DATASEG(get_seg_entry(sbi, GET_SEGNO(sbi, blkaddr))->type))
> +
>  /*
>   * indicate a block allocation direction: RIGHT and LEFT.
>   * RIGHT means allocating new sections towards the end of volume.
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] f2fs: check segment type before recover data
  2018-01-02  6:49 ` Chao Yu
@ 2018-01-02 11:02   ` Yunlong Song
  2018-01-04  1:38     ` Chao Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Yunlong Song @ 2018-01-02 11:02 UTC (permalink / raw)
  To: Chao Yu, jaegeuk, chao, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel



On 2018/1/2 14:49, Chao Yu wrote:
> On 2017/12/30 15:42, Yunlong Song wrote:
>> In some case, the node blocks has wrong blkaddr whose segment type is
> You mean *data block* has wrong blkaddr whose segment type is NODE?
Yes.
>
>> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
>> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
>> will cause __f2fs_replace_block change the curseg of node and do the
>> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
> Do you mean the root cause is that __f2fs_replace_block didn't update
> next_blkoff?
No, it's not the root cause. The root cause may be something like DDR flip.
>
>> result, when recovery process write checkpoint and sync nodes, the
>> next_blkoff of curseg is used in the segment bit map, then it will
>> cause f2fs_bug_on. So let's check the segment type before recover data,
>> and stop recover if it is not in DATA segment.
> Sorry, I can't catch the whole cause and effect from you description, if
> possible, could you give an example?
For example, the i_inline flag has F2FS_INLINE_XATTR, and the last 50 
i_addrs have xattr
context. But if DDR flips, the i_inline flag may miss F2FS_INLINE_XATTR, 
and the last 50 i_addrs
are considered as data block addr. If the xattr context is 0x1234, and 
0x1234 happens to be
a valid block addr, and the block 0x1234 happens to be in a warm node 
segment. Then do_recover_data
will call f2fs_replace_block() with dest = 0x1234, which will change 
curseg of warm node to
0x1234's segment, and make update_sit_entry(sbi, 0x1234, 1), the 
curseg->next_blkoff also
points to 0x1234's offset in its segment. When recovery process calls 
write_checkpoint, sync
nodes will write to 0x1234's offset of curseg warm node. The 
update_sit_entry will check that
offset and find the bitmap is already set to 1 and then calls f2fs_bug_on.

>
> Thanks,
>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>   fs/f2fs/recovery.c | 3 ++-
>>   fs/f2fs/segment.h  | 3 +++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>> index 7d63faf..e8fee4a 100644
>> --- a/fs/f2fs/recovery.c
>> +++ b/fs/f2fs/recovery.c
>> @@ -478,7 +478,8 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>   		}
>>   
>>   		/* dest is valid block, try to recover from src to dest */
>> -		if (is_valid_blkaddr(sbi, dest, META_POR)) {
>> +		if (is_valid_blkaddr(sbi, dest, META_POR) &&
>> +			is_data_blkaddr(sbi, dest)) {
>>   
>>   			if (src == NULL_ADDR) {
>>   				err = reserve_new_block(&dn);
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>> index 71a2aaa..5c5a215 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -115,6 +115,9 @@
>>   #define SECTOR_TO_BLOCK(sectors)					\
>>   	((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK)
>>   
>> +#define is_data_blkaddr(sbi, blkaddr)	\
>> +	(IS_DATASEG(get_seg_entry(sbi, GET_SEGNO(sbi, blkaddr))->type))
>> +
>>   /*
>>    * indicate a block allocation direction: RIGHT and LEFT.
>>    * RIGHT means allocating new sections towards the end of volume.
>>
>
> .
>

-- 
Thanks,
Yunlong Song

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] f2fs: check segment type before recover data
  2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
  2018-01-02  6:49 ` Chao Yu
@ 2018-01-03 20:04 ` Jaegeuk Kim
  2018-01-04  3:48 ` [PATCH v2] f2fs: check segment type in __f2fs_replace_block Yunlong Song
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2018-01-03 20:04 UTC (permalink / raw)
  To: Yunlong Song
  Cc: chao, yuchao0, yunlong.song, miaoxie, bintian.wang, shengyong1,
	heyunlei, linux-fsdevel, linux-f2fs-devel, linux-kernel

On 12/30, Yunlong Song wrote:
> In some case, the node blocks has wrong blkaddr whose segment type is
> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
> will cause __f2fs_replace_block change the curseg of node and do the
> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
> result, when recovery process write checkpoint and sync nodes, the
> next_blkoff of curseg is used in the segment bit map, then it will
> cause f2fs_bug_on. So let's check the segment type before recover data,
> and stop recover if it is not in DATA segment.

Then, why not checking this in __f2fs_replace_block()? And, you can do this,
only if se->valid_blocks != 0 at this point.

> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/recovery.c | 3 ++-
>  fs/f2fs/segment.h  | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index 7d63faf..e8fee4a 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -478,7 +478,8 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>  		}
>  
>  		/* dest is valid block, try to recover from src to dest */
> -		if (is_valid_blkaddr(sbi, dest, META_POR)) {
> +		if (is_valid_blkaddr(sbi, dest, META_POR) &&
> +			is_data_blkaddr(sbi, dest)) {
>  
>  			if (src == NULL_ADDR) {
>  				err = reserve_new_block(&dn);
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 71a2aaa..5c5a215 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -115,6 +115,9 @@
>  #define SECTOR_TO_BLOCK(sectors)					\
>  	((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK)
>  
> +#define is_data_blkaddr(sbi, blkaddr)	\
> +	(IS_DATASEG(get_seg_entry(sbi, GET_SEGNO(sbi, blkaddr))->type))
> +
>  /*
>   * indicate a block allocation direction: RIGHT and LEFT.
>   * RIGHT means allocating new sections towards the end of volume.
> -- 
> 1.8.5.2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] f2fs: check segment type before recover data
  2018-01-02 11:02   ` Yunlong Song
@ 2018-01-04  1:38     ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2018-01-04  1:38 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, chao, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

On 2018/1/2 19:02, Yunlong Song wrote:
> 
> 
> On 2018/1/2 14:49, Chao Yu wrote:
>> On 2017/12/30 15:42, Yunlong Song wrote:
>>> In some case, the node blocks has wrong blkaddr whose segment type is
>> You mean *data block* has wrong blkaddr whose segment type is NODE?
> Yes.
>>
>>> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
>>> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
>>> will cause __f2fs_replace_block change the curseg of node and do the
>>> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
>> Do you mean the root cause is that __f2fs_replace_block didn't update
>> next_blkoff?
> No, it's not the root cause. The root cause may be something like DDR flip.
>>
>>> result, when recovery process write checkpoint and sync nodes, the
>>> next_blkoff of curseg is used in the segment bit map, then it will
>>> cause f2fs_bug_on. So let's check the segment type before recover data,
>>> and stop recover if it is not in DATA segment.
>> Sorry, I can't catch the whole cause and effect from you description, if
>> possible, could you give an example?
> For example, the i_inline flag has F2FS_INLINE_XATTR, and the last 50 
> i_addrs have xattr
> context. But if DDR flips, the i_inline flag may miss F2FS_INLINE_XATTR, 
> and the last 50 i_addrs
> are considered as data block addr. If the xattr context is 0x1234, and 
> 0x1234 happens to be
> a valid block addr, and the block 0x1234 happens to be in a warm node 
> segment. Then do_recover_data
> will call f2fs_replace_block() with dest = 0x1234, which will change 
> curseg of warm node to
> 0x1234's segment, and make update_sit_entry(sbi, 0x1234, 1), the 
> curseg->next_blkoff also
> points to 0x1234's offset in its segment. When recovery process calls 
> write_checkpoint, sync
> nodes will write to 0x1234's offset of curseg warm node. The 
> update_sit_entry will check that
> offset and find the bitmap is already set to 1 and then calls f2fs_bug_on.

Got you, thanks for the explanation. IMO, for better debug, what about
adding f2fs_bug_on here to detect both software and hardware defect in
upstream code? In production, I think you can use your original implementation.

Thanks,

> 
>>
>> Thanks,
>>
>>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>> ---
>>>   fs/f2fs/recovery.c | 3 ++-
>>>   fs/f2fs/segment.h  | 3 +++
>>>   2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index 7d63faf..e8fee4a 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -478,7 +478,8 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>>   		}
>>>   
>>>   		/* dest is valid block, try to recover from src to dest */
>>> -		if (is_valid_blkaddr(sbi, dest, META_POR)) {
>>> +		if (is_valid_blkaddr(sbi, dest, META_POR) &&
>>> +			is_data_blkaddr(sbi, dest)) {
>>>   
>>>   			if (src == NULL_ADDR) {
>>>   				err = reserve_new_block(&dn);
>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>> index 71a2aaa..5c5a215 100644
>>> --- a/fs/f2fs/segment.h
>>> +++ b/fs/f2fs/segment.h
>>> @@ -115,6 +115,9 @@
>>>   #define SECTOR_TO_BLOCK(sectors)					\
>>>   	((sectors) >> F2FS_LOG_SECTORS_PER_BLOCK)
>>>   
>>> +#define is_data_blkaddr(sbi, blkaddr)	\
>>> +	(IS_DATASEG(get_seg_entry(sbi, GET_SEGNO(sbi, blkaddr))->type))
>>> +
>>>   /*
>>>    * indicate a block allocation direction: RIGHT and LEFT.
>>>    * RIGHT means allocating new sections towards the end of volume.
>>>
>>
>> .
>>
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] f2fs: check segment type in __f2fs_replace_block
  2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
  2018-01-02  6:49 ` Chao Yu
  2018-01-03 20:04 ` Jaegeuk Kim
@ 2018-01-04  3:48 ` Yunlong Song
  2018-01-04  5:56   ` Chao Yu
  2018-01-04  6:24 ` [PATCH v3] " Yunlong Song
  2018-01-04  7:02 ` [PATCH v4] " Yunlong Song
  4 siblings, 1 reply; 12+ messages in thread
From: Yunlong Song @ 2018-01-04  3:48 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 890d483..e3bbabf 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2719,6 +2719,8 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 	se = get_seg_entry(sbi, segno);
 	type = se->type;
 
+	f2fs_bug_on(sbi, se->valid_blocks && IS_NODESEG(type));
+
 	down_write(&SM_I(sbi)->curseg_lock);
 
 	if (!recover_curseg) {
-- 
1.8.5.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] f2fs: check segment type in __f2fs_replace_block
  2018-01-04  3:48 ` [PATCH v2] f2fs: check segment type in __f2fs_replace_block Yunlong Song
@ 2018-01-04  5:56   ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2018-01-04  5:56 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, chao, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Please add simple description here to explain why we need to add this, even
though this change is very simple.

On 2018/1/4 11:48, Yunlong Song wrote:
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 890d483..e3bbabf 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2719,6 +2719,8 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  	se = get_seg_entry(sbi, segno);
>  	type = se->type;
>  
> +	f2fs_bug_on(sbi, se->valid_blocks && IS_NODESEG(type));
> +
>  	down_write(&SM_I(sbi)->curseg_lock);
>  
>  	if (!recover_curseg) {
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] f2fs: check segment type in __f2fs_replace_block
  2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
                   ` (2 preceding siblings ...)
  2018-01-04  3:48 ` [PATCH v2] f2fs: check segment type in __f2fs_replace_block Yunlong Song
@ 2018-01-04  6:24 ` Yunlong Song
  2018-01-04  6:54   ` Jaegeuk Kim
  2018-01-04  7:02 ` [PATCH v4] " Yunlong Song
  4 siblings, 1 reply; 12+ messages in thread
From: Yunlong Song @ 2018-01-04  6:24 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

In some case, the node blocks has wrong blkaddr whose segment type is
NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
the xattr range. Since fsck.f2fs does not check the recovery nodes, this
will cause __f2fs_replace_block change the curseg of node and do the
update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
result, when recovery process write checkpoint and sync nodes, the
next_blkoff of curseg is used in the segment bit map, then it will
cause f2fs_bug_on. So let's check segment type in __f2fs_replace_block.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 890d483..6c6d2dd 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2719,6 +2719,8 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 	se = get_seg_entry(sbi, segno);
 	type = se->type;
 
+	f2fs_bug_on(sbi, se->valid_blocks && !IS_DATASEG(type));
+
 	down_write(&SM_I(sbi)->curseg_lock);
 
 	if (!recover_curseg) {
-- 
1.8.5.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] f2fs: check segment type in __f2fs_replace_block
  2018-01-04  6:24 ` [PATCH v3] " Yunlong Song
@ 2018-01-04  6:54   ` Jaegeuk Kim
  0 siblings, 0 replies; 12+ messages in thread
From: Jaegeuk Kim @ 2018-01-04  6:54 UTC (permalink / raw)
  To: Yunlong Song
  Cc: chao, yuchao0, yunlong.song, miaoxie, bintian.wang, shengyong1,
	heyunlei, linux-fsdevel, linux-f2fs-devel, linux-kernel

On 01/04, Yunlong Song wrote:
> In some case, the node blocks has wrong blkaddr whose segment type is
> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
> will cause __f2fs_replace_block change the curseg of node and do the
> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
> result, when recovery process write checkpoint and sync nodes, the
> next_blkoff of curseg is used in the segment bit map, then it will
> cause f2fs_bug_on. So let's check segment type in __f2fs_replace_block.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 890d483..6c6d2dd 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2719,6 +2719,8 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  	se = get_seg_entry(sbi, segno);
>  	type = se->type;
>  
> +	f2fs_bug_on(sbi, se->valid_blocks && !IS_DATASEG(type));
> +

Need to cover se->valid_blocks by the below curseg_lock at least?

>  	down_write(&SM_I(sbi)->curseg_lock);
>  
>  	if (!recover_curseg) {
> -- 
> 1.8.5.2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4] f2fs: check segment type in __f2fs_replace_block
  2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
                   ` (3 preceding siblings ...)
  2018-01-04  6:24 ` [PATCH v3] " Yunlong Song
@ 2018-01-04  7:02 ` Yunlong Song
  2018-01-04  7:10   ` Jaegeuk Kim
  4 siblings, 1 reply; 12+ messages in thread
From: Yunlong Song @ 2018-01-04  7:02 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

In some case, the node blocks has wrong blkaddr whose segment type is
NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
the xattr range. Since fsck.f2fs does not check the recovery nodes, this
will cause __f2fs_replace_block change the curseg of node and do the
update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
result, when recovery process write checkpoint and sync nodes, the
next_blkoff of curseg is used in the segment bit map, then it will
cause f2fs_bug_on. So let's check segment type in __f2fs_replace_block.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
 fs/f2fs/segment.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 890d483..50575d5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2720,6 +2720,7 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 	type = se->type;
 
 	down_write(&SM_I(sbi)->curseg_lock);
+	f2fs_bug_on(sbi, se->valid_blocks && !IS_DATASEG(type));
 
 	if (!recover_curseg) {
 		/* for recovery flow */
-- 
1.8.5.2

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v4] f2fs: check segment type in __f2fs_replace_block
  2018-01-04  7:02 ` [PATCH v4] " Yunlong Song
@ 2018-01-04  7:10   ` Jaegeuk Kim
  2018-01-04  7:22     ` Chao Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Jaegeuk Kim @ 2018-01-04  7:10 UTC (permalink / raw)
  To: Yunlong Song
  Cc: chao, yuchao0, yunlong.song, miaoxie, bintian.wang, shengyong1,
	heyunlei, linux-fsdevel, linux-f2fs-devel, linux-kernel

On 01/04, Yunlong Song wrote:
> In some case, the node blocks has wrong blkaddr whose segment type is
> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
> will cause __f2fs_replace_block change the curseg of node and do the
> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
> result, when recovery process write checkpoint and sync nodes, the
> next_blkoff of curseg is used in the segment bit map, then it will
> cause f2fs_bug_on. So let's check segment type in __f2fs_replace_block.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 890d483..50575d5 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2720,6 +2720,7 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  	type = se->type;
>  
>  	down_write(&SM_I(sbi)->curseg_lock);
> +	f2fs_bug_on(sbi, se->valid_blocks && !IS_DATASEG(type));

Let me just move this below like this and start some tests.

...

+       f2fs_bug_on(sbi, !IS_DATASEG(type));
        curseg = CURSEG_I(sbi, type);

>  
>  	if (!recover_curseg) {
>  		/* for recovery flow */
> -- 
> 1.8.5.2

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4] f2fs: check segment type in __f2fs_replace_block
  2018-01-04  7:10   ` Jaegeuk Kim
@ 2018-01-04  7:22     ` Chao Yu
  0 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2018-01-04  7:22 UTC (permalink / raw)
  To: Jaegeuk Kim, Yunlong Song
  Cc: chao, yunlong.song, miaoxie, bintian.wang, shengyong1, heyunlei,
	linux-fsdevel, linux-f2fs-devel, linux-kernel

On 2018/1/4 15:10, Jaegeuk Kim wrote:
> On 01/04, Yunlong Song wrote:
>> In some case, the node blocks has wrong blkaddr whose segment type is
>> NODE, e.g., recover inode has missing xattr flag and the blkaddr is in
>> the xattr range. Since fsck.f2fs does not check the recovery nodes, this
>> will cause __f2fs_replace_block change the curseg of node and do the
>> update_sit_entry(sbi, new_blkaddr, 1) with no next_blkoff refresh, as a
>> result, when recovery process write checkpoint and sync nodes, the
>> next_blkoff of curseg is used in the segment bit map, then it will
>> cause f2fs_bug_on. So let's check segment type in __f2fs_replace_block.
>>
>> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>> ---
>>  fs/f2fs/segment.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 890d483..50575d5 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -2720,6 +2720,7 @@ void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>  	type = se->type;
>>  
>>  	down_write(&SM_I(sbi)->curseg_lock);
>> +	f2fs_bug_on(sbi, se->valid_blocks && !IS_DATASEG(type));
> 
> Let me just move this below like this and start some tests.
> 
> ...
> 
> +       f2fs_bug_on(sbi, !IS_DATASEG(type));

Better,

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

>         curseg = CURSEG_I(sbi, type);
> 
>>  
>>  	if (!recover_curseg) {
>>  		/* for recovery flow */
>> -- 
>> 1.8.5.2
> 
> .
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-01-04  7:22 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-30  7:42 [PATCH] f2fs: check segment type before recover data Yunlong Song
2018-01-02  6:49 ` Chao Yu
2018-01-02 11:02   ` Yunlong Song
2018-01-04  1:38     ` Chao Yu
2018-01-03 20:04 ` Jaegeuk Kim
2018-01-04  3:48 ` [PATCH v2] f2fs: check segment type in __f2fs_replace_block Yunlong Song
2018-01-04  5:56   ` Chao Yu
2018-01-04  6:24 ` [PATCH v3] " Yunlong Song
2018-01-04  6:54   ` Jaegeuk Kim
2018-01-04  7:02 ` [PATCH v4] " Yunlong Song
2018-01-04  7:10   ` Jaegeuk Kim
2018-01-04  7:22     ` Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).