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

[Chao Yu <yuchao0@huawei.com>]

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++;
>     >
>