All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
@ 2017-08-01  7:56 ` Yunlong Song
  0 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-01  7:56 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, sylinux, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
cur_valid_map_mir update is skipped unlikely, so fix it.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 151968e..6f7731a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_bug_on(sbi, 1);
 #endif
 		}
+#ifdef CONFIG_F2FS_CHECK_FS
+		else
+			f2fs_set_bit(offset, se->cur_valid_map_mir);
+#endif
 		if (f2fs_discard_en(sbi) &&
 			!f2fs_test_and_set_bit(offset, se->discard_map))
 			sbi->discard_blks--;
@@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_bug_on(sbi, 1);
 #endif
 		}
+#ifdef CONFIG_F2FS_CHECK_FS
+		else
+			f2fs_clear_bit(offset, se->cur_valid_map_mir);
+#endif
 		if (f2fs_discard_en(sbi) &&
 			f2fs_test_and_clear_bit(offset, se->discard_map))
 			sbi->discard_blks++;
-- 
1.8.5.2

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

* [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
@ 2017-08-01  7:56 ` Yunlong Song
  0 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-01  7:56 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, sylinux, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
cur_valid_map_mir update is skipped unlikely, so fix it.

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 151968e..6f7731a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_bug_on(sbi, 1);
 #endif
 		}
+#ifdef CONFIG_F2FS_CHECK_FS
+		else
+			f2fs_set_bit(offset, se->cur_valid_map_mir);
+#endif
 		if (f2fs_discard_en(sbi) &&
 			!f2fs_test_and_set_bit(offset, se->discard_map))
 			sbi->discard_blks--;
@@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_bug_on(sbi, 1);
 #endif
 		}
+#ifdef CONFIG_F2FS_CHECK_FS
+		else
+			f2fs_clear_bit(offset, se->cur_valid_map_mir);
+#endif
 		if (f2fs_discard_en(sbi) &&
 			f2fs_test_and_clear_bit(offset, se->discard_map))
 			sbi->discard_blks++;
-- 
1.8.5.2

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

* Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
  2017-08-01  7:56 ` Yunlong Song
  (?)
@ 2017-08-01 15:44 ` Chao Yu
       [not found]   ` <c1935af.77af.15d9ebd35e3.Coremail.sylinux@163.com>
  -1 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2017-08-01 15:44 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, yuchao0, sylinux
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

Hi Yunlong,

How about checking consistence in between original and mirror bitmap all
the time as below?

---
 fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index a26c24dae70c..f32a19cf486a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 	struct seg_entry *se;
 	unsigned int segno, offset;
 	long int new_vblocks;
+	bool exist, mir_exist;

 	segno = GET_SEGNO(sbi, blkaddr);

@@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)

 	/* Update valid block bitmap */
 	if (del > 0) {
-		if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (f2fs_test_and_set_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_set_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when setting bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was set, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			!f2fs_test_and_set_bit(offset, se->discard_map))
 			sbi->discard_blks--;
@@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 				se->ckpt_valid_blocks++;
 		}
 	} else {
-		if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (!f2fs_test_and_clear_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_clear_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when clearing bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (!exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was cleared, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			f2fs_test_and_clear_bit(offset, se->discard_map))
 			sbi->discard_blks++;
-- 
2.13.0.90.g1eb437020

On 2017/8/1 15:56, Yunlong Song wrote:
> When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
> cur_valid_map_mir update is skipped unlikely, so fix it.
> 
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
> ---
>  fs/f2fs/segment.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 151968e..6f7731a 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  			f2fs_bug_on(sbi, 1);
>  #endif
>  		}
> +#ifdef CONFIG_F2FS_CHECK_FS
> +		else
> +			f2fs_set_bit(offset, se->cur_valid_map_mir);
> +#endif
>  		if (f2fs_discard_en(sbi) &&
>  			!f2fs_test_and_set_bit(offset, se->discard_map))
>  			sbi->discard_blks--;
> @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  			f2fs_bug_on(sbi, 1);
>  #endif
>  		}
> +#ifdef CONFIG_F2FS_CHECK_FS
> +		else
> +			f2fs_clear_bit(offset, se->cur_valid_map_mir);
> +#endif
>  		if (f2fs_discard_en(sbi) &&
>  			f2fs_test_and_clear_bit(offset, se->discard_map))
>  			sbi->discard_blks++;
> 

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

* Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
       [not found]   ` <c1935af.77af.15d9ebd35e3.Coremail.sylinux@163.com>
@ 2017-08-02  1:34     ` Chao Yu
  2017-08-02  8:02       ` Yunlong Song
  0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2017-08-02  1:34 UTC (permalink / raw)
  To: Yunlong Song, Chao Yu
  Cc: Yunlong Song, jaegeuk, miaoxie, bintian.wang, linux-fsdevel,
	linux-f2fs-devel, linux-kernel

Hi Yunlong,

On 2017/8/2 0:59, Yunlong Song wrote:
> Hi Chao,
>     I think there is no need to test mirror bitmap when original bitmap's check
> get passed, it is an instruction waste for "test". By the way, previous patch
> uses WARN to skip trigger panic when the memory is flipped, I think it is proper
> to not trigger panic in this situation, because it is not code bug at all in
> such case.

Original idea is trying to use bitmap mirror to detect bitmap corruption caused
by memory overflow or bit-transition of cache.

So if we encounter inconsistency in between original bitmap and mirror bitmap,
there may be memory overflow or cache bit-transition, we need to detect that;
another case is that both bitmap is consistent, but we are trying to reuse valid
block address or free invalid block address, indeed that is a bug.

Below modification can cover above two cases, which make original idea of
introducing mirror bitmap check working correctly.

Thanks,

> On 08/01/2017 23:44, Chao Yu <mailto:chao@kernel.org> wrote:
> 
>     Hi Yunlong,
> 
>     How about checking consistence in between original and mirror bitmap all
>     the time as below?
> 
>     ---
>     fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
>     1 file changed, 27 insertions(+), 14 deletions(-)
> 
>     diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>     index a26c24dae70c..f32a19cf486a 100644
>     --- a/fs/f2fs/segment.c
>     +++ b/fs/f2fs/segment.c
>     @@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi,
>     block_t blkaddr, int del)
>        struct seg_entry *se;
>        unsigned int segno, offset;
>        long int new_vblocks;
>     +    bool exist, mir_exist;
> 
>        segno = GET_SEGNO(sbi, blkaddr);
> 
>     @@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info
>     *sbi, block_t blkaddr, int del)
> 
>        /* Update valid block bitmap */
>        if (del > 0) {
>     -        if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
>     +        exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
>     #ifdef CONFIG_F2FS_CHECK_FS
>     -            if (f2fs_test_and_set_bit(offset,
>     -                        se->cur_valid_map_mir))
>     -                f2fs_bug_on(sbi, 1);
>     -            else
>     -                WARN_ON(1);
>     -#else
>     +        mir_exist = f2fs_test_and_set_bit(offset,
>     +                        se->cur_valid_map_mir);
>     +        if (exist != mir_exist) {
>     +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>     +                "when setting bitmap, blk:%u, old bit:%d",
>     +                blkaddr, exist);
>                f2fs_bug_on(sbi, 1);
>     +        }
>     #endif
>     +        if (exist) {
>     +            f2fs_msg(sbi->sb, KERN_ERR,
>     +                "Bitmap was set, blk:%u", blkaddr);
>     +            f2fs_bug_on(sbi, 1);
>            }
>     +
>            if (f2fs_discard_en(sbi) &&
>                !f2fs_test_and_set_bit(offset, se->discard_map))
>                sbi->discard_blks--;
>     @@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info
>     *sbi, block_t blkaddr, int del)
>                    se->ckpt_valid_blocks++;
>            }
>        } else {
>     -        if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
>     +        exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
>     #ifdef CONFIG_F2FS_CHECK_FS
>     -            if (!f2fs_test_and_clear_bit(offset,
>     -                        se->cur_valid_map_mir))
>     -                f2fs_bug_on(sbi, 1);
>     -            else
>     -                WARN_ON(1);
>     -#else
>     +        mir_exist = f2fs_test_and_clear_bit(offset,
>     +                        se->cur_valid_map_mir);
>     +        if (exist != mir_exist) {
>     +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>     +                "when clearing bitmap, blk:%u, old bit:%d",
>     +                blkaddr, exist);
>                f2fs_bug_on(sbi, 1);
>     +        }
>     #endif
>     +        if (!exist) {
>     +            f2fs_msg(sbi->sb, KERN_ERR,
>     +                "Bitmap was cleared, blk:%u", blkaddr);
>     +            f2fs_bug_on(sbi, 1);
>            }
>     +
>            if (f2fs_discard_en(sbi) &&
>                f2fs_test_and_clear_bit(offset, se->discard_map))
>                sbi->discard_blks++;
>     -- 
>     2.13.0.90.g1eb437020
> 
>     On 2017/8/1 15:56, Yunlong Song wrote:
>     > When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
>     > cur_valid_map_mir update is skipped unlikely, so fix it.
>     >
>     > Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>     > ---
>     >  fs/f2fs/segment.c | 8 ++++++++
>     >  1 file changed, 8 insertions(+)
>     >
>     > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>     > index 151968e..6f7731a 100644
>     > --- a/fs/f2fs/segment.c
>     > +++ b/fs/f2fs/segment.c
>     > @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info
>     *sbi, block_t blkaddr, int del)
>     >              f2fs_bug_on(sbi, 1);
>     >  #endif
>     >          }
>     > +#ifdef CONFIG_F2FS_CHECK_FS
>     > +        else
>     > +            f2fs_set_bit(offset, se->cur_valid_map_mir);
>     > +#endif
>     >          if (f2fs_discard_en(sbi) &&
>     >              !f2fs_test_and_set_bit(offset, se->discard_map))
>     >              sbi->discard_blks--;
>     > @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info
>     *sbi, block_t blkaddr, int del)
>     >              f2fs_bug_on(sbi, 1);
>     >  #endif
>     >          }
>     > +#ifdef CONFIG_F2FS_CHECK_FS
>     > +        else
>     > +            f2fs_clear_bit(offset, se->cur_valid_map_mir);
>     > +#endif
>     >          if (f2fs_discard_en(sbi) &&
>     >              f2fs_test_and_clear_bit(offset, se->discard_map))
>     >              sbi->discard_blks++;
>     >
> 

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

* Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
  2017-08-02  1:34     ` Chao Yu
@ 2017-08-02  8:02       ` Yunlong Song
  2017-08-02  8:47         ` Chao Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Yunlong Song @ 2017-08-02  8:02 UTC (permalink / raw)
  To: Chao Yu, Yunlong Song, Chao Yu
  Cc: jaegeuk, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel,
	linux-kernel

Hi Chao,
     For the memory overflow or cache bit-transition case, when CHECK_FS 
is on, should f2fs
enter panic or just provide WARNing information? I prefer WARNing, 
because it is not f2fs's
fault, f2fs does not need to pay for it, why not use WARN_ON instead?

On 2017/8/2 9:34, Chao Yu wrote:
> Hi Yunlong,
>
> On 2017/8/2 0:59, Yunlong Song wrote:
>> Hi Chao,
>>      I think there is no need to test mirror bitmap when original bitmap's check
>> get passed, it is an instruction waste for "test". By the way, previous patch
>> uses WARN to skip trigger panic when the memory is flipped, I think it is proper
>> to not trigger panic in this situation, because it is not code bug at all in
>> such case.
> Original idea is trying to use bitmap mirror to detect bitmap corruption caused
> by memory overflow or bit-transition of cache.
>
> So if we encounter inconsistency in between original bitmap and mirror bitmap,
> there may be memory overflow or cache bit-transition, we need to detect that;
> another case is that both bitmap is consistent, but we are trying to reuse valid
> block address or free invalid block address, indeed that is a bug.
>
> Below modification can cover above two cases, which make original idea of
> introducing mirror bitmap check working correctly.
>
> Thanks,
>
>> On 08/01/2017 23:44, Chao Yu <mailto:chao@kernel.org> wrote:
>>
>>      Hi Yunlong,
>>
>>      How about checking consistence in between original and mirror bitmap all
>>      the time as below?
>>
>>      ---
>>      fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
>>      1 file changed, 27 insertions(+), 14 deletions(-)
>>
>>      diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>      index a26c24dae70c..f32a19cf486a 100644
>>      --- a/fs/f2fs/segment.c
>>      +++ b/fs/f2fs/segment.c
>>      @@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi,
>>      block_t blkaddr, int del)
>>         struct seg_entry *se;
>>         unsigned int segno, offset;
>>         long int new_vblocks;
>>      +    bool exist, mir_exist;
>>
>>         segno = GET_SEGNO(sbi, blkaddr);
>>
>>      @@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>      *sbi, block_t blkaddr, int del)
>>
>>         /* Update valid block bitmap */
>>         if (del > 0) {
>>      -        if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
>>      +        exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
>>      #ifdef CONFIG_F2FS_CHECK_FS
>>      -            if (f2fs_test_and_set_bit(offset,
>>      -                        se->cur_valid_map_mir))
>>      -                f2fs_bug_on(sbi, 1);
>>      -            else
>>      -                WARN_ON(1);
>>      -#else
>>      +        mir_exist = f2fs_test_and_set_bit(offset,
>>      +                        se->cur_valid_map_mir);
>>      +        if (exist != mir_exist) {
>>      +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>      +                "when setting bitmap, blk:%u, old bit:%d",
>>      +                blkaddr, exist);
>>                 f2fs_bug_on(sbi, 1);
>>      +        }
>>      #endif
>>      +        if (exist) {
>>      +            f2fs_msg(sbi->sb, KERN_ERR,
>>      +                "Bitmap was set, blk:%u", blkaddr);
>>      +            f2fs_bug_on(sbi, 1);
>>             }
>>      +
>>             if (f2fs_discard_en(sbi) &&
>>                 !f2fs_test_and_set_bit(offset, se->discard_map))
>>                 sbi->discard_blks--;
>>      @@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>      *sbi, block_t blkaddr, int del)
>>                     se->ckpt_valid_blocks++;
>>             }
>>         } else {
>>      -        if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
>>      +        exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
>>      #ifdef CONFIG_F2FS_CHECK_FS
>>      -            if (!f2fs_test_and_clear_bit(offset,
>>      -                        se->cur_valid_map_mir))
>>      -                f2fs_bug_on(sbi, 1);
>>      -            else
>>      -                WARN_ON(1);
>>      -#else
>>      +        mir_exist = f2fs_test_and_clear_bit(offset,
>>      +                        se->cur_valid_map_mir);
>>      +        if (exist != mir_exist) {
>>      +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>      +                "when clearing bitmap, blk:%u, old bit:%d",
>>      +                blkaddr, exist);
>>                 f2fs_bug_on(sbi, 1);
>>      +        }
>>      #endif
>>      +        if (!exist) {
>>      +            f2fs_msg(sbi->sb, KERN_ERR,
>>      +                "Bitmap was cleared, blk:%u", blkaddr);
>>      +            f2fs_bug_on(sbi, 1);
>>             }
>>      +
>>             if (f2fs_discard_en(sbi) &&
>>                 f2fs_test_and_clear_bit(offset, se->discard_map))
>>                 sbi->discard_blks++;
>>      --
>>      2.13.0.90.g1eb437020
>>
>>      On 2017/8/1 15:56, Yunlong Song wrote:
>>      > When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
>>      > cur_valid_map_mir update is skipped unlikely, so fix it.
>>      >
>>      > Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>      > ---
>>      >  fs/f2fs/segment.c | 8 ++++++++
>>      >  1 file changed, 8 insertions(+)
>>      >
>>      > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>      > index 151968e..6f7731a 100644
>>      > --- a/fs/f2fs/segment.c
>>      > +++ b/fs/f2fs/segment.c
>>      > @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>      *sbi, block_t blkaddr, int del)
>>      >              f2fs_bug_on(sbi, 1);
>>      >  #endif
>>      >          }
>>      > +#ifdef CONFIG_F2FS_CHECK_FS
>>      > +        else
>>      > +            f2fs_set_bit(offset, se->cur_valid_map_mir);
>>      > +#endif
>>      >          if (f2fs_discard_en(sbi) &&
>>      >              !f2fs_test_and_set_bit(offset, se->discard_map))
>>      >              sbi->discard_blks--;
>>      > @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>      *sbi, block_t blkaddr, int del)
>>      >              f2fs_bug_on(sbi, 1);
>>      >  #endif
>>      >          }
>>      > +#ifdef CONFIG_F2FS_CHECK_FS
>>      > +        else
>>      > +            f2fs_clear_bit(offset, se->cur_valid_map_mir);
>>      > +#endif
>>      >          if (f2fs_discard_en(sbi) &&
>>      >              f2fs_test_and_clear_bit(offset, se->discard_map))
>>      >              sbi->discard_blks++;
>>      >
>>
>
> .
>

-- 
Thanks,
Yunlong Song

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

* Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
  2017-08-02  8:02       ` Yunlong Song
@ 2017-08-02  8:47         ` Chao Yu
  2017-08-02  9:00           ` Yunlong Song
  0 siblings, 1 reply; 12+ messages in thread
From: Chao Yu @ 2017-08-02  8:47 UTC (permalink / raw)
  To: Yunlong Song, Yunlong Song, Chao Yu
  Cc: jaegeuk, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel,
	linux-kernel

Hi Yunlong,

On 2017/8/2 16:02, Yunlong Song wrote:
> Hi Chao,
>      For the memory overflow or cache bit-transition case, when CHECK_FS 
> is on, should f2fs
> enter panic or just provide WARNing information? I prefer WARNing, 
> because it is not f2fs's
> fault, f2fs does not need to pay for it, why not use WARN_ON instead?

Actually, if that happens, I think both original and mirror bitmap datas are not
trustable, it's harmful to continue to trigger write IOs based on those bitmap
datas. So IMO, it would be better to trigger bug_on.

At least, if only warning is showed, we'd better to turn f2fs to readonly and
fail current write IO.

Thanks,

> 
> On 2017/8/2 9:34, Chao Yu wrote:
>> Hi Yunlong,
>>
>> On 2017/8/2 0:59, Yunlong Song wrote:
>>> Hi Chao,
>>>      I think there is no need to test mirror bitmap when original bitmap's check
>>> get passed, it is an instruction waste for "test". By the way, previous patch
>>> uses WARN to skip trigger panic when the memory is flipped, I think it is proper
>>> to not trigger panic in this situation, because it is not code bug at all in
>>> such case.
>> Original idea is trying to use bitmap mirror to detect bitmap corruption caused
>> by memory overflow or bit-transition of cache.
>>
>> So if we encounter inconsistency in between original bitmap and mirror bitmap,
>> there may be memory overflow or cache bit-transition, we need to detect that;
>> another case is that both bitmap is consistent, but we are trying to reuse valid
>> block address or free invalid block address, indeed that is a bug.
>>
>> Below modification can cover above two cases, which make original idea of
>> introducing mirror bitmap check working correctly.
>>
>> Thanks,
>>
>>> On 08/01/2017 23:44, Chao Yu <mailto:chao@kernel.org> wrote:
>>>
>>>      Hi Yunlong,
>>>
>>>      How about checking consistence in between original and mirror bitmap all
>>>      the time as below?
>>>
>>>      ---
>>>      fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
>>>      1 file changed, 27 insertions(+), 14 deletions(-)
>>>
>>>      diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>      index a26c24dae70c..f32a19cf486a 100644
>>>      --- a/fs/f2fs/segment.c
>>>      +++ b/fs/f2fs/segment.c
>>>      @@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi,
>>>      block_t blkaddr, int del)
>>>         struct seg_entry *se;
>>>         unsigned int segno, offset;
>>>         long int new_vblocks;
>>>      +    bool exist, mir_exist;
>>>
>>>         segno = GET_SEGNO(sbi, blkaddr);
>>>
>>>      @@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>      *sbi, block_t blkaddr, int del)
>>>
>>>         /* Update valid block bitmap */
>>>         if (del > 0) {
>>>      -        if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
>>>      +        exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
>>>      #ifdef CONFIG_F2FS_CHECK_FS
>>>      -            if (f2fs_test_and_set_bit(offset,
>>>      -                        se->cur_valid_map_mir))
>>>      -                f2fs_bug_on(sbi, 1);
>>>      -            else
>>>      -                WARN_ON(1);
>>>      -#else
>>>      +        mir_exist = f2fs_test_and_set_bit(offset,
>>>      +                        se->cur_valid_map_mir);
>>>      +        if (exist != mir_exist) {
>>>      +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>      +                "when setting bitmap, blk:%u, old bit:%d",
>>>      +                blkaddr, exist);
>>>                 f2fs_bug_on(sbi, 1);
>>>      +        }
>>>      #endif
>>>      +        if (exist) {
>>>      +            f2fs_msg(sbi->sb, KERN_ERR,
>>>      +                "Bitmap was set, blk:%u", blkaddr);
>>>      +            f2fs_bug_on(sbi, 1);
>>>             }
>>>      +
>>>             if (f2fs_discard_en(sbi) &&
>>>                 !f2fs_test_and_set_bit(offset, se->discard_map))
>>>                 sbi->discard_blks--;
>>>      @@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>      *sbi, block_t blkaddr, int del)
>>>                     se->ckpt_valid_blocks++;
>>>             }
>>>         } else {
>>>      -        if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
>>>      +        exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
>>>      #ifdef CONFIG_F2FS_CHECK_FS
>>>      -            if (!f2fs_test_and_clear_bit(offset,
>>>      -                        se->cur_valid_map_mir))
>>>      -                f2fs_bug_on(sbi, 1);
>>>      -            else
>>>      -                WARN_ON(1);
>>>      -#else
>>>      +        mir_exist = f2fs_test_and_clear_bit(offset,
>>>      +                        se->cur_valid_map_mir);
>>>      +        if (exist != mir_exist) {
>>>      +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>      +                "when clearing bitmap, blk:%u, old bit:%d",
>>>      +                blkaddr, exist);
>>>                 f2fs_bug_on(sbi, 1);
>>>      +        }
>>>      #endif
>>>      +        if (!exist) {
>>>      +            f2fs_msg(sbi->sb, KERN_ERR,
>>>      +                "Bitmap was cleared, blk:%u", blkaddr);
>>>      +            f2fs_bug_on(sbi, 1);
>>>             }
>>>      +
>>>             if (f2fs_discard_en(sbi) &&
>>>                 f2fs_test_and_clear_bit(offset, se->discard_map))
>>>                 sbi->discard_blks++;
>>>      --
>>>      2.13.0.90.g1eb437020
>>>
>>>      On 2017/8/1 15:56, Yunlong Song wrote:
>>>      > When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
>>>      > cur_valid_map_mir update is skipped unlikely, so fix it.
>>>      >
>>>      > Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>      > ---
>>>      >  fs/f2fs/segment.c | 8 ++++++++
>>>      >  1 file changed, 8 insertions(+)
>>>      >
>>>      > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>      > index 151968e..6f7731a 100644
>>>      > --- a/fs/f2fs/segment.c
>>>      > +++ b/fs/f2fs/segment.c
>>>      > @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>      *sbi, block_t blkaddr, int del)
>>>      >              f2fs_bug_on(sbi, 1);
>>>      >  #endif
>>>      >          }
>>>      > +#ifdef CONFIG_F2FS_CHECK_FS
>>>      > +        else
>>>      > +            f2fs_set_bit(offset, se->cur_valid_map_mir);
>>>      > +#endif
>>>      >          if (f2fs_discard_en(sbi) &&
>>>      >              !f2fs_test_and_set_bit(offset, se->discard_map))
>>>      >              sbi->discard_blks--;
>>>      > @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>      *sbi, block_t blkaddr, int del)
>>>      >              f2fs_bug_on(sbi, 1);
>>>      >  #endif
>>>      >          }
>>>      > +#ifdef CONFIG_F2FS_CHECK_FS
>>>      > +        else
>>>      > +            f2fs_clear_bit(offset, se->cur_valid_map_mir);
>>>      > +#endif
>>>      >          if (f2fs_discard_en(sbi) &&
>>>      >              f2fs_test_and_clear_bit(offset, se->discard_map))
>>>      >              sbi->discard_blks++;
>>>      >
>>>
>>
>> .
>>
> 

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

* Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map
  2017-08-02  8:47         ` Chao Yu
@ 2017-08-02  9:00           ` Yunlong Song
  0 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-02  9:00 UTC (permalink / raw)
  To: Chao Yu, Yunlong Song, Chao Yu
  Cc: jaegeuk, miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel,
	linux-kernel

Agree, it's safe to bug_on or readonly to protect data. This patch is fine.

On 2017/8/2 16:47, Chao Yu wrote:
> Hi Yunlong,
>
> On 2017/8/2 16:02, Yunlong Song wrote:
>> Hi Chao,
>>       For the memory overflow or cache bit-transition case, when CHECK_FS
>> is on, should f2fs
>> enter panic or just provide WARNing information? I prefer WARNing,
>> because it is not f2fs's
>> fault, f2fs does not need to pay for it, why not use WARN_ON instead?
> Actually, if that happens, I think both original and mirror bitmap datas are not
> trustable, it's harmful to continue to trigger write IOs based on those bitmap
> datas. So IMO, it would be better to trigger bug_on.
>
> At least, if only warning is showed, we'd better to turn f2fs to readonly and
> fail current write IO.
>
> Thanks,
>
>> On 2017/8/2 9:34, Chao Yu wrote:
>>> Hi Yunlong,
>>>
>>> On 2017/8/2 0:59, Yunlong Song wrote:
>>>> Hi Chao,
>>>>       I think there is no need to test mirror bitmap when original bitmap's check
>>>> get passed, it is an instruction waste for "test". By the way, previous patch
>>>> uses WARN to skip trigger panic when the memory is flipped, I think it is proper
>>>> to not trigger panic in this situation, because it is not code bug at all in
>>>> such case.
>>> Original idea is trying to use bitmap mirror to detect bitmap corruption caused
>>> by memory overflow or bit-transition of cache.
>>>
>>> So if we encounter inconsistency in between original bitmap and mirror bitmap,
>>> there may be memory overflow or cache bit-transition, we need to detect that;
>>> another case is that both bitmap is consistent, but we are trying to reuse valid
>>> block address or free invalid block address, indeed that is a bug.
>>>
>>> Below modification can cover above two cases, which make original idea of
>>> introducing mirror bitmap check working correctly.
>>>
>>> Thanks,
>>>
>>>> On 08/01/2017 23:44, Chao Yu <mailto:chao@kernel.org> wrote:
>>>>
>>>>       Hi Yunlong,
>>>>
>>>>       How about checking consistence in between original and mirror bitmap all
>>>>       the time as below?
>>>>
>>>>       ---
>>>>       fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
>>>>       1 file changed, 27 insertions(+), 14 deletions(-)
>>>>
>>>>       diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>       index a26c24dae70c..f32a19cf486a 100644
>>>>       --- a/fs/f2fs/segment.c
>>>>       +++ b/fs/f2fs/segment.c
>>>>       @@ -1510,6 +1510,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi,
>>>>       block_t blkaddr, int del)
>>>>          struct seg_entry *se;
>>>>          unsigned int segno, offset;
>>>>          long int new_vblocks;
>>>>       +    bool exist, mir_exist;
>>>>
>>>>          segno = GET_SEGNO(sbi, blkaddr);
>>>>
>>>>       @@ -1526,17 +1527,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>
>>>>          /* Update valid block bitmap */
>>>>          if (del > 0) {
>>>>       -        if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
>>>>       +        exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
>>>>       #ifdef CONFIG_F2FS_CHECK_FS
>>>>       -            if (f2fs_test_and_set_bit(offset,
>>>>       -                        se->cur_valid_map_mir))
>>>>       -                f2fs_bug_on(sbi, 1);
>>>>       -            else
>>>>       -                WARN_ON(1);
>>>>       -#else
>>>>       +        mir_exist = f2fs_test_and_set_bit(offset,
>>>>       +                        se->cur_valid_map_mir);
>>>>       +        if (exist != mir_exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>>       +                "when setting bitmap, blk:%u, old bit:%d",
>>>>       +                blkaddr, exist);
>>>>                  f2fs_bug_on(sbi, 1);
>>>>       +        }
>>>>       #endif
>>>>       +        if (exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR,
>>>>       +                "Bitmap was set, blk:%u", blkaddr);
>>>>       +            f2fs_bug_on(sbi, 1);
>>>>              }
>>>>       +
>>>>              if (f2fs_discard_en(sbi) &&
>>>>                  !f2fs_test_and_set_bit(offset, se->discard_map))
>>>>                  sbi->discard_blks--;
>>>>       @@ -1547,17 +1554,23 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>                      se->ckpt_valid_blocks++;
>>>>              }
>>>>          } else {
>>>>       -        if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
>>>>       +        exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
>>>>       #ifdef CONFIG_F2FS_CHECK_FS
>>>>       -            if (!f2fs_test_and_clear_bit(offset,
>>>>       -                        se->cur_valid_map_mir))
>>>>       -                f2fs_bug_on(sbi, 1);
>>>>       -            else
>>>>       -                WARN_ON(1);
>>>>       -#else
>>>>       +        mir_exist = f2fs_test_and_clear_bit(offset,
>>>>       +                        se->cur_valid_map_mir);
>>>>       +        if (exist != mir_exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
>>>>       +                "when clearing bitmap, blk:%u, old bit:%d",
>>>>       +                blkaddr, exist);
>>>>                  f2fs_bug_on(sbi, 1);
>>>>       +        }
>>>>       #endif
>>>>       +        if (!exist) {
>>>>       +            f2fs_msg(sbi->sb, KERN_ERR,
>>>>       +                "Bitmap was cleared, blk:%u", blkaddr);
>>>>       +            f2fs_bug_on(sbi, 1);
>>>>              }
>>>>       +
>>>>              if (f2fs_discard_en(sbi) &&
>>>>                  f2fs_test_and_clear_bit(offset, se->discard_map))
>>>>                  sbi->discard_blks++;
>>>>       --
>>>>       2.13.0.90.g1eb437020
>>>>
>>>>       On 2017/8/1 15:56, Yunlong Song wrote:
>>>>       > When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
>>>>       > cur_valid_map_mir update is skipped unlikely, so fix it.
>>>>       >
>>>>       > Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
>>>>       > ---
>>>>       >  fs/f2fs/segment.c | 8 ++++++++
>>>>       >  1 file changed, 8 insertions(+)
>>>>       >
>>>>       > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>       > index 151968e..6f7731a 100644
>>>>       > --- a/fs/f2fs/segment.c
>>>>       > +++ b/fs/f2fs/segment.c
>>>>       > @@ -1535,6 +1535,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>       >              f2fs_bug_on(sbi, 1);
>>>>       >  #endif
>>>>       >          }
>>>>       > +#ifdef CONFIG_F2FS_CHECK_FS
>>>>       > +        else
>>>>       > +            f2fs_set_bit(offset, se->cur_valid_map_mir);
>>>>       > +#endif
>>>>       >          if (f2fs_discard_en(sbi) &&
>>>>       >              !f2fs_test_and_set_bit(offset, se->discard_map))
>>>>       >              sbi->discard_blks--;
>>>>       > @@ -1556,6 +1560,10 @@ static void update_sit_entry(struct f2fs_sb_info
>>>>       *sbi, block_t blkaddr, int del)
>>>>       >              f2fs_bug_on(sbi, 1);
>>>>       >  #endif
>>>>       >          }
>>>>       > +#ifdef CONFIG_F2FS_CHECK_FS
>>>>       > +        else
>>>>       > +            f2fs_clear_bit(offset, se->cur_valid_map_mir);
>>>>       > +#endif
>>>>       >          if (f2fs_discard_en(sbi) &&
>>>>       >              f2fs_test_and_clear_bit(offset, se->discard_map))
>>>>       >              sbi->discard_blks++;
>>>>       >
>>>>
>>> .
>>>
>
> .
>

-- 
Thanks,
Yunlong Song

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

* [PATCH v2] f2fs: update cur_valid_map_mir together with cur_valid_map
  2017-08-01  7:56 ` Yunlong Song
@ 2017-08-02 13:20   ` Yunlong Song
  -1 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-02 13:20 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
cur_valid_map_mir update is skipped unlikely, so fix it. The fix
now changes the mirror check together with cur_valid_map all the
time.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 151968e..40e40c5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1508,6 +1508,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 	struct seg_entry *se;
 	unsigned int segno, offset;
 	long int new_vblocks;
+	bool exist, mir_exist;
 
 	segno = GET_SEGNO(sbi, blkaddr);
 
@@ -1524,17 +1525,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 
 	/* Update valid block bitmap */
 	if (del > 0) {
-		if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (f2fs_test_and_set_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_set_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when setting bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was wrongly set, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			!f2fs_test_and_set_bit(offset, se->discard_map))
 			sbi->discard_blks--;
@@ -1545,17 +1552,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 				se->ckpt_valid_blocks++;
 		}
 	} else {
-		if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (!f2fs_test_and_clear_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_clear_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when clearing bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (!exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was wrongly cleared, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			f2fs_test_and_clear_bit(offset, se->discard_map))
 			sbi->discard_blks++;
-- 
1.8.5.2

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

* [PATCH v2] f2fs: update cur_valid_map_mir together with cur_valid_map
@ 2017-08-02 13:20   ` Yunlong Song
  0 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-02 13:20 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

When cur_valid_map passes the f2fs_test_and_set(,clear)_bit test,
cur_valid_map_mir update is skipped unlikely, so fix it. The fix
now changes the mirror check together with cur_valid_map all the
time.

Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/segment.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 151968e..40e40c5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1508,6 +1508,7 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 	struct seg_entry *se;
 	unsigned int segno, offset;
 	long int new_vblocks;
+	bool exist, mir_exist;
 
 	segno = GET_SEGNO(sbi, blkaddr);
 
@@ -1524,17 +1525,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 
 	/* Update valid block bitmap */
 	if (del > 0) {
-		if (f2fs_test_and_set_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_set_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (f2fs_test_and_set_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_set_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when setting bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was wrongly set, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			!f2fs_test_and_set_bit(offset, se->discard_map))
 			sbi->discard_blks--;
@@ -1545,17 +1552,23 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 				se->ckpt_valid_blocks++;
 		}
 	} else {
-		if (!f2fs_test_and_clear_bit(offset, se->cur_valid_map)) {
+		exist = f2fs_test_and_clear_bit(offset, se->cur_valid_map);
 #ifdef CONFIG_F2FS_CHECK_FS
-			if (!f2fs_test_and_clear_bit(offset,
-						se->cur_valid_map_mir))
-				f2fs_bug_on(sbi, 1);
-			else
-				WARN_ON(1);
-#else
+		mir_exist = f2fs_test_and_clear_bit(offset,
+						se->cur_valid_map_mir);
+		if (exist != mir_exist) {
+			f2fs_msg(sbi->sb, KERN_ERR, "Inconsistent error "
+				"when clearing bitmap, blk:%u, old bit:%d",
+				blkaddr, exist);
 			f2fs_bug_on(sbi, 1);
+		}
 #endif
+		if (!exist) {
+			f2fs_msg(sbi->sb, KERN_ERR,
+				"Bitmap was wrongly cleared, blk:%u", blkaddr);
+			f2fs_bug_on(sbi, 1);
 		}
+
 		if (f2fs_discard_en(sbi) &&
 			f2fs_test_and_clear_bit(offset, se->discard_map))
 			sbi->discard_blks++;
-- 
1.8.5.2

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

* [PATCH] f2fs: do not change the valid_block value if cur_valid_map was wrongly set or cleared
  2017-08-02 13:20   ` Yunlong Song
@ 2017-08-02 14:16     ` Yunlong Song
  -1 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-02 14:16 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 40e40c5..9e3249a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1540,6 +1540,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_msg(sbi->sb, KERN_ERR,
 				"Bitmap was wrongly set, blk:%u", blkaddr);
 			f2fs_bug_on(sbi, 1);
+			se->valid_blocks--;
+			del = 0;
 		}
 
 		if (f2fs_discard_en(sbi) &&
@@ -1567,6 +1569,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_msg(sbi->sb, KERN_ERR,
 				"Bitmap was wrongly cleared, blk:%u", blkaddr);
 			f2fs_bug_on(sbi, 1);
+			se->valid_blocks++;
+			del = 0;
 		}
 
 		if (f2fs_discard_en(sbi) &&
-- 
1.8.5.2

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

* [PATCH] f2fs: do not change the valid_block value if cur_valid_map was wrongly set or cleared
@ 2017-08-02 14:16     ` Yunlong Song
  0 siblings, 0 replies; 12+ messages in thread
From: Yunlong Song @ 2017-08-02 14:16 UTC (permalink / raw)
  To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
  Cc: miaoxie, bintian.wang, linux-fsdevel, linux-f2fs-devel, linux-kernel

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

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 40e40c5..9e3249a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1540,6 +1540,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_msg(sbi->sb, KERN_ERR,
 				"Bitmap was wrongly set, blk:%u", blkaddr);
 			f2fs_bug_on(sbi, 1);
+			se->valid_blocks--;
+			del = 0;
 		}
 
 		if (f2fs_discard_en(sbi) &&
@@ -1567,6 +1569,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
 			f2fs_msg(sbi->sb, KERN_ERR,
 				"Bitmap was wrongly cleared, blk:%u", blkaddr);
 			f2fs_bug_on(sbi, 1);
+			se->valid_blocks++;
+			del = 0;
 		}
 
 		if (f2fs_discard_en(sbi) &&
-- 
1.8.5.2

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

* Re: [f2fs-dev] [PATCH] f2fs: do not change the valid_block value if cur_valid_map was wrongly set or cleared
  2017-08-02 14:16     ` Yunlong Song
  (?)
@ 2017-08-05  1:23     ` Chao Yu
  -1 siblings, 0 replies; 12+ messages in thread
From: Chao Yu @ 2017-08-05  1:23 UTC (permalink / raw)
  To: Yunlong Song, jaegeuk, yuchao0, yunlong.song
  Cc: linux-fsdevel, linux-kernel, linux-f2fs-devel

On 2017/8/2 22:16, Yunlong Song wrote:
> Signed-off-by: Yunlong Song <yunlong.song@huawei.com>

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

> ---
>  fs/f2fs/segment.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 40e40c5..9e3249a 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1540,6 +1540,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  			f2fs_msg(sbi->sb, KERN_ERR,
>  				"Bitmap was wrongly set, blk:%u", blkaddr);
>  			f2fs_bug_on(sbi, 1);
> +			se->valid_blocks--;
> +			del = 0;
>  		}
>  
>  		if (f2fs_discard_en(sbi) &&
> @@ -1567,6 +1569,8 @@ static void update_sit_entry(struct f2fs_sb_info *sbi, block_t blkaddr, int del)
>  			f2fs_msg(sbi->sb, KERN_ERR,
>  				"Bitmap was wrongly cleared, blk:%u", blkaddr);
>  			f2fs_bug_on(sbi, 1);
> +			se->valid_blocks++;
> +			del = 0;
>  		}
>  
>  		if (f2fs_discard_en(sbi) &&
> 

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

end of thread, other threads:[~2017-08-05  1:24 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-01  7:56 [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map Yunlong Song
2017-08-01  7:56 ` Yunlong Song
2017-08-01 15:44 ` Chao Yu
     [not found]   ` <c1935af.77af.15d9ebd35e3.Coremail.sylinux@163.com>
2017-08-02  1:34     ` Chao Yu
2017-08-02  8:02       ` Yunlong Song
2017-08-02  8:47         ` Chao Yu
2017-08-02  9:00           ` Yunlong Song
2017-08-02 13:20 ` [PATCH v2] " Yunlong Song
2017-08-02 13:20   ` Yunlong Song
2017-08-02 14:16   ` [PATCH] f2fs: do not change the valid_block value if cur_valid_map was wrongly set or cleared Yunlong Song
2017-08-02 14:16     ` Yunlong Song
2017-08-05  1:23     ` [f2fs-dev] " Chao Yu

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.