[PATCH 1/2] f2fs: allow unfixed f2fs_checkpoint.checksum_offset

[PATCH 1/2] f2fs: allow unfixed f2fs_checkpoint.checksum_offset

[Chao Yu]

Previously, f2fs_checkpoint.checksum_offset points fixed position of
f2fs_checkpoint structure:

"#define CP_CHKSUM_OFFSET	4092"

It is unnecessary, and it breaks the consecutiveness of nat and sit
bitmap stored across checkpoint park block and payload blocks.

This patch allows f2fs to handle unfixed .checksum_offset.

In addition, for the case checksum value is stored in the middle of
checkpoint park, calculating checksum value with superposition method
like we did for inode_checksum.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/checkpoint.c    | 27 +++++++++++++++++++++------
 include/linux/f2fs_fs.h |  4 ++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 441814607b13..a25556aef8cc 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -794,13 +794,27 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
 	}
 }
 
+static __u32 f2fs_checkpoint_chksum(struct f2fs_sb_info *sbi,
+						struct f2fs_checkpoint *ckpt)
+{
+	unsigned int chksum_ofs = le32_to_cpu(ckpt->checksum_offset);
+	__u32 chksum;
+
+	chksum = f2fs_crc32(sbi, ckpt, chksum_ofs);
+	if (chksum_ofs < CP_CHKSUM_OFFSET) {
+		chksum_ofs += sizeof(chksum);
+		chksum = f2fs_chksum(sbi, chksum, (__u8 *)ckpt + chksum_ofs,
+						F2FS_BLKSIZE - chksum_ofs);
+	}
+	return chksum;
+}
+
 static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
 		struct f2fs_checkpoint **cp_block, struct page **cp_page,
 		unsigned long long *version)
 {
-	unsigned long blk_size = sbi->blocksize;
 	size_t crc_offset = 0;
-	__u32 crc = 0;
+	__u32 crc;
 
 	*cp_page = f2fs_get_meta_page(sbi, cp_addr);
 	if (IS_ERR(*cp_page))
@@ -809,15 +823,16 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
 	*cp_block = (struct f2fs_checkpoint *)page_address(*cp_page);
 
 	crc_offset = le32_to_cpu((*cp_block)->checksum_offset);
-	if (crc_offset > (blk_size - sizeof(__le32))) {
+	if (crc_offset < CP_MIN_CHKSUM_OFFSET ||
+			crc_offset > CP_CHKSUM_OFFSET) {
 		f2fs_put_page(*cp_page, 1);
 		f2fs_msg(sbi->sb, KERN_WARNING,
 			"invalid crc_offset: %zu", crc_offset);
 		return -EINVAL;
 	}
 
-	crc = cur_cp_crc(*cp_block);
-	if (!f2fs_crc_valid(sbi, crc, *cp_block, crc_offset)) {
+	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
+	if (crc != cur_cp_crc(*cp_block)) {
 		f2fs_put_page(*cp_page, 1);
 		f2fs_msg(sbi->sb, KERN_WARNING, "invalid crc value");
 		return -EINVAL;
@@ -1425,7 +1440,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	get_sit_bitmap(sbi, __bitmap_ptr(sbi, SIT_BITMAP));
 	get_nat_bitmap(sbi, __bitmap_ptr(sbi, NAT_BITMAP));
 
-	crc32 = f2fs_crc32(sbi, ckpt, le32_to_cpu(ckpt->checksum_offset));
+	crc32 = f2fs_checkpoint_chksum(sbi, ckpt);
 	*((__le32 *)((unsigned char *)ckpt +
 				le32_to_cpu(ckpt->checksum_offset)))
 				= cpu_to_le32(crc32);
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index 55da9abed023..65559900d4d7 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -164,6 +164,10 @@ struct f2fs_checkpoint {
 	unsigned char sit_nat_version_bitmap[1];
 } __packed;
 
+#define CP_CHKSUM_OFFSET	4092	/* default chksum offset in checkpoint */
+#define CP_MIN_CHKSUM_OFFSET						\
+	(offsetof(struct f2fs_checkpoint, sit_nat_version_bitmap))
+
 /*
  * For orphan inode management
  */
-- 
2.18.0.rc1

[PATCH 2/2] f2fs: relocate chksum_offset for large_nat_bitmap feature

[Chao Yu]

For large_nat_bitmap feature, there is a design flaw:

Previous:

struct f2fs_checkpoint layout:
+--------------------------+  0x0000
| checkpoint_ver           |
| ......                   |
| checksum_offset          |------+
| ......                   |      |
| sit_nat_version_bitmap[] |<-----|-------+
| ......                   |      |       |
| checksum_value           |<-----+       |
+--------------------------+  0x1000      |
|                          |      nat_bitmap + sit_bitmap
| payload blocks           |              |
|                          |              |
+--------------------------|<-------------+

Obviously, if nat_bitmap size + sit_bitmap size is larger than
MAX_BITMAP_SIZE_IN_CKPT, nat_bitmap or sit_bitmap may overlap
checkpoint checksum's position, once checkpoint() is triggered
from kernel, nat or sit bitmap will be damaged by checksum field.

In order to fix this, let's relocate checksum_value's position
to the head of sit_nat_version_bitmap as below, then nat/sit
bitmap and chksum value update will become safe.

After:

struct f2fs_checkpoint layout:
+--------------------------+  0x0000
| checkpoint_ver           |
| ......                   |
| checksum_offset          |------+
| ......                   |      |
| sit_nat_version_bitmap[] |<-----+
| ......                   |<-------------+
|                          |              |
+--------------------------+  0x1000      |
|                          |      nat_bitmap + sit_bitmap
| payload blocks           |              |
|                          |              |
+--------------------------|<-------------+

Related report and discussion:

https://sourceforge.net/p/linux-f2fs/mailman/message/36642346/

Reported-by: Park Ju Hyung <qkrwngud825@gmail.com>
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/checkpoint.c | 11 +++++++++++
 fs/f2fs/f2fs.h       |  6 +++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index a25556aef8cc..081eee9e3d92 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -831,6 +831,17 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
 		return -EINVAL;
 	}
 
+	if (__is_set_ckpt_flags(*cp_block, CP_LARGE_NAT_BITMAP_FLAG)) {
+		if (crc_offset != CP_MIN_CHKSUM_OFFSET) {
+			f2fs_put_page(*cp_page, 1);
+			f2fs_msg(sbi->sb, KERN_WARNING,
+				"layout of large_nat_bitmap is deprecated, "
+				"run fsck to repair, chksum_offset: %zu",
+				crc_offset);
+			return -EINVAL;
+		}
+	}
+
 	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
 	if (crc != cur_cp_crc(*cp_block)) {
 		f2fs_put_page(*cp_page, 1);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 31531711e148..f5ffc09705eb 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1911,7 +1911,11 @@ static inline void *__bitmap_ptr(struct f2fs_sb_info *sbi, int flag)
 	if (is_set_ckpt_flags(sbi, CP_LARGE_NAT_BITMAP_FLAG)) {
 		offset = (flag == SIT_BITMAP) ?
 			le32_to_cpu(ckpt->nat_ver_bitmap_bytesize) : 0;
-		return &ckpt->sit_nat_version_bitmap + offset;
+		/*
+		 * if large_nat_bitmap feature is enabled, leave checksum
+		 * protection for all nat/sit bitmaps.
+		 */
+		return &ckpt->sit_nat_version_bitmap + offset + sizeof(__le32);
 	}
 
 	if (__cp_payload(sbi) > 0) {
-- 
2.18.0.rc1

Re: [PATCH 1/2] f2fs: allow unfixed f2fs_checkpoint.checksum_offset

[Jaegeuk Kim]

On 04/22, Chao Yu wrote:
> Previously, f2fs_checkpoint.checksum_offset points fixed position of
> f2fs_checkpoint structure:
> 
> "#define CP_CHKSUM_OFFSET	4092"
> 
> It is unnecessary, and it breaks the consecutiveness of nat and sit
> bitmap stored across checkpoint park block and payload blocks.
> 
> This patch allows f2fs to handle unfixed .checksum_offset.
> 
> In addition, for the case checksum value is stored in the middle of
> checkpoint park, calculating checksum value with superposition method
> like we did for inode_checksum.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/checkpoint.c    | 27 +++++++++++++++++++++------
>  include/linux/f2fs_fs.h |  4 ++++
>  2 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 441814607b13..a25556aef8cc 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -794,13 +794,27 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
>  	}
>  }
>  
> +static __u32 f2fs_checkpoint_chksum(struct f2fs_sb_info *sbi,
> +						struct f2fs_checkpoint *ckpt)
> +{
> +	unsigned int chksum_ofs = le32_to_cpu(ckpt->checksum_offset);
> +	__u32 chksum;
> +
> +	chksum = f2fs_crc32(sbi, ckpt, chksum_ofs);
> +	if (chksum_ofs < CP_CHKSUM_OFFSET) {
> +		chksum_ofs += sizeof(chksum);
> +		chksum = f2fs_chksum(sbi, chksum, (__u8 *)ckpt + chksum_ofs,
> +						F2FS_BLKSIZE - chksum_ofs);

Do we need to cover __cp_payload(sbi) * blksize - chksum_ofs?

> +	}
> +	return chksum;
> +}
> +
>  static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
>  		struct f2fs_checkpoint **cp_block, struct page **cp_page,
>  		unsigned long long *version)
>  {
> -	unsigned long blk_size = sbi->blocksize;
>  	size_t crc_offset = 0;
> -	__u32 crc = 0;
> +	__u32 crc;
>  
>  	*cp_page = f2fs_get_meta_page(sbi, cp_addr);
>  	if (IS_ERR(*cp_page))
> @@ -809,15 +823,16 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
>  	*cp_block = (struct f2fs_checkpoint *)page_address(*cp_page);
>  
>  	crc_offset = le32_to_cpu((*cp_block)->checksum_offset);
> -	if (crc_offset > (blk_size - sizeof(__le32))) {
> +	if (crc_offset < CP_MIN_CHKSUM_OFFSET ||
> +			crc_offset > CP_CHKSUM_OFFSET) {
>  		f2fs_put_page(*cp_page, 1);
>  		f2fs_msg(sbi->sb, KERN_WARNING,
>  			"invalid crc_offset: %zu", crc_offset);
>  		return -EINVAL;
>  	}
>  
> -	crc = cur_cp_crc(*cp_block);
> -	if (!f2fs_crc_valid(sbi, crc, *cp_block, crc_offset)) {
> +	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
> +	if (crc != cur_cp_crc(*cp_block)) {
>  		f2fs_put_page(*cp_page, 1);
>  		f2fs_msg(sbi->sb, KERN_WARNING, "invalid crc value");
>  		return -EINVAL;
> @@ -1425,7 +1440,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	get_sit_bitmap(sbi, __bitmap_ptr(sbi, SIT_BITMAP));
>  	get_nat_bitmap(sbi, __bitmap_ptr(sbi, NAT_BITMAP));
>  
> -	crc32 = f2fs_crc32(sbi, ckpt, le32_to_cpu(ckpt->checksum_offset));
> +	crc32 = f2fs_checkpoint_chksum(sbi, ckpt);
>  	*((__le32 *)((unsigned char *)ckpt +
>  				le32_to_cpu(ckpt->checksum_offset)))
>  				= cpu_to_le32(crc32);
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index 55da9abed023..65559900d4d7 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -164,6 +164,10 @@ struct f2fs_checkpoint {
>  	unsigned char sit_nat_version_bitmap[1];
>  } __packed;
>  
> +#define CP_CHKSUM_OFFSET	4092	/* default chksum offset in checkpoint */
> +#define CP_MIN_CHKSUM_OFFSET						\
> +	(offsetof(struct f2fs_checkpoint, sit_nat_version_bitmap))
> +
>  /*
>   * For orphan inode management
>   */
> -- 
> 2.18.0.rc1

Re: [f2fs-dev] [PATCH 1/2] f2fs: allow unfixed f2fs_checkpoint.checksum_offset

[Jaegeuk Kim]

On 04/23, Jaegeuk Kim wrote:
> On 04/22, Chao Yu wrote:
> > Previously, f2fs_checkpoint.checksum_offset points fixed position of
> > f2fs_checkpoint structure:
> > 
> > "#define CP_CHKSUM_OFFSET	4092"
> > 
> > It is unnecessary, and it breaks the consecutiveness of nat and sit
> > bitmap stored across checkpoint park block and payload blocks.
> > 
> > This patch allows f2fs to handle unfixed .checksum_offset.
> > 
> > In addition, for the case checksum value is stored in the middle of
> > checkpoint park, calculating checksum value with superposition method
> > like we did for inode_checksum.
> > 
> > Signed-off-by: Chao Yu <yuchao0@huawei.com>
> > ---
> >  fs/f2fs/checkpoint.c    | 27 +++++++++++++++++++++------
> >  include/linux/f2fs_fs.h |  4 ++++
> >  2 files changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 441814607b13..a25556aef8cc 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -794,13 +794,27 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
> >  	}
> >  }
> >  
> > +static __u32 f2fs_checkpoint_chksum(struct f2fs_sb_info *sbi,
> > +						struct f2fs_checkpoint *ckpt)
> > +{
> > +	unsigned int chksum_ofs = le32_to_cpu(ckpt->checksum_offset);
> > +	__u32 chksum;
> > +
> > +	chksum = f2fs_crc32(sbi, ckpt, chksum_ofs);
> > +	if (chksum_ofs < CP_CHKSUM_OFFSET) {
> > +		chksum_ofs += sizeof(chksum);
> > +		chksum = f2fs_chksum(sbi, chksum, (__u8 *)ckpt + chksum_ofs,
> > +						F2FS_BLKSIZE - chksum_ofs);
> 
> Do we need to cover __cp_payload(sbi) * blksize - chksum_ofs?

Self answer - it'd be fine to get 4KB only, since payload will be covered
by entire checkpoint pack.

> 
> > +	}
> > +	return chksum;
> > +}
> > +
> >  static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
> >  		struct f2fs_checkpoint **cp_block, struct page **cp_page,
> >  		unsigned long long *version)
> >  {
> > -	unsigned long blk_size = sbi->blocksize;
> >  	size_t crc_offset = 0;
> > -	__u32 crc = 0;
> > +	__u32 crc;
> >  
> >  	*cp_page = f2fs_get_meta_page(sbi, cp_addr);
> >  	if (IS_ERR(*cp_page))
> > @@ -809,15 +823,16 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
> >  	*cp_block = (struct f2fs_checkpoint *)page_address(*cp_page);
> >  
> >  	crc_offset = le32_to_cpu((*cp_block)->checksum_offset);
> > -	if (crc_offset > (blk_size - sizeof(__le32))) {
> > +	if (crc_offset < CP_MIN_CHKSUM_OFFSET ||
> > +			crc_offset > CP_CHKSUM_OFFSET) {
> >  		f2fs_put_page(*cp_page, 1);
> >  		f2fs_msg(sbi->sb, KERN_WARNING,
> >  			"invalid crc_offset: %zu", crc_offset);
> >  		return -EINVAL;
> >  	}
> >  
> > -	crc = cur_cp_crc(*cp_block);
> > -	if (!f2fs_crc_valid(sbi, crc, *cp_block, crc_offset)) {
> > +	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
> > +	if (crc != cur_cp_crc(*cp_block)) {
> >  		f2fs_put_page(*cp_page, 1);
> >  		f2fs_msg(sbi->sb, KERN_WARNING, "invalid crc value");
> >  		return -EINVAL;
> > @@ -1425,7 +1440,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
> >  	get_sit_bitmap(sbi, __bitmap_ptr(sbi, SIT_BITMAP));
> >  	get_nat_bitmap(sbi, __bitmap_ptr(sbi, NAT_BITMAP));
> >  
> > -	crc32 = f2fs_crc32(sbi, ckpt, le32_to_cpu(ckpt->checksum_offset));
> > +	crc32 = f2fs_checkpoint_chksum(sbi, ckpt);
> >  	*((__le32 *)((unsigned char *)ckpt +
> >  				le32_to_cpu(ckpt->checksum_offset)))
> >  				= cpu_to_le32(crc32);
> > diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> > index 55da9abed023..65559900d4d7 100644
> > --- a/include/linux/f2fs_fs.h
> > +++ b/include/linux/f2fs_fs.h
> > @@ -164,6 +164,10 @@ struct f2fs_checkpoint {
> >  	unsigned char sit_nat_version_bitmap[1];
> >  } __packed;
> >  
> > +#define CP_CHKSUM_OFFSET	4092	/* default chksum offset in checkpoint */
> > +#define CP_MIN_CHKSUM_OFFSET						\
> > +	(offsetof(struct f2fs_checkpoint, sit_nat_version_bitmap))
> > +
> >  /*
> >   * For orphan inode management
> >   */
> > -- 
> > 2.18.0.rc1
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH 1/2] f2fs: allow unfixed f2fs_checkpoint.checksum_offset

[Chao Yu]

On 2019/4/24 4:56, Jaegeuk Kim wrote:
> On 04/23, Jaegeuk Kim wrote:
>> On 04/22, Chao Yu wrote:
>>> Previously, f2fs_checkpoint.checksum_offset points fixed position of
>>> f2fs_checkpoint structure:
>>>
>>> "#define CP_CHKSUM_OFFSET	4092"
>>>
>>> It is unnecessary, and it breaks the consecutiveness of nat and sit
>>> bitmap stored across checkpoint park block and payload blocks.
>>>
>>> This patch allows f2fs to handle unfixed .checksum_offset.
>>>
>>> In addition, for the case checksum value is stored in the middle of
>>> checkpoint park, calculating checksum value with superposition method
>>> like we did for inode_checksum.
>>>
>>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>>> ---
>>>  fs/f2fs/checkpoint.c    | 27 +++++++++++++++++++++------
>>>  include/linux/f2fs_fs.h |  4 ++++
>>>  2 files changed, 25 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 441814607b13..a25556aef8cc 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -794,13 +794,27 @@ static void write_orphan_inodes(struct f2fs_sb_info *sbi, block_t start_blk)
>>>  	}
>>>  }
>>>  
>>> +static __u32 f2fs_checkpoint_chksum(struct f2fs_sb_info *sbi,
>>> +						struct f2fs_checkpoint *ckpt)
>>> +{
>>> +	unsigned int chksum_ofs = le32_to_cpu(ckpt->checksum_offset);
>>> +	__u32 chksum;
>>> +
>>> +	chksum = f2fs_crc32(sbi, ckpt, chksum_ofs);
>>> +	if (chksum_ofs < CP_CHKSUM_OFFSET) {
>>> +		chksum_ofs += sizeof(chksum);
>>> +		chksum = f2fs_chksum(sbi, chksum, (__u8 *)ckpt + chksum_ofs,
>>> +						F2FS_BLKSIZE - chksum_ofs);
>>
>> Do we need to cover __cp_payload(sbi) * blksize - chksum_ofs?
> 
> Self answer - it'd be fine to get 4KB only, since payload will be covered
> by entire checkpoint pack.

Yup, ;)

Thanks,

> 
>>
>>> +	}
>>> +	return chksum;
>>> +}
>>> +
>>>  static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
>>>  		struct f2fs_checkpoint **cp_block, struct page **cp_page,
>>>  		unsigned long long *version)
>>>  {
>>> -	unsigned long blk_size = sbi->blocksize;
>>>  	size_t crc_offset = 0;
>>> -	__u32 crc = 0;
>>> +	__u32 crc;
>>>  
>>>  	*cp_page = f2fs_get_meta_page(sbi, cp_addr);
>>>  	if (IS_ERR(*cp_page))
>>> @@ -809,15 +823,16 @@ static int get_checkpoint_version(struct f2fs_sb_info *sbi, block_t cp_addr,
>>>  	*cp_block = (struct f2fs_checkpoint *)page_address(*cp_page);
>>>  
>>>  	crc_offset = le32_to_cpu((*cp_block)->checksum_offset);
>>> -	if (crc_offset > (blk_size - sizeof(__le32))) {
>>> +	if (crc_offset < CP_MIN_CHKSUM_OFFSET ||
>>> +			crc_offset > CP_CHKSUM_OFFSET) {
>>>  		f2fs_put_page(*cp_page, 1);
>>>  		f2fs_msg(sbi->sb, KERN_WARNING,
>>>  			"invalid crc_offset: %zu", crc_offset);
>>>  		return -EINVAL;
>>>  	}
>>>  
>>> -	crc = cur_cp_crc(*cp_block);
>>> -	if (!f2fs_crc_valid(sbi, crc, *cp_block, crc_offset)) {
>>> +	crc = f2fs_checkpoint_chksum(sbi, *cp_block);
>>> +	if (crc != cur_cp_crc(*cp_block)) {
>>>  		f2fs_put_page(*cp_page, 1);
>>>  		f2fs_msg(sbi->sb, KERN_WARNING, "invalid crc value");
>>>  		return -EINVAL;
>>> @@ -1425,7 +1440,7 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>  	get_sit_bitmap(sbi, __bitmap_ptr(sbi, SIT_BITMAP));
>>>  	get_nat_bitmap(sbi, __bitmap_ptr(sbi, NAT_BITMAP));
>>>  
>>> -	crc32 = f2fs_crc32(sbi, ckpt, le32_to_cpu(ckpt->checksum_offset));
>>> +	crc32 = f2fs_checkpoint_chksum(sbi, ckpt);
>>>  	*((__le32 *)((unsigned char *)ckpt +
>>>  				le32_to_cpu(ckpt->checksum_offset)))
>>>  				= cpu_to_le32(crc32);
>>> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
>>> index 55da9abed023..65559900d4d7 100644
>>> --- a/include/linux/f2fs_fs.h
>>> +++ b/include/linux/f2fs_fs.h
>>> @@ -164,6 +164,10 @@ struct f2fs_checkpoint {
>>>  	unsigned char sit_nat_version_bitmap[1];
>>>  } __packed;
>>>  
>>> +#define CP_CHKSUM_OFFSET	4092	/* default chksum offset in checkpoint */
>>> +#define CP_MIN_CHKSUM_OFFSET						\
>>> +	(offsetof(struct f2fs_checkpoint, sit_nat_version_bitmap))
>>> +
>>>  /*
>>>   * For orphan inode management
>>>   */
>>> -- 
>>> 2.18.0.rc1
>>
>>
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

Re: [f2fs-dev] [PATCH 2/2] f2fs: relocate chksum_offset for large_nat_bitmap feature

[Ju Hyung Park]

Hi Chao,

On Mon, Apr 22, 2019 at 6:34 PM Chao Yu <yuchao0@huawei.com> wrote:
> +       if (__is_set_ckpt_flags(*cp_block, CP_LARGE_NAT_BITMAP_FLAG)) {
> +               if (crc_offset != CP_MIN_CHKSUM_OFFSET) {
> +                       f2fs_put_page(*cp_page, 1);
> +                       f2fs_msg(sbi->sb, KERN_WARNING,
> +                               "layout of large_nat_bitmap is deprecated, "
> +                               "run fsck to repair, chksum_offset: %zu",
> +                               crc_offset);
> +                       return -EINVAL;
> +               }
> +       }
> +

I try not to be a Grammar Nazi and a jerk on every patches, but since
this is a message a user would directly encounter, I'd like to see a
bit less ambiguous message.

How about "using deprecated layout of large_nat_bitmap, please run
fsck v1.13.0 or higher to repair, chksum_offset: %zu"?
The original message seems to suggest that large_nat_bitmap is
deprecated outright.

Also, I'd like to suggest to write down an actual version of
f2fs-tools here as we've seen older versions of fsck doing even more
damage and the users might not have the latest f2fs-tools installed.

Btw, what happens if we use the latest fsck to fix the corrupted image
and use the older kernel to mount it?
Would it even mount?

Thanks.

Re: [f2fs-dev] [PATCH 2/2] f2fs: relocate chksum_offset for large_nat_bitmap feature

[Chao Yu]

Hi Ju Hyung,

On 2019/4/24 19:43, Ju Hyung Park wrote:
> Hi Chao,
> 
> On Mon, Apr 22, 2019 at 6:34 PM Chao Yu <yuchao0@huawei.com> wrote:
>> +       if (__is_set_ckpt_flags(*cp_block, CP_LARGE_NAT_BITMAP_FLAG)) {
>> +               if (crc_offset != CP_MIN_CHKSUM_OFFSET) {
>> +                       f2fs_put_page(*cp_page, 1);
>> +                       f2fs_msg(sbi->sb, KERN_WARNING,
>> +                               "layout of large_nat_bitmap is deprecated, "
>> +                               "run fsck to repair, chksum_offset: %zu",
>> +                               crc_offset);
>> +                       return -EINVAL;
>> +               }
>> +       }
>> +
> 
> I try not to be a Grammar Nazi and a jerk on every patches, but since
> this is a message a user would directly encounter, I'd like to see a
> bit less ambiguous message.

Please feel free to give us your opinion or suggestion. :)

> 
> How about "using deprecated layout of large_nat_bitmap, please run
> fsck v1.13.0 or higher to repair, chksum_offset: %zu"?
> The original message seems to suggest that large_nat_bitmap is
> deprecated outright.
> 
> Also, I'd like to suggest to write down an actual version of
> f2fs-tools here as we've seen older versions of fsck doing even more
> damage and the users might not have the latest f2fs-tools installed.

Agreed, user should be told which version of fsck can repair that problem, will
update the message in next version.

> 
> Btw, what happens if we use the latest fsck to fix the corrupted image
> and use the older kernel to mount it?
> Would it even mount?

No, since fixed image is using a new layout, how about giving the detailed
information about which version of kernel user should update to, once we detect
such issue and trigger the repairing?

Thanks,

> 
> Thanks.
> .
>