Re: [PATCH] f2fs: update cur_valid_map_mir together with cur_valid_map

[Yunlong Song <yunlong.song@huawei.com>]

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