Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery

Gao Xiang Thu, 01 Feb 2018 05:58:45 -0800

Hi Chao,

On 2018/2/1 21:27, Chao Yu wrote:

Hi Xiang,


On 2018/2/1 0:16, Gaoxiang (OS) wrote:

This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver
since crc | cp_ver is more likely to get a collision or become 11..1111 | 
cp_ver.


FYI, we have discussed about this before:

https://patchwork.kernel.org/patch/9342639/

At that time, cp_ver will always be initialized with 1, so there is almost a
little chance to use high 32 bits, result in less collision, so I think it
will be OK.

But now, cp_ver will be initialized with a random 64 bits value, then the
collision will be increased, I agree that xor method will be better, but I'm
not sure we should use this, since layout change makes complicated handling
in codes for back compatibility.

And do you encounter any incorrect recovery, or is there any following feature
rely on this?

No... I just looked at node_footer because of another work and saw thispart of code by chance.I think XOR-ed calculation is much better in mathematics, and typicalmethod for mixing two values (eg. XOR encryption) is also XOR-edcalculation rather than OR-ed...


Thanks,


Thanks,


Signed-off-by: Gao Xiang <gaoxian...@huawei.com>
---
  fs/f2fs/checkpoint.c    |  4 ++--
  fs/f2fs/node.h          | 16 +++++++++++-----
  fs/f2fs/segment.c       |  3 ++-
  include/linux/f2fs_fs.h |  1 +
  4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8b0945b..9e7e63b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, 
struct cp_control *cpc)
        if (is_sbi_flag_set(sbi, SBI_NEED_FSCK))
                __set_ckpt_flags(ckpt, CP_FSCK_FLAG);

- /* set this flag to activate crc|cp_ver for recovery */

-       __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
+       /* set this flag to activate crc^cp_ver for recovery */
+       __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG);
        __clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);

spin_unlock_irqrestore(&sbi->cp_lock, flags);

diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index 081ef0d..7b9489f 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -293,8 +293,11 @@ static inline void fill_node_footer_blkaddr(struct page 
*page, block_t blkaddr)
        struct f2fs_node *rn = F2FS_NODE(page);
        __u64 cp_ver = cur_cp_version(ckpt);

- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))

-               cp_ver |= (cur_cp_crc(ckpt) << 32);
+       if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+               cp_ver ^= cur_cp_crc(ckpt) << 32;
+       /* for backward compatibility */
+       else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+               cp_ver |= cur_cp_crc(ckpt) << 32;

rn->footer.cp_ver = cpu_to_le64(cp_ver);

        rn->footer.next_blkaddr = cpu_to_le32(blkaddr);
@@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct page *page)

/* Don't care crc part, if fsck.f2fs sets it. */

        if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG))
-               return (cp_ver << 32) == (cpver_of_node(page) << 32);
+               return (__u32)cp_ver == (__u32)cpver_of_node(page);

- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))

-               cp_ver |= (cur_cp_crc(ckpt) << 32);
+       if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+               cp_ver ^= cur_cp_crc(ckpt) << 32;
+       /* for backward compatibility */
+       else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+               cp_ver |= cur_cp_crc(ckpt) << 32;

return cp_ver == cpver_of_node(page);

  }
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 205b0d9..64d0c1f 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct 
f2fs_sb_info *sbi,

if (force)

                new_curseg(sbi, type, true);
-       else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) &&
+       else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) ||
+               is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FLAG)) &&
                                        type == CURSEG_WARM_NODE)
                new_curseg(sbi, type, false);
        else if (curseg->alloc_type == LFS && is_next_segment_free(sbi, type))
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f7f0990..07ddf4b 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -117,6 +117,7 @@ struct f2fs_super_block {
  /*
   * For checkpoint
   */
+#define CP_CRC_RECOVERY_XOR_FLAG       0x00000800
  #define CP_LARGE_NAT_BITMAP_FLAG      0x00000400
  #define CP_NOCRC_RECOVERY_FLAG        0x00000200
  #define CP_TRIMMED_FLAG               0x00000100


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery

Reply via email to