[PATCH] erofs-utils: code for superblock checksum calculation.

[PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

Adding code for superblock checksum calculation.

incorporated the changes suggested in previous patch.

Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
---
 include/erofs/config.h |  1 +
 include/erofs_fs.h     | 10 ++++++++
 mkfs/main.c            | 64 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/include/erofs/config.h b/include/erofs/config.h
index 05fe6b2..40cd466 100644
--- a/include/erofs/config.h
+++ b/include/erofs/config.h
@@ -22,6 +22,7 @@ struct erofs_configure {
 	char *c_src_path;
 	char *c_compr_alg_master;
 	int c_compr_level_master;
+	int c_feature_flags;
 };
 
 extern struct erofs_configure cfg;
diff --git a/include/erofs_fs.h b/include/erofs_fs.h
index 601b477..9ac2635 100644
--- a/include/erofs_fs.h
+++ b/include/erofs_fs.h
@@ -20,6 +20,16 @@
 #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
 #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
 
+/*
+ * feature definations.
+ */
+#define EROFS_DEFAULT_FEATURES		EROFS_FEATURE_SB_CHKSUM
+#define EROFS_FEATURE_SB_CHKSUM		0x0001
+
+
+#define EROFS_HAS_COMPAT_FEATURE(super,mask)	\
+	( le32_to_cpu((super)->features) & (mask) )
+
 struct erofs_super_block {
 /*  0 */__le32 magic;           /* in the little endian */
 /*  4 */__le32 checksum;        /* crc32c(super_block) */
diff --git a/mkfs/main.c b/mkfs/main.c
index f127fe1..355fd2c 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -31,6 +31,45 @@ static void usage(void)
 	fprintf(stderr, " -EX[,...] X=extended options\n");
 }
 
+#define CRCPOLY	0x82F63B78
+static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
+{
+	int i;
+	u32 crc = seed;
+
+	while (len--) {
+		crc ^= *in++;
+		for (i = 0; i < 8; i++) {
+			crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
+		}
+	}
+	erofs_dump("calculated crc: 0x%x\n", crc);
+	return crc;
+}
+
+char *feature_opts[] = {
+	"nosbcrc", NULL
+};
+#define O_SB_CKSUM	0
+
+static int parse_feature_subopts(char *opts)
+{
+	char *arg;
+
+	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
+	while (*opts != '\0') {
+		switch(getsubopt(&opts, feature_opts, &arg)) {
+		case O_SB_CKSUM:
+			cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
+			break;
+		default:
+			erofs_err("incorrect suboption");
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static int parse_extended_opts(const char *opts)
 {
 #define MATCH_EXTENTED_OPT(opt, token, keylen) \
@@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 {
 	int opt, i;
 
-	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
+	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
+	while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
 		switch (opt) {
 		case 'z':
 			if (!optarg) {
@@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 				return opt;
 			break;
 
+		case 'O':
+			opt = parse_feature_subopts(optarg);
+			if (opt)
+				return opt;
+			break;
+
 		default: /* '?' */
 			return -EINVAL;
 		}
@@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 	return 0;
 }
 
+u32 erofs_superblock_checksum(struct erofs_super_block *sb)
+{
+	u32 crc;
+	crc = crc32c(~0, (const unsigned char *)sb,
+		    sizeof(struct erofs_super_block));
+	erofs_dump("superblock checksum: 0x%x\n", crc);
+	return crc;
+}
+
 int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 				  erofs_nid_t root_nid)
 {
@@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 		.meta_blkaddr  = sbi.meta_blkaddr,
 		.xattr_blkaddr = 0,
 		.requirements = cpu_to_le32(sbi.requirements),
+		.features = cpu_to_le32(cfg.c_feature_flags),
 	};
 	const unsigned int sb_blksize =
 		round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
@@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 	sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
 	sb.root_nid     = cpu_to_le16(root_nid);
 
+	if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
+		sb.checksum = 0;
+		u32 crc = erofs_superblock_checksum(&sb);
+		sb.checksum = cpu_to_le32(crc);
+	}
+
 	buf = calloc(sb_blksize, 1);
 	if (!buf) {
 		erofs_err("Failed to allocate memory for sb: %s",
-- 
2.9.3

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik,

On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> Adding code for superblock checksum calculation.
> 
> incorporated the changes suggested in previous patch.
> 
> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>

Thanks for your v2 patch.

Actually, I have some concern about the length of checksum,
sizeof(struct erofs_super_block) could be changed in the
later version, it's bad for EROFS future scalablity.

And I tend not to add another on-disk field to record
the size of erofs_super_block as well, because the old
Linux kernel cannot handle more about the new size,
so it has little use except for checksum calculation.

Few hours ago, I discussed with Chao about this concern,
I think this feature can be changed to do multiple-block
checksum at the mount time, e.g:
 - for small images, we can check the whole image once
   at the mount time;
 - for the large image, we can check the superblock
   at the mount time, the rest can be handled by
   block-based verification layer.

But we agreed that don't add this for this round
since it's quite a new feature.

All in all, it's a new feature since we are addressing moving
out of staging for this round. I tend to postpone this feature
for now. I understand that you are very interested in EROFS.
Considering EROFS current staging status, it's not such a place
to add new features at all! I have marked your patch down and
I will work with you later. Hope to get your understanding...

Thanks,
Gao Xiang

> ---
>  include/erofs/config.h |  1 +
>  include/erofs_fs.h     | 10 ++++++++
>  mkfs/main.c            | 64 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/include/erofs/config.h b/include/erofs/config.h
> index 05fe6b2..40cd466 100644
> --- a/include/erofs/config.h
> +++ b/include/erofs/config.h
> @@ -22,6 +22,7 @@ struct erofs_configure {
>  	char *c_src_path;
>  	char *c_compr_alg_master;
>  	int c_compr_level_master;
> +	int c_feature_flags;
>  };
>  
>  extern struct erofs_configure cfg;
> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> index 601b477..9ac2635 100644
> --- a/include/erofs_fs.h
> +++ b/include/erofs_fs.h
> @@ -20,6 +20,16 @@
>  #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
>  #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
>  
> +/*
> + * feature definations.
> + */
> +#define EROFS_DEFAULT_FEATURES		EROFS_FEATURE_SB_CHKSUM
> +#define EROFS_FEATURE_SB_CHKSUM		0x0001
> +
> +
> +#define EROFS_HAS_COMPAT_FEATURE(super,mask)	\
> +	( le32_to_cpu((super)->features) & (mask) )
> +
>  struct erofs_super_block {
>  /*  0 */__le32 magic;           /* in the little endian */
>  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> diff --git a/mkfs/main.c b/mkfs/main.c
> index f127fe1..355fd2c 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -31,6 +31,45 @@ static void usage(void)
>  	fprintf(stderr, " -EX[,...] X=extended options\n");
>  }
>  
> +#define CRCPOLY	0x82F63B78
> +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
> +{
> +	int i;
> +	u32 crc = seed;
> +
> +	while (len--) {
> +		crc ^= *in++;
> +		for (i = 0; i < 8; i++) {
> +			crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> +		}
> +	}
> +	erofs_dump("calculated crc: 0x%x\n", crc);
> +	return crc;
> +}
> +
> +char *feature_opts[] = {
> +	"nosbcrc", NULL
> +};
> +#define O_SB_CKSUM	0
> +
> +static int parse_feature_subopts(char *opts)
> +{
> +	char *arg;
> +
> +	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> +	while (*opts != '\0') {
> +		switch(getsubopt(&opts, feature_opts, &arg)) {
> +		case O_SB_CKSUM:
> +			cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
> +			break;
> +		default:
> +			erofs_err("incorrect suboption");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int parse_extended_opts(const char *opts)
>  {
>  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  {
>  	int opt, i;
>  
> -	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> +	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> +	while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
>  		switch (opt) {
>  		case 'z':
>  			if (!optarg) {
> @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  				return opt;
>  			break;
>  
> +		case 'O':
> +			opt = parse_feature_subopts(optarg);
> +			if (opt)
> +				return opt;
> +			break;
> +
>  		default: /* '?' */
>  			return -EINVAL;
>  		}
> @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  	return 0;
>  }
>  
> +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> +{
> +	u32 crc;
> +	crc = crc32c(~0, (const unsigned char *)sb,
> +		    sizeof(struct erofs_super_block));
> +	erofs_dump("superblock checksum: 0x%x\n", crc);
> +	return crc;
> +}
> +
>  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  				  erofs_nid_t root_nid)
>  {
> @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  		.meta_blkaddr  = sbi.meta_blkaddr,
>  		.xattr_blkaddr = 0,
>  		.requirements = cpu_to_le32(sbi.requirements),
> +		.features = cpu_to_le32(cfg.c_feature_flags),
>  	};
>  	const unsigned int sb_blksize =
>  		round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  	sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
>  	sb.root_nid     = cpu_to_le16(root_nid);
>  
> +	if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> +		sb.checksum = 0;
> +		u32 crc = erofs_superblock_checksum(&sb);
> +		sb.checksum = cpu_to_le32(crc);
> +	}
> +
>  	buf = calloc(sb_blksize, 1);
>  	if (!buf) {
>  		erofs_err("Failed to allocate memory for sb: %s",
> -- 
> 2.9.3
> 

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

On Sat, Aug 24, 2019 at 10:00:25PM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Pratik,
> 
> On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > Adding code for superblock checksum calculation.
> > 
> > incorporated the changes suggested in previous patch.
> > 
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> 
> Thanks for your v2 patch.
> 
> Actually, I have some concern about the length of checksum,
> sizeof(struct erofs_super_block) could be changed in the
> later version, it's bad for EROFS future scalablity.
> 
> And I tend not to add another on-disk field to record
> the size of erofs_super_block as well, because the old
> Linux kernel cannot handle more about the new size,
> so it has little use except for checksum calculation.
> 
> Few hours ago, I discussed with Chao about this concern,
> I think this feature can be changed to do multiple-block
> checksum at the mount time, e.g:
>  - for small images, we can check the whole image once
>    at the mount time;
>  - for the large image, we can check the superblock
>    at the mount time, the rest can be handled by
>    block-based verification layer.
> 
> But we agreed that don't add this for this round
> since it's quite a new feature.
> 
> All in all, it's a new feature since we are addressing moving
> out of staging for this round. I tend to postpone this feature
> for now. I understand that you are very interested in EROFS.
> Considering EROFS current staging status, it's not such a place
> to add new features at all! I have marked your patch down and
                            ^ it should be a stop sign (.)
 
Sorry... that's my keyboard fault...

Let's postpone all new features and address more about moving out
EROFS. Only in this way, EROFS could have good future or it would
be dead. Hope to get your understanding...

Thanks,
Gao Xiang

> I will work with you later. Hope to get your understanding...
> 
> Thanks,
> Gao Xiang
> 
> > ---
> >  include/erofs/config.h |  1 +
> >  include/erofs_fs.h     | 10 ++++++++
> >  mkfs/main.c            | 64 +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 74 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > index 05fe6b2..40cd466 100644
> > --- a/include/erofs/config.h
> > +++ b/include/erofs/config.h
> > @@ -22,6 +22,7 @@ struct erofs_configure {
> >  	char *c_src_path;
> >  	char *c_compr_alg_master;
> >  	int c_compr_level_master;
> > +	int c_feature_flags;
> >  };
> >  
> >  extern struct erofs_configure cfg;
> > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > index 601b477..9ac2635 100644
> > --- a/include/erofs_fs.h
> > +++ b/include/erofs_fs.h
> > @@ -20,6 +20,16 @@
> >  #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
> >  #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
> >  
> > +/*
> > + * feature definations.
> > + */
> > +#define EROFS_DEFAULT_FEATURES		EROFS_FEATURE_SB_CHKSUM
> > +#define EROFS_FEATURE_SB_CHKSUM		0x0001
> > +
> > +
> > +#define EROFS_HAS_COMPAT_FEATURE(super,mask)	\
> > +	( le32_to_cpu((super)->features) & (mask) )
> > +
> >  struct erofs_super_block {
> >  /*  0 */__le32 magic;           /* in the little endian */
> >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index f127fe1..355fd2c 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -31,6 +31,45 @@ static void usage(void)
> >  	fprintf(stderr, " -EX[,...] X=extended options\n");
> >  }
> >  
> > +#define CRCPOLY	0x82F63B78
> > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
> > +{
> > +	int i;
> > +	u32 crc = seed;
> > +
> > +	while (len--) {
> > +		crc ^= *in++;
> > +		for (i = 0; i < 8; i++) {
> > +			crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > +		}
> > +	}
> > +	erofs_dump("calculated crc: 0x%x\n", crc);
> > +	return crc;
> > +}
> > +
> > +char *feature_opts[] = {
> > +	"nosbcrc", NULL
> > +};
> > +#define O_SB_CKSUM	0
> > +
> > +static int parse_feature_subopts(char *opts)
> > +{
> > +	char *arg;
> > +
> > +	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > +	while (*opts != '\0') {
> > +		switch(getsubopt(&opts, feature_opts, &arg)) {
> > +		case O_SB_CKSUM:
> > +			cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
> > +			break;
> > +		default:
> > +			erofs_err("incorrect suboption");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int parse_extended_opts(const char *opts)
> >  {
> >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> >  {
> >  	int opt, i;
> >  
> > -	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > +	cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > +	while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> >  		switch (opt) {
> >  		case 'z':
> >  			if (!optarg) {
> > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> >  				return opt;
> >  			break;
> >  
> > +		case 'O':
> > +			opt = parse_feature_subopts(optarg);
> > +			if (opt)
> > +				return opt;
> > +			break;
> > +
> >  		default: /* '?' */
> >  			return -EINVAL;
> >  		}
> > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > +{
> > +	u32 crc;
> > +	crc = crc32c(~0, (const unsigned char *)sb,
> > +		    sizeof(struct erofs_super_block));
> > +	erofs_dump("superblock checksum: 0x%x\n", crc);
> > +	return crc;
> > +}
> > +
> >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >  				  erofs_nid_t root_nid)
> >  {
> > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >  		.meta_blkaddr  = sbi.meta_blkaddr,
> >  		.xattr_blkaddr = 0,
> >  		.requirements = cpu_to_le32(sbi.requirements),
> > +		.features = cpu_to_le32(cfg.c_feature_flags),
> >  	};
> >  	const unsigned int sb_blksize =
> >  		round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >  	sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> >  	sb.root_nid     = cpu_to_le16(root_nid);
> >  
> > +	if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > +		sb.checksum = 0;
> > +		u32 crc = erofs_superblock_checksum(&sb);
> > +		sb.checksum = cpu_to_le32(crc);
> > +	}
> > +
> >  	buf = calloc(sb_blksize, 1);
> >  	if (!buf) {
> >  		erofs_err("Failed to allocate memory for sb: %s",
> > -- 
> > 2.9.3
> > 

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 7187 bytes --]

Hi Gao,

I completely understand your concern.You can drop this patch for now.
Once erofs makes it 'fs/' please do reconsider implementing it.

One more thing, can we still send non feature patches?

--Pratik


On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:

> Hi Pratik,
>
> On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > Adding code for superblock checksum calculation.
> >
> > incorporated the changes suggested in previous patch.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
>
> Thanks for your v2 patch.
>
> Actually, I have some concern about the length of checksum,
> sizeof(struct erofs_super_block) could be changed in the
> later version, it's bad for EROFS future scalablity.
>
> And I tend not to add another on-disk field to record
> the size of erofs_super_block as well, because the old
> Linux kernel cannot handle more about the new size,
> so it has little use except for checksum calculation.
>
> Few hours ago, I discussed with Chao about this concern,
> I think this feature can be changed to do multiple-block
> checksum at the mount time, e.g:
>  - for small images, we can check the whole image once
>    at the mount time;
>  - for the large image, we can check the superblock
>    at the mount time, the rest can be handled by
>    block-based verification layer.
>
> But we agreed that don't add this for this round
> since it's quite a new feature.
>
> All in all, it's a new feature since we are addressing moving
> out of staging for this round. I tend to postpone this feature
> for now. I understand that you are very interested in EROFS.
> Considering EROFS current staging status, it's not such a place
> to add new features at all! I have marked your patch down and
> I will work with you later. Hope to get your understanding...
>
> Thanks,
> Gao Xiang
>
> > ---
> >  include/erofs/config.h |  1 +
> >  include/erofs_fs.h     | 10 ++++++++
> >  mkfs/main.c            | 64
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > index 05fe6b2..40cd466 100644
> > --- a/include/erofs/config.h
> > +++ b/include/erofs/config.h
> > @@ -22,6 +22,7 @@ struct erofs_configure {
> >       char *c_src_path;
> >       char *c_compr_alg_master;
> >       int c_compr_level_master;
> > +     int c_feature_flags;
> >  };
> >
> >  extern struct erofs_configure cfg;
> > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > index 601b477..9ac2635 100644
> > --- a/include/erofs_fs.h
> > +++ b/include/erofs_fs.h
> > @@ -20,6 +20,16 @@
> >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> >  #define EROFS_ALL_REQUIREMENTS
>  EROFS_REQUIREMENT_LZ4_0PADDING
> >
> > +/*
> > + * feature definations.
> > + */
> > +#define EROFS_DEFAULT_FEATURES               EROFS_FEATURE_SB_CHKSUM
> > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > +
> > +
> > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > +     ( le32_to_cpu((super)->features) & (mask) )
> > +
> >  struct erofs_super_block {
> >  /*  0 */__le32 magic;           /* in the little endian */
> >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index f127fe1..355fd2c 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -31,6 +31,45 @@ static void usage(void)
> >       fprintf(stderr, " -EX[,...] X=extended options\n");
> >  }
> >
> > +#define CRCPOLY      0x82F63B78
> > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
> > +{
> > +     int i;
> > +     u32 crc = seed;
> > +
> > +     while (len--) {
> > +             crc ^= *in++;
> > +             for (i = 0; i < 8; i++) {
> > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > +             }
> > +     }
> > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > +     return crc;
> > +}
> > +
> > +char *feature_opts[] = {
> > +     "nosbcrc", NULL
> > +};
> > +#define O_SB_CKSUM   0
> > +
> > +static int parse_feature_subopts(char *opts)
> > +{
> > +     char *arg;
> > +
> > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > +     while (*opts != '\0') {
> > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > +             case O_SB_CKSUM:
> > +                     cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
> > +                     break;
> > +             default:
> > +                     erofs_err("incorrect suboption");
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int parse_extended_opts(const char *opts)
> >  {
> >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >  {
> >       int opt, i;
> >
> > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> >               switch (opt) {
> >               case 'z':
> >                       if (!optarg) {
> > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >                               return opt;
> >                       break;
> >
> > +             case 'O':
> > +                     opt = parse_feature_subopts(optarg);
> > +                     if (opt)
> > +                             return opt;
> > +                     break;
> > +
> >               default: /* '?' */
> >                       return -EINVAL;
> >               }
> > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >       return 0;
> >  }
> >
> > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > +{
> > +     u32 crc;
> > +     crc = crc32c(~0, (const unsigned char *)sb,
> > +                 sizeof(struct erofs_super_block));
> > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > +     return crc;
> > +}
> > +
> >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >                                 erofs_nid_t root_nid)
> >  {
> > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> >               .meta_blkaddr  = sbi.meta_blkaddr,
> >               .xattr_blkaddr = 0,
> >               .requirements = cpu_to_le32(sbi.requirements),
> > +             .features = cpu_to_le32(cfg.c_feature_flags),
> >       };
> >       const unsigned int sb_blksize =
> >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> >       sb.root_nid     = cpu_to_le16(root_nid);
> >
> > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > +             sb.checksum = 0;
> > +             u32 crc = erofs_superblock_checksum(&sb);
> > +             sb.checksum = cpu_to_le32(crc);
> > +     }
> > +
> >       buf = calloc(sb_blksize, 1);
> >       if (!buf) {
> >               erofs_err("Failed to allocate memory for sb: %s",
> > --
> > 2.9.3
> >
>

[-- Attachment #2: Type: text/html, Size: 9589 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik,

On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> Hi Gao,
> 
> I completely understand your concern.You can drop this patch for now.

Thank you.

> Once erofs makes it 'fs/' please do reconsider implementing it.
> 
> One more thing, can we still send non feature patches?

Yes, of course. All bugfix patches for erofs kernel and erofs-utils
are OK, if it's important, I will apply it for 5.4 round immediately.

Thanks,
Gao Xiang

> 
> --Pratik
> 
> 
> On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> 
> > Hi Pratik,
> >
> > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > Adding code for superblock checksum calculation.
> > >
> > > incorporated the changes suggested in previous patch.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> >
> > Thanks for your v2 patch.
> >
> > Actually, I have some concern about the length of checksum,
> > sizeof(struct erofs_super_block) could be changed in the
> > later version, it's bad for EROFS future scalablity.
> >
> > And I tend not to add another on-disk field to record
> > the size of erofs_super_block as well, because the old
> > Linux kernel cannot handle more about the new size,
> > so it has little use except for checksum calculation.
> >
> > Few hours ago, I discussed with Chao about this concern,
> > I think this feature can be changed to do multiple-block
> > checksum at the mount time, e.g:
> >  - for small images, we can check the whole image once
> >    at the mount time;
> >  - for the large image, we can check the superblock
> >    at the mount time, the rest can be handled by
> >    block-based verification layer.
> >
> > But we agreed that don't add this for this round
> > since it's quite a new feature.
> >
> > All in all, it's a new feature since we are addressing moving
> > out of staging for this round. I tend to postpone this feature
> > for now. I understand that you are very interested in EROFS.
> > Considering EROFS current staging status, it's not such a place
> > to add new features at all! I have marked your patch down and
> > I will work with you later. Hope to get your understanding...
> >
> > Thanks,
> > Gao Xiang
> >
> > > ---
> > >  include/erofs/config.h |  1 +
> > >  include/erofs_fs.h     | 10 ++++++++
> > >  mkfs/main.c            | 64
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > index 05fe6b2..40cd466 100644
> > > --- a/include/erofs/config.h
> > > +++ b/include/erofs/config.h
> > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > >       char *c_src_path;
> > >       char *c_compr_alg_master;
> > >       int c_compr_level_master;
> > > +     int c_feature_flags;
> > >  };
> > >
> > >  extern struct erofs_configure cfg;
> > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > index 601b477..9ac2635 100644
> > > --- a/include/erofs_fs.h
> > > +++ b/include/erofs_fs.h
> > > @@ -20,6 +20,16 @@
> > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > >  #define EROFS_ALL_REQUIREMENTS
> >  EROFS_REQUIREMENT_LZ4_0PADDING
> > >
> > > +/*
> > > + * feature definations.
> > > + */
> > > +#define EROFS_DEFAULT_FEATURES               EROFS_FEATURE_SB_CHKSUM
> > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > +
> > > +
> > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > +
> > >  struct erofs_super_block {
> > >  /*  0 */__le32 magic;           /* in the little endian */
> > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > index f127fe1..355fd2c 100644
> > > --- a/mkfs/main.c
> > > +++ b/mkfs/main.c
> > > @@ -31,6 +31,45 @@ static void usage(void)
> > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > >  }
> > >
> > > +#define CRCPOLY      0x82F63B78
> > > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
> > > +{
> > > +     int i;
> > > +     u32 crc = seed;
> > > +
> > > +     while (len--) {
> > > +             crc ^= *in++;
> > > +             for (i = 0; i < 8; i++) {
> > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > > +             }
> > > +     }
> > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > +     return crc;
> > > +}
> > > +
> > > +char *feature_opts[] = {
> > > +     "nosbcrc", NULL
> > > +};
> > > +#define O_SB_CKSUM   0
> > > +
> > > +static int parse_feature_subopts(char *opts)
> > > +{
> > > +     char *arg;
> > > +
> > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > +     while (*opts != '\0') {
> > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > +             case O_SB_CKSUM:
> > > +                     cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
> > > +                     break;
> > > +             default:
> > > +                     erofs_err("incorrect suboption");
> > > +                     return -EINVAL;
> > > +             }
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > >  static int parse_extended_opts(const char *opts)
> > >  {
> > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >  {
> > >       int opt, i;
> > >
> > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > >               switch (opt) {
> > >               case 'z':
> > >                       if (!optarg) {
> > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >                               return opt;
> > >                       break;
> > >
> > > +             case 'O':
> > > +                     opt = parse_feature_subopts(optarg);
> > > +                     if (opt)
> > > +                             return opt;
> > > +                     break;
> > > +
> > >               default: /* '?' */
> > >                       return -EINVAL;
> > >               }
> > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >       return 0;
> > >  }
> > >
> > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > +{
> > > +     u32 crc;
> > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > +                 sizeof(struct erofs_super_block));
> > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > +     return crc;
> > > +}
> > > +
> > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > >                                 erofs_nid_t root_nid)
> > >  {
> > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > >               .xattr_blkaddr = 0,
> > >               .requirements = cpu_to_le32(sbi.requirements),
> > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > >       };
> > >       const unsigned int sb_blksize =
> > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > >       sb.root_nid     = cpu_to_le16(root_nid);
> > >
> > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > > +             sb.checksum = 0;
> > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > +             sb.checksum = cpu_to_le32(crc);
> > > +     }
> > > +
> > >       buf = calloc(sb_blksize, 1);
> > >       if (!buf) {
> > >               erofs_err("Failed to allocate memory for sb: %s",
> > > --
> > > 2.9.3
> > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik,

On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> Hi Gao,
> 
> I completely understand your concern.You can drop this patch for now.
> Once erofs makes it 'fs/' please do reconsider implementing it.

I think we can work on this pending feature for v5.5 now.
My idea is to add an extra field in erofs_super_block to
indicate the number of blocks (4k) for checking.

So for small images, this feature can checksum the whole image at mount time,
and for large images, this feature can be used to checksum the super block only
(and then use verficiation subsystem to verify on-the-fly in the future...)

The following workflow is for erofs-utils, I think it's
 1) erofs_mkfs_update_super_block with checksum = 0
 2) erofs_bflush(NULL)
 3) reread the corresponding blocks and calculate checksum;
 4) write checksum to erofs_super_block;

Does it sound reasonable? and do you still have interest and
time for this? Looking forword to your reply...

Thanks,
Gao Xiang

> 
> One more thing, can we still send non feature patches?
> 
> --Pratik
> 
> 
> On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> 
> > Hi Pratik,
> >
> > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > Adding code for superblock checksum calculation.
> > >
> > > incorporated the changes suggested in previous patch.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> >
> > Thanks for your v2 patch.
> >
> > Actually, I have some concern about the length of checksum,
> > sizeof(struct erofs_super_block) could be changed in the
> > later version, it's bad for EROFS future scalablity.
> >
> > And I tend not to add another on-disk field to record
> > the size of erofs_super_block as well, because the old
> > Linux kernel cannot handle more about the new size,
> > so it has little use except for checksum calculation.
> >
> > Few hours ago, I discussed with Chao about this concern,
> > I think this feature can be changed to do multiple-block
> > checksum at the mount time, e.g:
> >  - for small images, we can check the whole image once
> >    at the mount time;
> >  - for the large image, we can check the superblock
> >    at the mount time, the rest can be handled by
> >    block-based verification layer.
> >
> > But we agreed that don't add this for this round
> > since it's quite a new feature.
> >
> > All in all, it's a new feature since we are addressing moving
> > out of staging for this round. I tend to postpone this feature
> > for now. I understand that you are very interested in EROFS.
> > Considering EROFS current staging status, it's not such a place
> > to add new features at all! I have marked your patch down and
> > I will work with you later. Hope to get your understanding...
> >
> > Thanks,
> > Gao Xiang
> >
> > > ---
> > >  include/erofs/config.h |  1 +
> > >  include/erofs_fs.h     | 10 ++++++++
> > >  mkfs/main.c            | 64
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > index 05fe6b2..40cd466 100644
> > > --- a/include/erofs/config.h
> > > +++ b/include/erofs/config.h
> > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > >       char *c_src_path;
> > >       char *c_compr_alg_master;
> > >       int c_compr_level_master;
> > > +     int c_feature_flags;
> > >  };
> > >
> > >  extern struct erofs_configure cfg;
> > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > index 601b477..9ac2635 100644
> > > --- a/include/erofs_fs.h
> > > +++ b/include/erofs_fs.h
> > > @@ -20,6 +20,16 @@
> > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > >  #define EROFS_ALL_REQUIREMENTS
> >  EROFS_REQUIREMENT_LZ4_0PADDING
> > >
> > > +/*
> > > + * feature definations.
> > > + */
> > > +#define EROFS_DEFAULT_FEATURES               EROFS_FEATURE_SB_CHKSUM
> > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > +
> > > +
> > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > +
> > >  struct erofs_super_block {
> > >  /*  0 */__le32 magic;           /* in the little endian */
> > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > index f127fe1..355fd2c 100644
> > > --- a/mkfs/main.c
> > > +++ b/mkfs/main.c
> > > @@ -31,6 +31,45 @@ static void usage(void)
> > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > >  }
> > >
> > > +#define CRCPOLY      0x82F63B78
> > > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t len)
> > > +{
> > > +     int i;
> > > +     u32 crc = seed;
> > > +
> > > +     while (len--) {
> > > +             crc ^= *in++;
> > > +             for (i = 0; i < 8; i++) {
> > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > > +             }
> > > +     }
> > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > +     return crc;
> > > +}
> > > +
> > > +char *feature_opts[] = {
> > > +     "nosbcrc", NULL
> > > +};
> > > +#define O_SB_CKSUM   0
> > > +
> > > +static int parse_feature_subopts(char *opts)
> > > +{
> > > +     char *arg;
> > > +
> > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > +     while (*opts != '\0') {
> > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > +             case O_SB_CKSUM:
> > > +                     cfg.c_feature_flags |= (~EROFS_FEATURE_SB_CHKSUM);
> > > +                     break;
> > > +             default:
> > > +                     erofs_err("incorrect suboption");
> > > +                     return -EINVAL;
> > > +             }
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > >  static int parse_extended_opts(const char *opts)
> > >  {
> > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >  {
> > >       int opt, i;
> > >
> > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > >               switch (opt) {
> > >               case 'z':
> > >                       if (!optarg) {
> > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >                               return opt;
> > >                       break;
> > >
> > > +             case 'O':
> > > +                     opt = parse_feature_subopts(optarg);
> > > +                     if (opt)
> > > +                             return opt;
> > > +                     break;
> > > +
> > >               default: /* '?' */
> > >                       return -EINVAL;
> > >               }
> > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >       return 0;
> > >  }
> > >
> > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > +{
> > > +     u32 crc;
> > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > +                 sizeof(struct erofs_super_block));
> > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > +     return crc;
> > > +}
> > > +
> > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > >                                 erofs_nid_t root_nid)
> > >  {
> > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > >               .xattr_blkaddr = 0,
> > >               .requirements = cpu_to_le32(sbi.requirements),
> > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > >       };
> > >       const unsigned int sb_blksize =
> > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > >       sb.root_nid     = cpu_to_le16(root_nid);
> > >
> > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > > +             sb.checksum = 0;
> > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > +             sb.checksum = cpu_to_le32(crc);
> > > +     }
> > > +
> > >       buf = calloc(sb_blksize, 1);
> > >       if (!buf) {
> > >               erofs_err("Failed to allocate memory for sb: %s",
> > > --
> > > 2.9.3
> > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 9145 bytes --]

Hi Gao,

Yes I can work on it. Sorry I missed this mail. I think your approach is
good. Let me go through it one more time and reply back.

--Pratik

On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:

> Hi Pratik,
>
> On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > Hi Gao,
> >
> > I completely understand your concern.You can drop this patch for now.
> > Once erofs makes it 'fs/' please do reconsider implementing it.
>
> I think we can work on this pending feature for v5.5 now.
> My idea is to add an extra field in erofs_super_block to
> indicate the number of blocks (4k) for checking.
>
> So for small images, this feature can checksum the whole image at mount
> time,
> and for large images, this feature can be used to checksum the super block
> only
> (and then use verficiation subsystem to verify on-the-fly in the future...)
>
> The following workflow is for erofs-utils, I think it's
>  1) erofs_mkfs_update_super_block with checksum = 0
>  2) erofs_bflush(NULL)
>  3) reread the corresponding blocks and calculate checksum;
>  4) write checksum to erofs_super_block;
>
> Does it sound reasonable? and do you still have interest and
> time for this? Looking forword to your reply...
>
> Thanks,
> Gao Xiang
>
> >
> > One more thing, can we still send non feature patches?
> >
> > --Pratik
> >
> >
> > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> >
> > > Hi Pratik,
> > >
> > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > Adding code for superblock checksum calculation.
> > > >
> > > > incorporated the changes suggested in previous patch.
> > > >
> > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > >
> > > Thanks for your v2 patch.
> > >
> > > Actually, I have some concern about the length of checksum,
> > > sizeof(struct erofs_super_block) could be changed in the
> > > later version, it's bad for EROFS future scalablity.
> > >
> > > And I tend not to add another on-disk field to record
> > > the size of erofs_super_block as well, because the old
> > > Linux kernel cannot handle more about the new size,
> > > so it has little use except for checksum calculation.
> > >
> > > Few hours ago, I discussed with Chao about this concern,
> > > I think this feature can be changed to do multiple-block
> > > checksum at the mount time, e.g:
> > >  - for small images, we can check the whole image once
> > >    at the mount time;
> > >  - for the large image, we can check the superblock
> > >    at the mount time, the rest can be handled by
> > >    block-based verification layer.
> > >
> > > But we agreed that don't add this for this round
> > > since it's quite a new feature.
> > >
> > > All in all, it's a new feature since we are addressing moving
> > > out of staging for this round. I tend to postpone this feature
> > > for now. I understand that you are very interested in EROFS.
> > > Considering EROFS current staging status, it's not such a place
> > > to add new features at all! I have marked your patch down and
> > > I will work with you later. Hope to get your understanding...
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > > > ---
> > > >  include/erofs/config.h |  1 +
> > > >  include/erofs_fs.h     | 10 ++++++++
> > > >  mkfs/main.c            | 64
> > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > index 05fe6b2..40cd466 100644
> > > > --- a/include/erofs/config.h
> > > > +++ b/include/erofs/config.h
> > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > >       char *c_src_path;
> > > >       char *c_compr_alg_master;
> > > >       int c_compr_level_master;
> > > > +     int c_feature_flags;
> > > >  };
> > > >
> > > >  extern struct erofs_configure cfg;
> > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > index 601b477..9ac2635 100644
> > > > --- a/include/erofs_fs.h
> > > > +++ b/include/erofs_fs.h
> > > > @@ -20,6 +20,16 @@
> > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > >  #define EROFS_ALL_REQUIREMENTS
> > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > >
> > > > +/*
> > > > + * feature definations.
> > > > + */
> > > > +#define EROFS_DEFAULT_FEATURES               EROFS_FEATURE_SB_CHKSUM
> > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > +
> > > > +
> > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > +
> > > >  struct erofs_super_block {
> > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > index f127fe1..355fd2c 100644
> > > > --- a/mkfs/main.c
> > > > +++ b/mkfs/main.c
> > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > >  }
> > > >
> > > > +#define CRCPOLY      0x82F63B78
> > > > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t
> len)
> > > > +{
> > > > +     int i;
> > > > +     u32 crc = seed;
> > > > +
> > > > +     while (len--) {
> > > > +             crc ^= *in++;
> > > > +             for (i = 0; i < 8; i++) {
> > > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > > > +             }
> > > > +     }
> > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > +     return crc;
> > > > +}
> > > > +
> > > > +char *feature_opts[] = {
> > > > +     "nosbcrc", NULL
> > > > +};
> > > > +#define O_SB_CKSUM   0
> > > > +
> > > > +static int parse_feature_subopts(char *opts)
> > > > +{
> > > > +     char *arg;
> > > > +
> > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > +     while (*opts != '\0') {
> > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > +             case O_SB_CKSUM:
> > > > +                     cfg.c_feature_flags |=
> (~EROFS_FEATURE_SB_CHKSUM);
> > > > +                     break;
> > > > +             default:
> > > > +                     erofs_err("incorrect suboption");
> > > > +                     return -EINVAL;
> > > > +             }
> > > > +     }
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int parse_extended_opts(const char *opts)
> > > >  {
> > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char
> > > *argv[])
> > > >  {
> > > >       int opt, i;
> > > >
> > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > >               switch (opt) {
> > > >               case 'z':
> > > >                       if (!optarg) {
> > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > > *argv[])
> > > >                               return opt;
> > > >                       break;
> > > >
> > > > +             case 'O':
> > > > +                     opt = parse_feature_subopts(optarg);
> > > > +                     if (opt)
> > > > +                             return opt;
> > > > +                     break;
> > > > +
> > > >               default: /* '?' */
> > > >                       return -EINVAL;
> > > >               }
> > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char
> > > *argv[])
> > > >       return 0;
> > > >  }
> > > >
> > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > +{
> > > > +     u32 crc;
> > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > +                 sizeof(struct erofs_super_block));
> > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > +     return crc;
> > > > +}
> > > > +
> > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > > >                                 erofs_nid_t root_nid)
> > > >  {
> > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > erofs_buffer_head *bh,
> > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > >               .xattr_blkaddr = 0,
> > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > >       };
> > > >       const unsigned int sb_blksize =
> > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > erofs_buffer_head *bh,
> > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > >
> > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > > > +             sb.checksum = 0;
> > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > +             sb.checksum = cpu_to_le32(crc);
> > > > +     }
> > > > +
> > > >       buf = calloc(sb_blksize, 1);
> > > >       if (!buf) {
> > > >               erofs_err("Failed to allocate memory for sb: %s",
> > > > --
> > > > 2.9.3
> > > >
> > >
>

[-- Attachment #2: Type: text/html, Size: 12872 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang]

Hi Pratik,

On Wed, Oct 09, 2019 at 11:59:01AM +0530, Pratik Shinde wrote:
> Hi Gao,
> 
> Yes I can work on it. Sorry I missed this mail. I think your approach is
> good. Let me go through it one more time and reply back.

Thanks for your reply and interest :)
I think we can complete all pending features together
if you have some time on this stuff. (fsdebug utility is
on the way as well...)

BTW, I'm now investigating new high CR algorithm (very likely
XZ) as well, it will be likely a RFC version in this round for
wider scenarios and later decompression subsystem.

Preliminary TODO lists will be discussed in this year China
Linux Storage & Filesystem workshop (next week) and will be
posted to mailing lists for further wider discussion (if more
folks have interest in developing it) as well. :)

Thanks,
Gao Xiang

> 
> --Pratik
> 
> On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> 
> > Hi Pratik,
> >
> > On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > > Hi Gao,
> > >
> > > I completely understand your concern.You can drop this patch for now.
> > > Once erofs makes it 'fs/' please do reconsider implementing it.
> >
> > I think we can work on this pending feature for v5.5 now.
> > My idea is to add an extra field in erofs_super_block to
> > indicate the number of blocks (4k) for checking.
> >
> > So for small images, this feature can checksum the whole image at mount
> > time,
> > and for large images, this feature can be used to checksum the super block
> > only
> > (and then use verficiation subsystem to verify on-the-fly in the future...)
> >
> > The following workflow is for erofs-utils, I think it's
> >  1) erofs_mkfs_update_super_block with checksum = 0
> >  2) erofs_bflush(NULL)
> >  3) reread the corresponding blocks and calculate checksum;
> >  4) write checksum to erofs_super_block;
> >
> > Does it sound reasonable? and do you still have interest and
> > time for this? Looking forword to your reply...
> >
> > Thanks,
> > Gao Xiang
> >
> > >
> > > One more thing, can we still send non feature patches?
> > >
> > > --Pratik
> > >
> > >
> > > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> > >
> > > > Hi Pratik,
> > > >
> > > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > > Adding code for superblock checksum calculation.
> > > > >
> > > > > incorporated the changes suggested in previous patch.
> > > > >
> > > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > >
> > > > Thanks for your v2 patch.
> > > >
> > > > Actually, I have some concern about the length of checksum,
> > > > sizeof(struct erofs_super_block) could be changed in the
> > > > later version, it's bad for EROFS future scalablity.
> > > >
> > > > And I tend not to add another on-disk field to record
> > > > the size of erofs_super_block as well, because the old
> > > > Linux kernel cannot handle more about the new size,
> > > > so it has little use except for checksum calculation.
> > > >
> > > > Few hours ago, I discussed with Chao about this concern,
> > > > I think this feature can be changed to do multiple-block
> > > > checksum at the mount time, e.g:
> > > >  - for small images, we can check the whole image once
> > > >    at the mount time;
> > > >  - for the large image, we can check the superblock
> > > >    at the mount time, the rest can be handled by
> > > >    block-based verification layer.
> > > >
> > > > But we agreed that don't add this for this round
> > > > since it's quite a new feature.
> > > >
> > > > All in all, it's a new feature since we are addressing moving
> > > > out of staging for this round. I tend to postpone this feature
> > > > for now. I understand that you are very interested in EROFS.
> > > > Considering EROFS current staging status, it's not such a place
> > > > to add new features at all! I have marked your patch down and
> > > > I will work with you later. Hope to get your understanding...
> > > >
> > > > Thanks,
> > > > Gao Xiang
> > > >
> > > > > ---
> > > > >  include/erofs/config.h |  1 +
> > > > >  include/erofs_fs.h     | 10 ++++++++
> > > > >  mkfs/main.c            | 64
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > > index 05fe6b2..40cd466 100644
> > > > > --- a/include/erofs/config.h
> > > > > +++ b/include/erofs/config.h
> > > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > > >       char *c_src_path;
> > > > >       char *c_compr_alg_master;
> > > > >       int c_compr_level_master;
> > > > > +     int c_feature_flags;
> > > > >  };
> > > > >
> > > > >  extern struct erofs_configure cfg;
> > > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > > index 601b477..9ac2635 100644
> > > > > --- a/include/erofs_fs.h
> > > > > +++ b/include/erofs_fs.h
> > > > > @@ -20,6 +20,16 @@
> > > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > > >  #define EROFS_ALL_REQUIREMENTS
> > > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > > >
> > > > > +/*
> > > > > + * feature definations.
> > > > > + */
> > > > > +#define EROFS_DEFAULT_FEATURES               EROFS_FEATURE_SB_CHKSUM
> > > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > > +
> > > > > +
> > > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > > +
> > > > >  struct erofs_super_block {
> > > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > > index f127fe1..355fd2c 100644
> > > > > --- a/mkfs/main.c
> > > > > +++ b/mkfs/main.c
> > > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > > >  }
> > > > >
> > > > > +#define CRCPOLY      0x82F63B78
> > > > > +static inline u32 crc32c(u32 seed, unsigned char const *in, size_t
> > len)
> > > > > +{
> > > > > +     int i;
> > > > > +     u32 crc = seed;
> > > > > +
> > > > > +     while (len--) {
> > > > > +             crc ^= *in++;
> > > > > +             for (i = 0; i < 8; i++) {
> > > > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY : 0);
> > > > > +             }
> > > > > +     }
> > > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > > +     return crc;
> > > > > +}
> > > > > +
> > > > > +char *feature_opts[] = {
> > > > > +     "nosbcrc", NULL
> > > > > +};
> > > > > +#define O_SB_CKSUM   0
> > > > > +
> > > > > +static int parse_feature_subopts(char *opts)
> > > > > +{
> > > > > +     char *arg;
> > > > > +
> > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > +     while (*opts != '\0') {
> > > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > > +             case O_SB_CKSUM:
> > > > > +                     cfg.c_feature_flags |=
> > (~EROFS_FEATURE_SB_CHKSUM);
> > > > > +                     break;
> > > > > +             default:
> > > > > +                     erofs_err("incorrect suboption");
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > > +     }
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  static int parse_extended_opts(const char *opts)
> > > > >  {
> > > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc, char
> > > > *argv[])
> > > > >  {
> > > > >       int opt, i;
> > > > >
> > > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > > >               switch (opt) {
> > > > >               case 'z':
> > > > >                       if (!optarg) {
> > > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > > > *argv[])
> > > > >                               return opt;
> > > > >                       break;
> > > > >
> > > > > +             case 'O':
> > > > > +                     opt = parse_feature_subopts(optarg);
> > > > > +                     if (opt)
> > > > > +                             return opt;
> > > > > +                     break;
> > > > > +
> > > > >               default: /* '?' */
> > > > >                       return -EINVAL;
> > > > >               }
> > > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc, char
> > > > *argv[])
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > > +{
> > > > > +     u32 crc;
> > > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > > +                 sizeof(struct erofs_super_block));
> > > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > > +     return crc;
> > > > > +}
> > > > > +
> > > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > > > >                                 erofs_nid_t root_nid)
> > > > >  {
> > > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > > erofs_buffer_head *bh,
> > > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > > >               .xattr_blkaddr = 0,
> > > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > > >       };
> > > > >       const unsigned int sb_blksize =
> > > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > > erofs_buffer_head *bh,
> > > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > > >
> > > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > > > > +             sb.checksum = 0;
> > > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > > +             sb.checksum = cpu_to_le32(crc);
> > > > > +     }
> > > > > +
> > > > >       buf = calloc(sb_blksize, 1);
> > > > >       if (!buf) {
> > > > >               erofs_err("Failed to allocate memory for sb: %s",
> > > > > --
> > > > > 2.9.3
> > > > >
> > > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 11063 bytes --]

Hello Gao,

Yes I would like to work on pending features.
Also please let us know the new development items.

--Pratik

On Wed, 9 Oct, 2019, 12:24 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:

> Hi Pratik,
>
> On Wed, Oct 09, 2019 at 11:59:01AM +0530, Pratik Shinde wrote:
> > Hi Gao,
> >
> > Yes I can work on it. Sorry I missed this mail. I think your approach is
> > good. Let me go through it one more time and reply back.
>
> Thanks for your reply and interest :)
> I think we can complete all pending features together
> if you have some time on this stuff. (fsdebug utility is
> on the way as well...)
>
> BTW, I'm now investigating new high CR algorithm (very likely
> XZ) as well, it will be likely a RFC version in this round for
> wider scenarios and later decompression subsystem.
>
> Preliminary TODO lists will be discussed in this year China
> Linux Storage & Filesystem workshop (next week) and will be
> posted to mailing lists for further wider discussion (if more
> folks have interest in developing it) as well. :)
>
> Thanks,
> Gao Xiang
>
> >
> > --Pratik
> >
> > On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> >
> > > Hi Pratik,
> > >
> > > On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > > > Hi Gao,
> > > >
> > > > I completely understand your concern.You can drop this patch for now.
> > > > Once erofs makes it 'fs/' please do reconsider implementing it.
> > >
> > > I think we can work on this pending feature for v5.5 now.
> > > My idea is to add an extra field in erofs_super_block to
> > > indicate the number of blocks (4k) for checking.
> > >
> > > So for small images, this feature can checksum the whole image at mount
> > > time,
> > > and for large images, this feature can be used to checksum the super
> block
> > > only
> > > (and then use verficiation subsystem to verify on-the-fly in the
> future...)
> > >
> > > The following workflow is for erofs-utils, I think it's
> > >  1) erofs_mkfs_update_super_block with checksum = 0
> > >  2) erofs_bflush(NULL)
> > >  3) reread the corresponding blocks and calculate checksum;
> > >  4) write checksum to erofs_super_block;
> > >
> > > Does it sound reasonable? and do you still have interest and
> > > time for this? Looking forword to your reply...
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > > >
> > > > One more thing, can we still send non feature patches?
> > > >
> > > > --Pratik
> > > >
> > > >
> > > > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> > > >
> > > > > Hi Pratik,
> > > > >
> > > > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > > > Adding code for superblock checksum calculation.
> > > > > >
> > > > > > incorporated the changes suggested in previous patch.
> > > > > >
> > > > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > > >
> > > > > Thanks for your v2 patch.
> > > > >
> > > > > Actually, I have some concern about the length of checksum,
> > > > > sizeof(struct erofs_super_block) could be changed in the
> > > > > later version, it's bad for EROFS future scalablity.
> > > > >
> > > > > And I tend not to add another on-disk field to record
> > > > > the size of erofs_super_block as well, because the old
> > > > > Linux kernel cannot handle more about the new size,
> > > > > so it has little use except for checksum calculation.
> > > > >
> > > > > Few hours ago, I discussed with Chao about this concern,
> > > > > I think this feature can be changed to do multiple-block
> > > > > checksum at the mount time, e.g:
> > > > >  - for small images, we can check the whole image once
> > > > >    at the mount time;
> > > > >  - for the large image, we can check the superblock
> > > > >    at the mount time, the rest can be handled by
> > > > >    block-based verification layer.
> > > > >
> > > > > But we agreed that don't add this for this round
> > > > > since it's quite a new feature.
> > > > >
> > > > > All in all, it's a new feature since we are addressing moving
> > > > > out of staging for this round. I tend to postpone this feature
> > > > > for now. I understand that you are very interested in EROFS.
> > > > > Considering EROFS current staging status, it's not such a place
> > > > > to add new features at all! I have marked your patch down and
> > > > > I will work with you later. Hope to get your understanding...
> > > > >
> > > > > Thanks,
> > > > > Gao Xiang
> > > > >
> > > > > > ---
> > > > > >  include/erofs/config.h |  1 +
> > > > > >  include/erofs_fs.h     | 10 ++++++++
> > > > > >  mkfs/main.c            | 64
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > > > index 05fe6b2..40cd466 100644
> > > > > > --- a/include/erofs/config.h
> > > > > > +++ b/include/erofs/config.h
> > > > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > > > >       char *c_src_path;
> > > > > >       char *c_compr_alg_master;
> > > > > >       int c_compr_level_master;
> > > > > > +     int c_feature_flags;
> > > > > >  };
> > > > > >
> > > > > >  extern struct erofs_configure cfg;
> > > > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > > > index 601b477..9ac2635 100644
> > > > > > --- a/include/erofs_fs.h
> > > > > > +++ b/include/erofs_fs.h
> > > > > > @@ -20,6 +20,16 @@
> > > > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > > > >  #define EROFS_ALL_REQUIREMENTS
> > > > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > > > >
> > > > > > +/*
> > > > > > + * feature definations.
> > > > > > + */
> > > > > > +#define EROFS_DEFAULT_FEATURES
>  EROFS_FEATURE_SB_CHKSUM
> > > > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > > > +
> > > > > > +
> > > > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > > > +
> > > > > >  struct erofs_super_block {
> > > > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > > > index f127fe1..355fd2c 100644
> > > > > > --- a/mkfs/main.c
> > > > > > +++ b/mkfs/main.c
> > > > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > > > >  }
> > > > > >
> > > > > > +#define CRCPOLY      0x82F63B78
> > > > > > +static inline u32 crc32c(u32 seed, unsigned char const *in,
> size_t
> > > len)
> > > > > > +{
> > > > > > +     int i;
> > > > > > +     u32 crc = seed;
> > > > > > +
> > > > > > +     while (len--) {
> > > > > > +             crc ^= *in++;
> > > > > > +             for (i = 0; i < 8; i++) {
> > > > > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY :
> 0);
> > > > > > +             }
> > > > > > +     }
> > > > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > > > +     return crc;
> > > > > > +}
> > > > > > +
> > > > > > +char *feature_opts[] = {
> > > > > > +     "nosbcrc", NULL
> > > > > > +};
> > > > > > +#define O_SB_CKSUM   0
> > > > > > +
> > > > > > +static int parse_feature_subopts(char *opts)
> > > > > > +{
> > > > > > +     char *arg;
> > > > > > +
> > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > +     while (*opts != '\0') {
> > > > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > > > +             case O_SB_CKSUM:
> > > > > > +                     cfg.c_feature_flags |=
> > > (~EROFS_FEATURE_SB_CHKSUM);
> > > > > > +                     break;
> > > > > > +             default:
> > > > > > +                     erofs_err("incorrect suboption");
> > > > > > +                     return -EINVAL;
> > > > > > +             }
> > > > > > +     }
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static int parse_extended_opts(const char *opts)
> > > > > >  {
> > > > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc,
> char
> > > > > *argv[])
> > > > > >  {
> > > > > >       int opt, i;
> > > > > >
> > > > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > > > >               switch (opt) {
> > > > > >               case 'z':
> > > > > >                       if (!optarg) {
> > > > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc,
> char
> > > > > *argv[])
> > > > > >                               return opt;
> > > > > >                       break;
> > > > > >
> > > > > > +             case 'O':
> > > > > > +                     opt = parse_feature_subopts(optarg);
> > > > > > +                     if (opt)
> > > > > > +                             return opt;
> > > > > > +                     break;
> > > > > > +
> > > > > >               default: /* '?' */
> > > > > >                       return -EINVAL;
> > > > > >               }
> > > > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc,
> char
> > > > > *argv[])
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > > > +{
> > > > > > +     u32 crc;
> > > > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > > > +                 sizeof(struct erofs_super_block));
> > > > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > > > +     return crc;
> > > > > > +}
> > > > > > +
> > > > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > > > > >                                 erofs_nid_t root_nid)
> > > > > >  {
> > > > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > > > erofs_buffer_head *bh,
> > > > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > > > >               .xattr_blkaddr = 0,
> > > > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > > > >       };
> > > > > >       const unsigned int sb_blksize =
> > > > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > > > erofs_buffer_head *bh,
> > > > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > > > >
> > > > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb,
> EROFS_FEATURE_SB_CHKSUM)) {
> > > > > > +             sb.checksum = 0;
> > > > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > > > +             sb.checksum = cpu_to_le32(crc);
> > > > > > +     }
> > > > > > +
> > > > > >       buf = calloc(sb_blksize, 1);
> > > > > >       if (!buf) {
> > > > > >               erofs_err("Failed to allocate memory for sb: %s",
> > > > > > --
> > > > > > 2.9.3
> > > > > >
> > > > >
> > >
>

[-- Attachment #2: Type: text/html, Size: 16435 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang]

Hi Pratik,

On Wed, Oct 09, 2019 at 01:54:40PM +0530, Pratik Shinde wrote:
> Hello Gao,
> 
> Yes I would like to work on pending features.
> Also please let us know the new development items.

Thanks, I will post later after gathering the first round
suggestions to mailing lists. We have time to improve this
filesystem. :)

Thanks,
Gao Xiang

> 
> --Pratik
> 
> On Wed, 9 Oct, 2019, 12:24 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:
> 
> > Hi Pratik,
> >
> > On Wed, Oct 09, 2019 at 11:59:01AM +0530, Pratik Shinde wrote:
> > > Hi Gao,
> > >
> > > Yes I can work on it. Sorry I missed this mail. I think your approach is
> > > good. Let me go through it one more time and reply back.
> >
> > Thanks for your reply and interest :)
> > I think we can complete all pending features together
> > if you have some time on this stuff. (fsdebug utility is
> > on the way as well...)
> >
> > BTW, I'm now investigating new high CR algorithm (very likely
> > XZ) as well, it will be likely a RFC version in this round for
> > wider scenarios and later decompression subsystem.
> >
> > Preliminary TODO lists will be discussed in this year China
> > Linux Storage & Filesystem workshop (next week) and will be
> > posted to mailing lists for further wider discussion (if more
> > folks have interest in developing it) as well. :)
> >
> > Thanks,
> > Gao Xiang
> >
> > >
> > > --Pratik
> > >
> > > On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> > >
> > > > Hi Pratik,
> > > >
> > > > On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > > > > Hi Gao,
> > > > >
> > > > > I completely understand your concern.You can drop this patch for now.
> > > > > Once erofs makes it 'fs/' please do reconsider implementing it.
> > > >
> > > > I think we can work on this pending feature for v5.5 now.
> > > > My idea is to add an extra field in erofs_super_block to
> > > > indicate the number of blocks (4k) for checking.
> > > >
> > > > So for small images, this feature can checksum the whole image at mount
> > > > time,
> > > > and for large images, this feature can be used to checksum the super
> > block
> > > > only
> > > > (and then use verficiation subsystem to verify on-the-fly in the
> > future...)
> > > >
> > > > The following workflow is for erofs-utils, I think it's
> > > >  1) erofs_mkfs_update_super_block with checksum = 0
> > > >  2) erofs_bflush(NULL)
> > > >  3) reread the corresponding blocks and calculate checksum;
> > > >  4) write checksum to erofs_super_block;
> > > >
> > > > Does it sound reasonable? and do you still have interest and
> > > > time for this? Looking forword to your reply...
> > > >
> > > > Thanks,
> > > > Gao Xiang
> > > >
> > > > >
> > > > > One more thing, can we still send non feature patches?
> > > > >
> > > > > --Pratik
> > > > >
> > > > >
> > > > > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com> wrote:
> > > > >
> > > > > > Hi Pratik,
> > > > > >
> > > > > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > > > > Adding code for superblock checksum calculation.
> > > > > > >
> > > > > > > incorporated the changes suggested in previous patch.
> > > > > > >
> > > > > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > > > >
> > > > > > Thanks for your v2 patch.
> > > > > >
> > > > > > Actually, I have some concern about the length of checksum,
> > > > > > sizeof(struct erofs_super_block) could be changed in the
> > > > > > later version, it's bad for EROFS future scalablity.
> > > > > >
> > > > > > And I tend not to add another on-disk field to record
> > > > > > the size of erofs_super_block as well, because the old
> > > > > > Linux kernel cannot handle more about the new size,
> > > > > > so it has little use except for checksum calculation.
> > > > > >
> > > > > > Few hours ago, I discussed with Chao about this concern,
> > > > > > I think this feature can be changed to do multiple-block
> > > > > > checksum at the mount time, e.g:
> > > > > >  - for small images, we can check the whole image once
> > > > > >    at the mount time;
> > > > > >  - for the large image, we can check the superblock
> > > > > >    at the mount time, the rest can be handled by
> > > > > >    block-based verification layer.
> > > > > >
> > > > > > But we agreed that don't add this for this round
> > > > > > since it's quite a new feature.
> > > > > >
> > > > > > All in all, it's a new feature since we are addressing moving
> > > > > > out of staging for this round. I tend to postpone this feature
> > > > > > for now. I understand that you are very interested in EROFS.
> > > > > > Considering EROFS current staging status, it's not such a place
> > > > > > to add new features at all! I have marked your patch down and
> > > > > > I will work with you later. Hope to get your understanding...
> > > > > >
> > > > > > Thanks,
> > > > > > Gao Xiang
> > > > > >
> > > > > > > ---
> > > > > > >  include/erofs/config.h |  1 +
> > > > > > >  include/erofs_fs.h     | 10 ++++++++
> > > > > > >  mkfs/main.c            | 64
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > > > > index 05fe6b2..40cd466 100644
> > > > > > > --- a/include/erofs/config.h
> > > > > > > +++ b/include/erofs/config.h
> > > > > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > > > > >       char *c_src_path;
> > > > > > >       char *c_compr_alg_master;
> > > > > > >       int c_compr_level_master;
> > > > > > > +     int c_feature_flags;
> > > > > > >  };
> > > > > > >
> > > > > > >  extern struct erofs_configure cfg;
> > > > > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > > > > index 601b477..9ac2635 100644
> > > > > > > --- a/include/erofs_fs.h
> > > > > > > +++ b/include/erofs_fs.h
> > > > > > > @@ -20,6 +20,16 @@
> > > > > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > > > > >  #define EROFS_ALL_REQUIREMENTS
> > > > > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > > > > >
> > > > > > > +/*
> > > > > > > + * feature definations.
> > > > > > > + */
> > > > > > > +#define EROFS_DEFAULT_FEATURES
> >  EROFS_FEATURE_SB_CHKSUM
> > > > > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > > > > +
> > > > > > > +
> > > > > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > > > > +
> > > > > > >  struct erofs_super_block {
> > > > > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > > > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > > > > index f127fe1..355fd2c 100644
> > > > > > > --- a/mkfs/main.c
> > > > > > > +++ b/mkfs/main.c
> > > > > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > > > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > > > > >  }
> > > > > > >
> > > > > > > +#define CRCPOLY      0x82F63B78
> > > > > > > +static inline u32 crc32c(u32 seed, unsigned char const *in,
> > size_t
> > > > len)
> > > > > > > +{
> > > > > > > +     int i;
> > > > > > > +     u32 crc = seed;
> > > > > > > +
> > > > > > > +     while (len--) {
> > > > > > > +             crc ^= *in++;
> > > > > > > +             for (i = 0; i < 8; i++) {
> > > > > > > +                     crc = (crc >> 1) ^ ((crc & 1) ? CRCPOLY :
> > 0);
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > > > > +     return crc;
> > > > > > > +}
> > > > > > > +
> > > > > > > +char *feature_opts[] = {
> > > > > > > +     "nosbcrc", NULL
> > > > > > > +};
> > > > > > > +#define O_SB_CKSUM   0
> > > > > > > +
> > > > > > > +static int parse_feature_subopts(char *opts)
> > > > > > > +{
> > > > > > > +     char *arg;
> > > > > > > +
> > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > +     while (*opts != '\0') {
> > > > > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > > > > +             case O_SB_CKSUM:
> > > > > > > +                     cfg.c_feature_flags |=
> > > > (~EROFS_FEATURE_SB_CHKSUM);
> > > > > > > +                     break;
> > > > > > > +             default:
> > > > > > > +                     erofs_err("incorrect suboption");
> > > > > > > +                     return -EINVAL;
> > > > > > > +             }
> > > > > > > +     }
> > > > > > > +     return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int parse_extended_opts(const char *opts)
> > > > > > >  {
> > > > > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int argc,
> > char
> > > > > > *argv[])
> > > > > > >  {
> > > > > > >       int opt, i;
> > > > > > >
> > > > > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > > > > >               switch (opt) {
> > > > > > >               case 'z':
> > > > > > >                       if (!optarg) {
> > > > > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int argc,
> > char
> > > > > > *argv[])
> > > > > > >                               return opt;
> > > > > > >                       break;
> > > > > > >
> > > > > > > +             case 'O':
> > > > > > > +                     opt = parse_feature_subopts(optarg);
> > > > > > > +                     if (opt)
> > > > > > > +                             return opt;
> > > > > > > +                     break;
> > > > > > > +
> > > > > > >               default: /* '?' */
> > > > > > >                       return -EINVAL;
> > > > > > >               }
> > > > > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int argc,
> > char
> > > > > > *argv[])
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > > > > +{
> > > > > > > +     u32 crc;
> > > > > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > > > > +                 sizeof(struct erofs_super_block));
> > > > > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > > > > +     return crc;
> > > > > > > +}
> > > > > > > +
> > > > > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > > > > > >                                 erofs_nid_t root_nid)
> > > > > > >  {
> > > > > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > > > > erofs_buffer_head *bh,
> > > > > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > > > > >               .xattr_blkaddr = 0,
> > > > > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > > > > >       };
> > > > > > >       const unsigned int sb_blksize =
> > > > > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > > > > erofs_buffer_head *bh,
> > > > > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > > > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > > > > >
> > > > > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb,
> > EROFS_FEATURE_SB_CHKSUM)) {
> > > > > > > +             sb.checksum = 0;
> > > > > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > > > > +             sb.checksum = cpu_to_le32(crc);
> > > > > > > +     }
> > > > > > > +
> > > > > > >       buf = calloc(sb_blksize, 1);
> > > > > > >       if (!buf) {
> > > > > > >               erofs_err("Failed to allocate memory for sb: %s",
> > > > > > > --
> > > > > > > 2.9.3
> > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 12973 bytes --]

Hello Gao,

I have few questions regarding the checksum feature:
1) Are we still keeping it as a feature. (i.e enabled by default . but can
be disabled during mkfs)
2) For small images we are going to cksum the entire image. what could be
the maximum size of 'small' images.

also, I think we don't have routines to read buffers back into the memory.
I will write them first and send a patch.

-Pratik

On Wed, Oct 9, 2019 at 2:14 PM Gao Xiang <gaoxiang25@huawei.com> wrote:

> Hi Pratik,
>
> On Wed, Oct 09, 2019 at 01:54:40PM +0530, Pratik Shinde wrote:
> > Hello Gao,
> >
> > Yes I would like to work on pending features.
> > Also please let us know the new development items.
>
> Thanks, I will post later after gathering the first round
> suggestions to mailing lists. We have time to improve this
> filesystem. :)
>
> Thanks,
> Gao Xiang
>
> >
> > --Pratik
> >
> > On Wed, 9 Oct, 2019, 12:24 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:
> >
> > > Hi Pratik,
> > >
> > > On Wed, Oct 09, 2019 at 11:59:01AM +0530, Pratik Shinde wrote:
> > > > Hi Gao,
> > > >
> > > > Yes I can work on it. Sorry I missed this mail. I think your
> approach is
> > > > good. Let me go through it one more time and reply back.
> > >
> > > Thanks for your reply and interest :)
> > > I think we can complete all pending features together
> > > if you have some time on this stuff. (fsdebug utility is
> > > on the way as well...)
> > >
> > > BTW, I'm now investigating new high CR algorithm (very likely
> > > XZ) as well, it will be likely a RFC version in this round for
> > > wider scenarios and later decompression subsystem.
> > >
> > > Preliminary TODO lists will be discussed in this year China
> > > Linux Storage & Filesystem workshop (next week) and will be
> > > posted to mailing lists for further wider discussion (if more
> > > folks have interest in developing it) as well. :)
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > > >
> > > > --Pratik
> > > >
> > > > On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> > > >
> > > > > Hi Pratik,
> > > > >
> > > > > On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > > > > > Hi Gao,
> > > > > >
> > > > > > I completely understand your concern.You can drop this patch for
> now.
> > > > > > Once erofs makes it 'fs/' please do reconsider implementing it.
> > > > >
> > > > > I think we can work on this pending feature for v5.5 now.
> > > > > My idea is to add an extra field in erofs_super_block to
> > > > > indicate the number of blocks (4k) for checking.
> > > > >
> > > > > So for small images, this feature can checksum the whole image at
> mount
> > > > > time,
> > > > > and for large images, this feature can be used to checksum the
> super
> > > block
> > > > > only
> > > > > (and then use verficiation subsystem to verify on-the-fly in the
> > > future...)
> > > > >
> > > > > The following workflow is for erofs-utils, I think it's
> > > > >  1) erofs_mkfs_update_super_block with checksum = 0
> > > > >  2) erofs_bflush(NULL)
> > > > >  3) reread the corresponding blocks and calculate checksum;
> > > > >  4) write checksum to erofs_super_block;
> > > > >
> > > > > Does it sound reasonable? and do you still have interest and
> > > > > time for this? Looking forword to your reply...
> > > > >
> > > > > Thanks,
> > > > > Gao Xiang
> > > > >
> > > > > >
> > > > > > One more thing, can we still send non feature patches?
> > > > > >
> > > > > > --Pratik
> > > > > >
> > > > > >
> > > > > > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com>
> wrote:
> > > > > >
> > > > > > > Hi Pratik,
> > > > > > >
> > > > > > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > > > > > Adding code for superblock checksum calculation.
> > > > > > > >
> > > > > > > > incorporated the changes suggested in previous patch.
> > > > > > > >
> > > > > > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > > > > >
> > > > > > > Thanks for your v2 patch.
> > > > > > >
> > > > > > > Actually, I have some concern about the length of checksum,
> > > > > > > sizeof(struct erofs_super_block) could be changed in the
> > > > > > > later version, it's bad for EROFS future scalablity.
> > > > > > >
> > > > > > > And I tend not to add another on-disk field to record
> > > > > > > the size of erofs_super_block as well, because the old
> > > > > > > Linux kernel cannot handle more about the new size,
> > > > > > > so it has little use except for checksum calculation.
> > > > > > >
> > > > > > > Few hours ago, I discussed with Chao about this concern,
> > > > > > > I think this feature can be changed to do multiple-block
> > > > > > > checksum at the mount time, e.g:
> > > > > > >  - for small images, we can check the whole image once
> > > > > > >    at the mount time;
> > > > > > >  - for the large image, we can check the superblock
> > > > > > >    at the mount time, the rest can be handled by
> > > > > > >    block-based verification layer.
> > > > > > >
> > > > > > > But we agreed that don't add this for this round
> > > > > > > since it's quite a new feature.
> > > > > > >
> > > > > > > All in all, it's a new feature since we are addressing moving
> > > > > > > out of staging for this round. I tend to postpone this feature
> > > > > > > for now. I understand that you are very interested in EROFS.
> > > > > > > Considering EROFS current staging status, it's not such a place
> > > > > > > to add new features at all! I have marked your patch down and
> > > > > > > I will work with you later. Hope to get your understanding...
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Gao Xiang
> > > > > > >
> > > > > > > > ---
> > > > > > > >  include/erofs/config.h |  1 +
> > > > > > > >  include/erofs_fs.h     | 10 ++++++++
> > > > > > > >  mkfs/main.c            | 64
> > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > > > > > index 05fe6b2..40cd466 100644
> > > > > > > > --- a/include/erofs/config.h
> > > > > > > > +++ b/include/erofs/config.h
> > > > > > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > > > > > >       char *c_src_path;
> > > > > > > >       char *c_compr_alg_master;
> > > > > > > >       int c_compr_level_master;
> > > > > > > > +     int c_feature_flags;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  extern struct erofs_configure cfg;
> > > > > > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > > > > > index 601b477..9ac2635 100644
> > > > > > > > --- a/include/erofs_fs.h
> > > > > > > > +++ b/include/erofs_fs.h
> > > > > > > > @@ -20,6 +20,16 @@
> > > > > > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > > > > > >  #define EROFS_ALL_REQUIREMENTS
> > > > > > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * feature definations.
> > > > > > > > + */
> > > > > > > > +#define EROFS_DEFAULT_FEATURES
> > >  EROFS_FEATURE_SB_CHKSUM
> > > > > > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > > > > > +
> > > > > > > > +
> > > > > > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > > > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > > > > > +
> > > > > > > >  struct erofs_super_block {
> > > > > > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > > > > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > > > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > > > > > index f127fe1..355fd2c 100644
> > > > > > > > --- a/mkfs/main.c
> > > > > > > > +++ b/mkfs/main.c
> > > > > > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > > > > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +#define CRCPOLY      0x82F63B78
> > > > > > > > +static inline u32 crc32c(u32 seed, unsigned char const *in,
> > > size_t
> > > > > len)
> > > > > > > > +{
> > > > > > > > +     int i;
> > > > > > > > +     u32 crc = seed;
> > > > > > > > +
> > > > > > > > +     while (len--) {
> > > > > > > > +             crc ^= *in++;
> > > > > > > > +             for (i = 0; i < 8; i++) {
> > > > > > > > +                     crc = (crc >> 1) ^ ((crc & 1) ?
> CRCPOLY :
> > > 0);
> > > > > > > > +             }
> > > > > > > > +     }
> > > > > > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > > > > > +     return crc;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +char *feature_opts[] = {
> > > > > > > > +     "nosbcrc", NULL
> > > > > > > > +};
> > > > > > > > +#define O_SB_CKSUM   0
> > > > > > > > +
> > > > > > > > +static int parse_feature_subopts(char *opts)
> > > > > > > > +{
> > > > > > > > +     char *arg;
> > > > > > > > +
> > > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > > +     while (*opts != '\0') {
> > > > > > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > > > > > +             case O_SB_CKSUM:
> > > > > > > > +                     cfg.c_feature_flags |=
> > > > > (~EROFS_FEATURE_SB_CHKSUM);
> > > > > > > > +                     break;
> > > > > > > > +             default:
> > > > > > > > +                     erofs_err("incorrect suboption");
> > > > > > > > +                     return -EINVAL;
> > > > > > > > +             }
> > > > > > > > +     }
> > > > > > > > +     return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static int parse_extended_opts(const char *opts)
> > > > > > > >  {
> > > > > > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > > > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int
> argc,
> > > char
> > > > > > > *argv[])
> > > > > > > >  {
> > > > > > > >       int opt, i;
> > > > > > > >
> > > > > > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > > > > > >               switch (opt) {
> > > > > > > >               case 'z':
> > > > > > > >                       if (!optarg) {
> > > > > > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int
> argc,
> > > char
> > > > > > > *argv[])
> > > > > > > >                               return opt;
> > > > > > > >                       break;
> > > > > > > >
> > > > > > > > +             case 'O':
> > > > > > > > +                     opt = parse_feature_subopts(optarg);
> > > > > > > > +                     if (opt)
> > > > > > > > +                             return opt;
> > > > > > > > +                     break;
> > > > > > > > +
> > > > > > > >               default: /* '?' */
> > > > > > > >                       return -EINVAL;
> > > > > > > >               }
> > > > > > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int
> argc,
> > > char
> > > > > > > *argv[])
> > > > > > > >       return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > > > > > +{
> > > > > > > > +     u32 crc;
> > > > > > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > > > > > +                 sizeof(struct erofs_super_block));
> > > > > > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > > > > > +     return crc;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head
> *bh,
> > > > > > > >                                 erofs_nid_t root_nid)
> > > > > > > >  {
> > > > > > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > > > > > erofs_buffer_head *bh,
> > > > > > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > > > > > >               .xattr_blkaddr = 0,
> > > > > > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > > > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > > > > > >       };
> > > > > > > >       const unsigned int sb_blksize =
> > > > > > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > > > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > > > > > erofs_buffer_head *bh,
> > > > > > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > > > > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > > > > > >
> > > > > > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb,
> > > EROFS_FEATURE_SB_CHKSUM)) {
> > > > > > > > +             sb.checksum = 0;
> > > > > > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > > > > > +             sb.checksum = cpu_to_le32(crc);
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > >       buf = calloc(sb_blksize, 1);
> > > > > > > >       if (!buf) {
> > > > > > > >               erofs_err("Failed to allocate memory for sb:
> %s",
> > > > > > > > --
> > > > > > > > 2.9.3
> > > > > > > >
> > > > > > >
> > > > >
> > >
>

[-- Attachment #2: Type: text/html, Size: 20172 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik,

On Wed, Oct 09, 2019 at 07:44:08PM +0530, Pratik Shinde wrote:
> Hello Gao,
> 
> I have few questions regarding the checksum feature:
> 1) Are we still keeping it as a feature. (i.e enabled by default . but can
> be disabled during mkfs)

Yes, I think it should be a new compatible feature suggested by Richard
and enabled by default (this feature should be marked in on-disk
super_block...)

> 2) For small images we are going to cksum the entire image. what could be
> the maximum size of 'small' images.

I think we can checksum the first 4k for now in mkfs by default
(or only checksum 4k as the first step), and then it can be
configured to a larger number n*4k. (But for the kernel side,
it has to be fully implemented due to forward compatibility.)

> 
> also, I think we don't have routines to read buffers back into the memory.
> I will write them first and send a patch.

Yes, I remembered that you once wrote a similar function, that is ok.
And it's no rush, we have more than a month to develop pending features
for linux-5.5. Just a reminder because we can prepare them for now :)

Thanks,
Gao Xiang

> 
> -Pratik
> 
> On Wed, Oct 9, 2019 at 2:14 PM Gao Xiang <gaoxiang25@huawei.com> wrote:
> 
> > Hi Pratik,
> >
> > On Wed, Oct 09, 2019 at 01:54:40PM +0530, Pratik Shinde wrote:
> > > Hello Gao,
> > >
> > > Yes I would like to work on pending features.
> > > Also please let us know the new development items.
> >
> > Thanks, I will post later after gathering the first round
> > suggestions to mailing lists. We have time to improve this
> > filesystem. :)
> >
> > Thanks,
> > Gao Xiang
> >
> > >
> > > --Pratik
> > >
> > > On Wed, 9 Oct, 2019, 12:24 PM Gao Xiang, <gaoxiang25@huawei.com> wrote:
> > >
> > > > Hi Pratik,
> > > >
> > > > On Wed, Oct 09, 2019 at 11:59:01AM +0530, Pratik Shinde wrote:
> > > > > Hi Gao,
> > > > >
> > > > > Yes I can work on it. Sorry I missed this mail. I think your
> > approach is
> > > > > good. Let me go through it one more time and reply back.
> > > >
> > > > Thanks for your reply and interest :)
> > > > I think we can complete all pending features together
> > > > if you have some time on this stuff. (fsdebug utility is
> > > > on the way as well...)
> > > >
> > > > BTW, I'm now investigating new high CR algorithm (very likely
> > > > XZ) as well, it will be likely a RFC version in this round for
> > > > wider scenarios and later decompression subsystem.
> > > >
> > > > Preliminary TODO lists will be discussed in this year China
> > > > Linux Storage & Filesystem workshop (next week) and will be
> > > > posted to mailing lists for further wider discussion (if more
> > > > folks have interest in developing it) as well. :)
> > > >
> > > > Thanks,
> > > > Gao Xiang
> > > >
> > > > >
> > > > > --Pratik
> > > > >
> > > > > On Sun, 6 Oct, 2019, 11:09 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> > > > >
> > > > > > Hi Pratik,
> > > > > >
> > > > > > On Sat, Aug 24, 2019 at 08:26:28PM +0530, Pratik Shinde wrote:
> > > > > > > Hi Gao,
> > > > > > >
> > > > > > > I completely understand your concern.You can drop this patch for
> > now.
> > > > > > > Once erofs makes it 'fs/' please do reconsider implementing it.
> > > > > >
> > > > > > I think we can work on this pending feature for v5.5 now.
> > > > > > My idea is to add an extra field in erofs_super_block to
> > > > > > indicate the number of blocks (4k) for checking.
> > > > > >
> > > > > > So for small images, this feature can checksum the whole image at
> > mount
> > > > > > time,
> > > > > > and for large images, this feature can be used to checksum the
> > super
> > > > block
> > > > > > only
> > > > > > (and then use verficiation subsystem to verify on-the-fly in the
> > > > future...)
> > > > > >
> > > > > > The following workflow is for erofs-utils, I think it's
> > > > > >  1) erofs_mkfs_update_super_block with checksum = 0
> > > > > >  2) erofs_bflush(NULL)
> > > > > >  3) reread the corresponding blocks and calculate checksum;
> > > > > >  4) write checksum to erofs_super_block;
> > > > > >
> > > > > > Does it sound reasonable? and do you still have interest and
> > > > > > time for this? Looking forword to your reply...
> > > > > >
> > > > > > Thanks,
> > > > > > Gao Xiang
> > > > > >
> > > > > > >
> > > > > > > One more thing, can we still send non feature patches?
> > > > > > >
> > > > > > > --Pratik
> > > > > > >
> > > > > > >
> > > > > > > On Sat, 24 Aug, 2019, 7:30 PM Gao Xiang, <hsiangkao@aol.com>
> > wrote:
> > > > > > >
> > > > > > > > Hi Pratik,
> > > > > > > >
> > > > > > > > On Sat, Aug 24, 2019 at 06:08:03PM +0530, Pratik Shinde wrote:
> > > > > > > > > Adding code for superblock checksum calculation.
> > > > > > > > >
> > > > > > > > > incorporated the changes suggested in previous patch.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > > > > > >
> > > > > > > > Thanks for your v2 patch.
> > > > > > > >
> > > > > > > > Actually, I have some concern about the length of checksum,
> > > > > > > > sizeof(struct erofs_super_block) could be changed in the
> > > > > > > > later version, it's bad for EROFS future scalablity.
> > > > > > > >
> > > > > > > > And I tend not to add another on-disk field to record
> > > > > > > > the size of erofs_super_block as well, because the old
> > > > > > > > Linux kernel cannot handle more about the new size,
> > > > > > > > so it has little use except for checksum calculation.
> > > > > > > >
> > > > > > > > Few hours ago, I discussed with Chao about this concern,
> > > > > > > > I think this feature can be changed to do multiple-block
> > > > > > > > checksum at the mount time, e.g:
> > > > > > > >  - for small images, we can check the whole image once
> > > > > > > >    at the mount time;
> > > > > > > >  - for the large image, we can check the superblock
> > > > > > > >    at the mount time, the rest can be handled by
> > > > > > > >    block-based verification layer.
> > > > > > > >
> > > > > > > > But we agreed that don't add this for this round
> > > > > > > > since it's quite a new feature.
> > > > > > > >
> > > > > > > > All in all, it's a new feature since we are addressing moving
> > > > > > > > out of staging for this round. I tend to postpone this feature
> > > > > > > > for now. I understand that you are very interested in EROFS.
> > > > > > > > Considering EROFS current staging status, it's not such a place
> > > > > > > > to add new features at all! I have marked your patch down and
> > > > > > > > I will work with you later. Hope to get your understanding...
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Gao Xiang
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >  include/erofs/config.h |  1 +
> > > > > > > > >  include/erofs_fs.h     | 10 ++++++++
> > > > > > > > >  mkfs/main.c            | 64
> > > > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > > > >  3 files changed, 74 insertions(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > > > > > > > index 05fe6b2..40cd466 100644
> > > > > > > > > --- a/include/erofs/config.h
> > > > > > > > > +++ b/include/erofs/config.h
> > > > > > > > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > > > > > > > >       char *c_src_path;
> > > > > > > > >       char *c_compr_alg_master;
> > > > > > > > >       int c_compr_level_master;
> > > > > > > > > +     int c_feature_flags;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  extern struct erofs_configure cfg;
> > > > > > > > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > > > > > > > index 601b477..9ac2635 100644
> > > > > > > > > --- a/include/erofs_fs.h
> > > > > > > > > +++ b/include/erofs_fs.h
> > > > > > > > > @@ -20,6 +20,16 @@
> > > > > > > > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > > > > > > > >  #define EROFS_ALL_REQUIREMENTS
> > > > > > > >  EROFS_REQUIREMENT_LZ4_0PADDING
> > > > > > > > >
> > > > > > > > > +/*
> > > > > > > > > + * feature definations.
> > > > > > > > > + */
> > > > > > > > > +#define EROFS_DEFAULT_FEATURES
> > > >  EROFS_FEATURE_SB_CHKSUM
> > > > > > > > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > > > > > > > +
> > > > > > > > > +
> > > > > > > > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > > > > > > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > > > > > > > +
> > > > > > > > >  struct erofs_super_block {
> > > > > > > > >  /*  0 */__le32 magic;           /* in the little endian */
> > > > > > > > >  /*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > > > > > > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > > > > > > > index f127fe1..355fd2c 100644
> > > > > > > > > --- a/mkfs/main.c
> > > > > > > > > +++ b/mkfs/main.c
> > > > > > > > > @@ -31,6 +31,45 @@ static void usage(void)
> > > > > > > > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +#define CRCPOLY      0x82F63B78
> > > > > > > > > +static inline u32 crc32c(u32 seed, unsigned char const *in,
> > > > size_t
> > > > > > len)
> > > > > > > > > +{
> > > > > > > > > +     int i;
> > > > > > > > > +     u32 crc = seed;
> > > > > > > > > +
> > > > > > > > > +     while (len--) {
> > > > > > > > > +             crc ^= *in++;
> > > > > > > > > +             for (i = 0; i < 8; i++) {
> > > > > > > > > +                     crc = (crc >> 1) ^ ((crc & 1) ?
> > CRCPOLY :
> > > > 0);
> > > > > > > > > +             }
> > > > > > > > > +     }
> > > > > > > > > +     erofs_dump("calculated crc: 0x%x\n", crc);
> > > > > > > > > +     return crc;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +char *feature_opts[] = {
> > > > > > > > > +     "nosbcrc", NULL
> > > > > > > > > +};
> > > > > > > > > +#define O_SB_CKSUM   0
> > > > > > > > > +
> > > > > > > > > +static int parse_feature_subopts(char *opts)
> > > > > > > > > +{
> > > > > > > > > +     char *arg;
> > > > > > > > > +
> > > > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > > > +     while (*opts != '\0') {
> > > > > > > > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > > > > > > > +             case O_SB_CKSUM:
> > > > > > > > > +                     cfg.c_feature_flags |=
> > > > > > (~EROFS_FEATURE_SB_CHKSUM);
> > > > > > > > > +                     break;
> > > > > > > > > +             default:
> > > > > > > > > +                     erofs_err("incorrect suboption");
> > > > > > > > > +                     return -EINVAL;
> > > > > > > > > +             }
> > > > > > > > > +     }
> > > > > > > > > +     return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  static int parse_extended_opts(const char *opts)
> > > > > > > > >  {
> > > > > > > > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > > > > > > > @@ -79,7 +118,8 @@ static int mkfs_parse_options_cfg(int
> > argc,
> > > > char
> > > > > > > > *argv[])
> > > > > > > > >  {
> > > > > > > > >       int opt, i;
> > > > > > > > >
> > > > > > > > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > > > > > > > +     cfg.c_feature_flags = EROFS_DEFAULT_FEATURES;
> > > > > > > > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > > > > > > > >               switch (opt) {
> > > > > > > > >               case 'z':
> > > > > > > > >                       if (!optarg) {
> > > > > > > > > @@ -113,6 +153,12 @@ static int mkfs_parse_options_cfg(int
> > argc,
> > > > char
> > > > > > > > *argv[])
> > > > > > > > >                               return opt;
> > > > > > > > >                       break;
> > > > > > > > >
> > > > > > > > > +             case 'O':
> > > > > > > > > +                     opt = parse_feature_subopts(optarg);
> > > > > > > > > +                     if (opt)
> > > > > > > > > +                             return opt;
> > > > > > > > > +                     break;
> > > > > > > > > +
> > > > > > > > >               default: /* '?' */
> > > > > > > > >                       return -EINVAL;
> > > > > > > > >               }
> > > > > > > > > @@ -144,6 +190,15 @@ static int mkfs_parse_options_cfg(int
> > argc,
> > > > char
> > > > > > > > *argv[])
> > > > > > > > >       return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > > > > > > > +{
> > > > > > > > > +     u32 crc;
> > > > > > > > > +     crc = crc32c(~0, (const unsigned char *)sb,
> > > > > > > > > +                 sizeof(struct erofs_super_block));
> > > > > > > > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > > > > > > > +     return crc;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head
> > *bh,
> > > > > > > > >                                 erofs_nid_t root_nid)
> > > > > > > > >  {
> > > > > > > > > @@ -155,6 +210,7 @@ int erofs_mkfs_update_super_block(struct
> > > > > > > > erofs_buffer_head *bh,
> > > > > > > > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > > > > > > > >               .xattr_blkaddr = 0,
> > > > > > > > >               .requirements = cpu_to_le32(sbi.requirements),
> > > > > > > > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > > > > > > > >       };
> > > > > > > > >       const unsigned int sb_blksize =
> > > > > > > > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > > > > > > > @@ -169,6 +225,12 @@ int erofs_mkfs_update_super_block(struct
> > > > > > > > erofs_buffer_head *bh,
> > > > > > > > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > > > > > > > >       sb.root_nid     = cpu_to_le16(root_nid);
> > > > > > > > >
> > > > > > > > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb,
> > > > EROFS_FEATURE_SB_CHKSUM)) {
> > > > > > > > > +             sb.checksum = 0;
> > > > > > > > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > > > > > > > +             sb.checksum = cpu_to_le32(crc);
> > > > > > > > > +     }
> > > > > > > > > +
> > > > > > > > >       buf = calloc(sb_blksize, 1);
> > > > > > > > >       if (!buf) {
> > > > > > > > >               erofs_err("Failed to allocate memory for sb:
> > %s",
> > > > > > > > > --
> > > > > > > > > 2.9.3
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik, and Chao,

On Sat, Aug 24, 2019 at 03:35:40PM +0530, Pratik Shinde wrote:
> Thanks Chao,
> 
> Initially I was of doing the same, but then I thought changing the position
> of cheksum field to either start OR end
> will be much simpler.

To Pratik: erofs layout is almost determined from linux-4.19, and it is fine
           for this chksum field;

> I think I will go with Gao's method. thereby not changing the layout of
> super-block.
> So just to be clear, I will be writing our own crc32c() function correct ?

           Yes, just go with crc32c(). and it's very simple to implement :)

> 
> --Pratik.
> 
> On Sat, Aug 24, 2019 at 3:31 PM Chao Yu <chao@kernel.org> wrote:
> 
> > On 2019-8-24 17:49, Gao Xiang via Linux-erofs wrote:
> > > Hi Pratik,
> > >
> > > On Sat, Aug 24, 2019 at 02:52:31PM +0530, Pratik Shinde wrote:
> > >> Hi Gao,
> > >> Yes I will make the suboption naming similar to that of mk2fs.
> > >
> > > Great! :)
> > >
> > >>
> > >> The reason I changed the position of 'checksum' field :
> > >>
> > >> Since we are calculating the checksum of erofs_super_block structure and
> > >> storing it in the same structure; we cannot include
> > >> this field for actual crc calculations. Keeping it at the end makes it
> > easy
> > >> for me to calculate length of the data of which
> > >> checksum needs to be calculated. I saw similar logic in other
> > filesystems
> > >> like ext4.
> > >
> > > No, that is just they didn't add checksum field at the beginning of its
> > design.
> > > I think you can leave chksum to 0, and calculate chksum and fill it, to
> > be specfic:
> > >
> > > In the erofs-utils,
> > >  1) fill .checksum to 0;
> > >  2) calculate crc32c of the entire erofs_super_block;
> > >  3) fill the real checksum to .checksum;
> > >
> > > In the kernel,
> > >  1) read .checksum to a variable;
> > >  2) fill .checksum to 0;
> > >  3) calculate crc32c of the entire erofs_super_block;
> > >  4) compare the given one and the calculated one;
> >
> > That's one way, FYI, the way used in ext4/f2fs now is:
> > - calc [0, checksum]'s chksum
> > - use above chksum as seed of chksum within range [chksum + chksum_size,
> > end]
> > - fill chksum value in range [checksum, chksum + chksum_size]

To Chao: Yes, I think that's ok as well, but we can do much simple in EROFS,
         and it won't break crc32c pipeline... (calculate in one crc32c function) :)

Thanks,
Gao Xiang

> >
> > Thanks,
> >
> > >
> > > That is all :)
> > >
> > >>
> > >> We can write our own crc32() function. :) There is no problem. I thought
> > >> zlib already provides one & we can use it.
> > >
> > > I think we can use crc32c() since I already wrote comments for erofs in
> > 4.19.
> > >
> > > Looking forward to your next version :)
> > >
> > > Thanks,
> > > Gao Xiang
> > >
> > >> anyways , I will write.
> > >>
> > >> --Pratik.
> > >>
> > >> On Sat, Aug 24, 2019 at 2:16 PM Gao Xiang <hsiangkao@gmx.com> wrote:
> > >>
> > >>> Hi Pratik,
> > >>>
> > >>> On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
> > >>>> Adding code for superblock checksum calculation.
> > >>>>
> > >>>> This patch adds following things:
> > >>>> 1)Handle suboptions('-o') to mkfs utility.
> > >>>
> > >>> Thanks for your patch. :)
> > >>>
> > >>> Can we use "-O feature" instead in order to keep in line with mke2fs?
> > >>>
> > >>>> 2)Add superblock checksum calculation(-o sb_cksum) as suboption.
> > >>>
> > >>> ditto. and I think we can enable sbcrc by default since it is a compat
> > >>> feature,
> > >>> and add "-O nosbcrc" to disable it.
> > >>>
> > >>>> 3)Calculate superblock checksum if feature is enabled.
> > >>>>
> > >>>> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > >>>
> > >>> And could you please also read my following comments and fix them.
> > >>> and I'd like to accept your erofs-utils modification in advance. :)
> > >>>
> > >>>
> > >>> But now you can see we are moving EROFS out of staging now as
> > >>> the "real" part of Linux, this is the fundamental stuff of other
> > >>> new features if we want to develop more actively... So we can wait
> > >>> for the final result and add this new feature to kernel then...
> > >>>
> > >>>> ---
> > >>>>  include/erofs/config.h |  1 +
> > >>>>  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
> > >>>>  mkfs/main.c            | 53
> > >>> +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >>>>  3 files changed, 76 insertions(+), 18 deletions(-)
> > >>>>
> > >>>> diff --git a/include/erofs/config.h b/include/erofs/config.h
> > >>>> index 05fe6b2..40cd466 100644
> > >>>> --- a/include/erofs/config.h
> > >>>> +++ b/include/erofs/config.h
> > >>>> @@ -22,6 +22,7 @@ struct erofs_configure {
> > >>>>       char *c_src_path;
> > >>>>       char *c_compr_alg_master;
> > >>>>       int c_compr_level_master;
> > >>>> +     int c_feature_flags;
> > >>>
> > >>> we can add this to sbi like requirements...
> > >>>
> > >>>>  };
> > >>>>
> > >>>>  extern struct erofs_configure cfg;
> > >>>> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > >>>> index 601b477..c9ef057 100644
> > >>>> --- a/include/erofs_fs.h
> > >>>> +++ b/include/erofs_fs.h
> > >>>> @@ -20,25 +20,31 @@
> > >>>>  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > >>>>  #define EROFS_ALL_REQUIREMENTS
> > >>>  EROFS_REQUIREMENT_LZ4_0PADDING
> > >>>>
> > >>>> +/*
> > >>>> + * feature definations.
> > >>>> + */
> > >>>> +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > >>>> +
> > >>>> +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > >>>> +     ( le32_to_cpu((super)->features) & (mask) )
> > >>>> +
> > >>>>  struct erofs_super_block {
> > >>>>  /*  0 */__le32 magic;           /* in the little endian */
> > >>>> -/*  4 */__le32 checksum;        /* crc32c(super_block) */
> > >>>> -/*  8 */__le32 features;        /* (aka. feature_compat) */
> > >>>> -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE
> > only
> > >>> */
> > >>>> -/* 13 */__u8 reserved;
> > >>>> -
> > >>>> -/* 14 */__le16 root_nid;
> > >>>> -/* 16 */__le64 inos;            /* total valid ino # (== f_files -
> > >>> f_favail) */
> > >>>> -
> > >>>> -/* 24 */__le64 build_time;      /* inode v1 time derivation */
> > >>>> -/* 32 */__le32 build_time_nsec;
> > >>>> -/* 36 */__le32 blocks;          /* used for statfs */
> > >>>> -/* 40 */__le32 meta_blkaddr;
> > >>>> -/* 44 */__le32 xattr_blkaddr;
> > >>>> -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > >>>> -/* 64 */__u8 volume_name[16];   /* volume name */
> > >>>> -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
> > >>>> -
> > >>>> +/*  4 */__le32 features;        /* (aka. feature_compat) */
> > >>>> +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE
> > only
> > >>> */
> > >>>> +/*  9 */__u8 reserved;
> > >>>> +
> > >>>> +/* 10 */__le16 root_nid;
> > >>>> +/* 12 */__le64 inos;            /* total valid ino # (== f_files -
> > >>> f_favail) */
> > >>>> +/* 20 */__le64 build_time;      /* inode v1 time derivation */
> > >>>> +/* 28 */__le32 build_time_nsec;
> > >>>> +/* 32 */__le32 blocks;          /* used for statfs */
> > >>>> +/* 36 */__le32 meta_blkaddr;
> > >>>> +/* 40 */__le32 xattr_blkaddr;
> > >>>> +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > >>>> +/* 60 */__u8 volume_name[16];   /* volume name */
> > >>>> +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
> > >>>> +/* 80 */__le32 checksum;        /* crc32c(super_block) */
> > >>>>  /* 84 */__u8 reserved2[44];
> > >>>
> > >>> Why modifying the above?
> > >>>
> > >>>>  } __packed;                     /* 128 bytes */
> > >>>>
> > >>>> diff --git a/mkfs/main.c b/mkfs/main.c
> > >>>> index f127fe1..26e14a3 100644
> > >>>> --- a/mkfs/main.c
> > >>>> +++ b/mkfs/main.c
> > >>>> @@ -13,12 +13,14 @@
> > >>>>  #include <limits.h>
> > >>>>  #include <libgen.h>
> > >>>>  #include <sys/stat.h>
> > >>>> +#include <zlib.h>
> > >>>
> > >>> I have no idea that we should introduce "zlib" just for crc32c
> > currently...
> > >>> Maybe we can add some independent crc32 function..
> > >>>
> > >>> Thanks,
> > >>> Gao Xiang
> > >>>
> > >>>>  #include "erofs/config.h"
> > >>>>  #include "erofs/print.h"
> > >>>>  #include "erofs/cache.h"
> > >>>>  #include "erofs/inode.h"
> > >>>>  #include "erofs/io.h"
> > >>>>  #include "erofs/compress.h"
> > >>>> +#include "erofs/defs.h"
> > >>>>
> > >>>>  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> > >>> erofs_super_block))
> > >>>>
> > >>>> @@ -31,6 +33,28 @@ static void usage(void)
> > >>>>       fprintf(stderr, " -EX[,...] X=extended options\n");
> > >>>>  }
> > >>>>
> > >>>> +char *feature_opts[] = {
> > >>>> +     "sb_cksum", NULL
> > >>>> +};
> > >>>> +#define O_SB_CKSUM   0
> > >>>> +
> > >>>> +static int parse_feature_subopts(char *opts)
> > >>>> +{
> > >>>> +     char *arg;
> > >>>> +
> > >>>> +     while (*opts != '\0') {
> > >>>> +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > >>>> +             case O_SB_CKSUM:
> > >>>> +                     cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
> > >>>> +                     break;
> > >>>> +             default:
> > >>>> +                     erofs_err("incorrect suboption");
> > >>>> +                     return -EINVAL;
> > >>>> +             }
> > >>>> +     }
> > >>>> +     return 0;
> > >>>> +}
> > >>>> +
> > >>>>  static int parse_extended_opts(const char *opts)
> > >>>>  {
> > >>>>  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > >>>> @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char
> > >>> *argv[])
> > >>>>  {
> > >>>>       int opt, i;
> > >>>>
> > >>>> -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > >>>> +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > >>>>               switch (opt) {
> > >>>>               case 'z':
> > >>>>                       if (!optarg) {
> > >>>> @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > >>> *argv[])
> > >>>>                               return opt;
> > >>>>                       break;
> > >>>>
> > >>>> +             case 'o':
> > >>>> +                     opt = parse_feature_subopts(optarg);
> > >>>> +                     if (opt)
> > >>>> +                             return opt;
> > >>>> +                     break;
> > >>>> +
> > >>>>               default: /* '?' */
> > >>>>                       return -EINVAL;
> > >>>>               }
> > >>>> @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char
> > >>> *argv[])
> > >>>>       return 0;
> > >>>>  }
> > >>>>
> > >>>> +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > >>>> +{
> > >>>> +     int offset;
> > >>>> +     u32 crc;
> > >>>> +
> > >>>> +     offset = offsetof(struct erofs_super_block, checksum);
> > >>>> +     if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
> > >>>> +             erofs_err("Invalid offset of checksum field: %d",
> > offset);
> > >>>> +             return -1;
> > >>>> +     }
> > >>>> +     crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
> > >>>> +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > >>>> +     return 0;
> > >>>> +}
> > >>>> +
> > >>>>  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > >>>>                                 erofs_nid_t root_nid)
> > >>>>  {
> > >>>> @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct
> > >>> erofs_buffer_head *bh,
> > >>>>               .meta_blkaddr  = sbi.meta_blkaddr,
> > >>>>               .xattr_blkaddr = 0,
> > >>>>               .requirements = cpu_to_le32(sbi.requirements),
> > >>>> +             .features = cpu_to_le32(cfg.c_feature_flags),
> > >>>>       };
> > >>>>       const unsigned int sb_blksize =
> > >>>>               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > >>>> @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct
> > >>> erofs_buffer_head *bh,
> > >>>>       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > >>>>       sb.root_nid     = cpu_to_le16(root_nid);
> > >>>>
> > >>>> +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > >>>> +             u32 crc = erofs_superblock_checksum(&sb);
> > >>>> +             sb.checksum = cpu_to_le32(crc);
> > >>>> +     }
> > >>>> +
> > >>>>       buf = calloc(sb_blksize, 1);
> > >>>>       if (!buf) {
> > >>>>               erofs_err("Failed to allocate memory for sb: %s",
> > >>>> --
> > >>>> 2.9.3
> > >>>>
> > >>>
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 11363 bytes --]

Thanks Chao,

Initially I was of doing the same, but then I thought changing the position
of cheksum field to either start OR end
will be much simpler.
I think I will go with Gao's method. thereby not changing the layout of
super-block.
So just to be clear, I will be writing our own crc32c() function correct ?

--Pratik.

On Sat, Aug 24, 2019 at 3:31 PM Chao Yu <chao@kernel.org> wrote:

> On 2019-8-24 17:49, Gao Xiang via Linux-erofs wrote:
> > Hi Pratik,
> >
> > On Sat, Aug 24, 2019 at 02:52:31PM +0530, Pratik Shinde wrote:
> >> Hi Gao,
> >> Yes I will make the suboption naming similar to that of mk2fs.
> >
> > Great! :)
> >
> >>
> >> The reason I changed the position of 'checksum' field :
> >>
> >> Since we are calculating the checksum of erofs_super_block structure and
> >> storing it in the same structure; we cannot include
> >> this field for actual crc calculations. Keeping it at the end makes it
> easy
> >> for me to calculate length of the data of which
> >> checksum needs to be calculated. I saw similar logic in other
> filesystems
> >> like ext4.
> >
> > No, that is just they didn't add checksum field at the beginning of its
> design.
> > I think you can leave chksum to 0, and calculate chksum and fill it, to
> be specfic:
> >
> > In the erofs-utils,
> >  1) fill .checksum to 0;
> >  2) calculate crc32c of the entire erofs_super_block;
> >  3) fill the real checksum to .checksum;
> >
> > In the kernel,
> >  1) read .checksum to a variable;
> >  2) fill .checksum to 0;
> >  3) calculate crc32c of the entire erofs_super_block;
> >  4) compare the given one and the calculated one;
>
> That's one way, FYI, the way used in ext4/f2fs now is:
> - calc [0, checksum]'s chksum
> - use above chksum as seed of chksum within range [chksum + chksum_size,
> end]
> - fill chksum value in range [checksum, chksum + chksum_size]
>
> Thanks,
>
> >
> > That is all :)
> >
> >>
> >> We can write our own crc32() function. :) There is no problem. I thought
> >> zlib already provides one & we can use it.
> >
> > I think we can use crc32c() since I already wrote comments for erofs in
> 4.19.
> >
> > Looking forward to your next version :)
> >
> > Thanks,
> > Gao Xiang
> >
> >> anyways , I will write.
> >>
> >> --Pratik.
> >>
> >> On Sat, Aug 24, 2019 at 2:16 PM Gao Xiang <hsiangkao@gmx.com> wrote:
> >>
> >>> Hi Pratik,
> >>>
> >>> On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
> >>>> Adding code for superblock checksum calculation.
> >>>>
> >>>> This patch adds following things:
> >>>> 1)Handle suboptions('-o') to mkfs utility.
> >>>
> >>> Thanks for your patch. :)
> >>>
> >>> Can we use "-O feature" instead in order to keep in line with mke2fs?
> >>>
> >>>> 2)Add superblock checksum calculation(-o sb_cksum) as suboption.
> >>>
> >>> ditto. and I think we can enable sbcrc by default since it is a compat
> >>> feature,
> >>> and add "-O nosbcrc" to disable it.
> >>>
> >>>> 3)Calculate superblock checksum if feature is enabled.
> >>>>
> >>>> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> >>>
> >>> And could you please also read my following comments and fix them.
> >>> and I'd like to accept your erofs-utils modification in advance. :)
> >>>
> >>>
> >>> But now you can see we are moving EROFS out of staging now as
> >>> the "real" part of Linux, this is the fundamental stuff of other
> >>> new features if we want to develop more actively... So we can wait
> >>> for the final result and add this new feature to kernel then...
> >>>
> >>>> ---
> >>>>  include/erofs/config.h |  1 +
> >>>>  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
> >>>>  mkfs/main.c            | 53
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>>  3 files changed, 76 insertions(+), 18 deletions(-)
> >>>>
> >>>> diff --git a/include/erofs/config.h b/include/erofs/config.h
> >>>> index 05fe6b2..40cd466 100644
> >>>> --- a/include/erofs/config.h
> >>>> +++ b/include/erofs/config.h
> >>>> @@ -22,6 +22,7 @@ struct erofs_configure {
> >>>>       char *c_src_path;
> >>>>       char *c_compr_alg_master;
> >>>>       int c_compr_level_master;
> >>>> +     int c_feature_flags;
> >>>
> >>> we can add this to sbi like requirements...
> >>>
> >>>>  };
> >>>>
> >>>>  extern struct erofs_configure cfg;
> >>>> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> >>>> index 601b477..c9ef057 100644
> >>>> --- a/include/erofs_fs.h
> >>>> +++ b/include/erofs_fs.h
> >>>> @@ -20,25 +20,31 @@
> >>>>  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> >>>>  #define EROFS_ALL_REQUIREMENTS
> >>>  EROFS_REQUIREMENT_LZ4_0PADDING
> >>>>
> >>>> +/*
> >>>> + * feature definations.
> >>>> + */
> >>>> +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> >>>> +
> >>>> +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> >>>> +     ( le32_to_cpu((super)->features) & (mask) )
> >>>> +
> >>>>  struct erofs_super_block {
> >>>>  /*  0 */__le32 magic;           /* in the little endian */
> >>>> -/*  4 */__le32 checksum;        /* crc32c(super_block) */
> >>>> -/*  8 */__le32 features;        /* (aka. feature_compat) */
> >>>> -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE
> only
> >>> */
> >>>> -/* 13 */__u8 reserved;
> >>>> -
> >>>> -/* 14 */__le16 root_nid;
> >>>> -/* 16 */__le64 inos;            /* total valid ino # (== f_files -
> >>> f_favail) */
> >>>> -
> >>>> -/* 24 */__le64 build_time;      /* inode v1 time derivation */
> >>>> -/* 32 */__le32 build_time_nsec;
> >>>> -/* 36 */__le32 blocks;          /* used for statfs */
> >>>> -/* 40 */__le32 meta_blkaddr;
> >>>> -/* 44 */__le32 xattr_blkaddr;
> >>>> -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
> >>>> -/* 64 */__u8 volume_name[16];   /* volume name */
> >>>> -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
> >>>> -
> >>>> +/*  4 */__le32 features;        /* (aka. feature_compat) */
> >>>> +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE
> only
> >>> */
> >>>> +/*  9 */__u8 reserved;
> >>>> +
> >>>> +/* 10 */__le16 root_nid;
> >>>> +/* 12 */__le64 inos;            /* total valid ino # (== f_files -
> >>> f_favail) */
> >>>> +/* 20 */__le64 build_time;      /* inode v1 time derivation */
> >>>> +/* 28 */__le32 build_time_nsec;
> >>>> +/* 32 */__le32 blocks;          /* used for statfs */
> >>>> +/* 36 */__le32 meta_blkaddr;
> >>>> +/* 40 */__le32 xattr_blkaddr;
> >>>> +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
> >>>> +/* 60 */__u8 volume_name[16];   /* volume name */
> >>>> +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
> >>>> +/* 80 */__le32 checksum;        /* crc32c(super_block) */
> >>>>  /* 84 */__u8 reserved2[44];
> >>>
> >>> Why modifying the above?
> >>>
> >>>>  } __packed;                     /* 128 bytes */
> >>>>
> >>>> diff --git a/mkfs/main.c b/mkfs/main.c
> >>>> index f127fe1..26e14a3 100644
> >>>> --- a/mkfs/main.c
> >>>> +++ b/mkfs/main.c
> >>>> @@ -13,12 +13,14 @@
> >>>>  #include <limits.h>
> >>>>  #include <libgen.h>
> >>>>  #include <sys/stat.h>
> >>>> +#include <zlib.h>
> >>>
> >>> I have no idea that we should introduce "zlib" just for crc32c
> currently...
> >>> Maybe we can add some independent crc32 function..
> >>>
> >>> Thanks,
> >>> Gao Xiang
> >>>
> >>>>  #include "erofs/config.h"
> >>>>  #include "erofs/print.h"
> >>>>  #include "erofs/cache.h"
> >>>>  #include "erofs/inode.h"
> >>>>  #include "erofs/io.h"
> >>>>  #include "erofs/compress.h"
> >>>> +#include "erofs/defs.h"
> >>>>
> >>>>  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> >>> erofs_super_block))
> >>>>
> >>>> @@ -31,6 +33,28 @@ static void usage(void)
> >>>>       fprintf(stderr, " -EX[,...] X=extended options\n");
> >>>>  }
> >>>>
> >>>> +char *feature_opts[] = {
> >>>> +     "sb_cksum", NULL
> >>>> +};
> >>>> +#define O_SB_CKSUM   0
> >>>> +
> >>>> +static int parse_feature_subopts(char *opts)
> >>>> +{
> >>>> +     char *arg;
> >>>> +
> >>>> +     while (*opts != '\0') {
> >>>> +             switch(getsubopt(&opts, feature_opts, &arg)) {
> >>>> +             case O_SB_CKSUM:
> >>>> +                     cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
> >>>> +                     break;
> >>>> +             default:
> >>>> +                     erofs_err("incorrect suboption");
> >>>> +                     return -EINVAL;
> >>>> +             }
> >>>> +     }
> >>>> +     return 0;
> >>>> +}
> >>>> +
> >>>>  static int parse_extended_opts(const char *opts)
> >>>>  {
> >>>>  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> >>>> @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char
> >>> *argv[])
> >>>>  {
> >>>>       int opt, i;
> >>>>
> >>>> -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> >>>> +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> >>>>               switch (opt) {
> >>>>               case 'z':
> >>>>                       if (!optarg) {
> >>>> @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char
> >>> *argv[])
> >>>>                               return opt;
> >>>>                       break;
> >>>>
> >>>> +             case 'o':
> >>>> +                     opt = parse_feature_subopts(optarg);
> >>>> +                     if (opt)
> >>>> +                             return opt;
> >>>> +                     break;
> >>>> +
> >>>>               default: /* '?' */
> >>>>                       return -EINVAL;
> >>>>               }
> >>>> @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char
> >>> *argv[])
> >>>>       return 0;
> >>>>  }
> >>>>
> >>>> +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> >>>> +{
> >>>> +     int offset;
> >>>> +     u32 crc;
> >>>> +
> >>>> +     offset = offsetof(struct erofs_super_block, checksum);
> >>>> +     if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
> >>>> +             erofs_err("Invalid offset of checksum field: %d",
> offset);
> >>>> +             return -1;
> >>>> +     }
> >>>> +     crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
> >>>> +     erofs_dump("superblock checksum: 0x%x\n", crc);
> >>>> +     return 0;
> >>>> +}
> >>>> +
> >>>>  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >>>>                                 erofs_nid_t root_nid)
> >>>>  {
> >>>> @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct
> >>> erofs_buffer_head *bh,
> >>>>               .meta_blkaddr  = sbi.meta_blkaddr,
> >>>>               .xattr_blkaddr = 0,
> >>>>               .requirements = cpu_to_le32(sbi.requirements),
> >>>> +             .features = cpu_to_le32(cfg.c_feature_flags),
> >>>>       };
> >>>>       const unsigned int sb_blksize =
> >>>>               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> >>>> @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct
> >>> erofs_buffer_head *bh,
> >>>>       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> >>>>       sb.root_nid     = cpu_to_le16(root_nid);
> >>>>
> >>>> +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> >>>> +             u32 crc = erofs_superblock_checksum(&sb);
> >>>> +             sb.checksum = cpu_to_le32(crc);
> >>>> +     }
> >>>> +
> >>>>       buf = calloc(sb_blksize, 1);
> >>>>       if (!buf) {
> >>>>               erofs_err("Failed to allocate memory for sb: %s",
> >>>> --
> >>>> 2.9.3
> >>>>
> >>>
>

[-- Attachment #2: Type: text/html, Size: 16642 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Chao Yu]

On 2019-8-24 17:49, Gao Xiang via Linux-erofs wrote:
> Hi Pratik,
> 
> On Sat, Aug 24, 2019 at 02:52:31PM +0530, Pratik Shinde wrote:
>> Hi Gao,
>> Yes I will make the suboption naming similar to that of mk2fs.
> 
> Great! :)
> 
>>
>> The reason I changed the position of 'checksum' field :
>>
>> Since we are calculating the checksum of erofs_super_block structure and
>> storing it in the same structure; we cannot include
>> this field for actual crc calculations. Keeping it at the end makes it easy
>> for me to calculate length of the data of which
>> checksum needs to be calculated. I saw similar logic in other filesystems
>> like ext4.
> 
> No, that is just they didn't add checksum field at the beginning of its design.
> I think you can leave chksum to 0, and calculate chksum and fill it, to be specfic:
> 
> In the erofs-utils,
>  1) fill .checksum to 0;
>  2) calculate crc32c of the entire erofs_super_block;
>  3) fill the real checksum to .checksum;
> 
> In the kernel,
>  1) read .checksum to a variable;
>  2) fill .checksum to 0;
>  3) calculate crc32c of the entire erofs_super_block;
>  4) compare the given one and the calculated one;

That's one way, FYI, the way used in ext4/f2fs now is:
- calc [0, checksum]'s chksum
- use above chksum as seed of chksum within range [chksum + chksum_size, end]
- fill chksum value in range [checksum, chksum + chksum_size]

Thanks,

> 
> That is all :)
> 
>>
>> We can write our own crc32() function. :) There is no problem. I thought
>> zlib already provides one & we can use it.
> 
> I think we can use crc32c() since I already wrote comments for erofs in 4.19.
> 
> Looking forward to your next version :)
> 
> Thanks,
> Gao Xiang
> 
>> anyways , I will write.
>>
>> --Pratik.
>>
>> On Sat, Aug 24, 2019 at 2:16 PM Gao Xiang <hsiangkao@gmx.com> wrote:
>>
>>> Hi Pratik,
>>>
>>> On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
>>>> Adding code for superblock checksum calculation.
>>>>
>>>> This patch adds following things:
>>>> 1)Handle suboptions('-o') to mkfs utility.
>>>
>>> Thanks for your patch. :)
>>>
>>> Can we use "-O feature" instead in order to keep in line with mke2fs?
>>>
>>>> 2)Add superblock checksum calculation(-o sb_cksum) as suboption.
>>>
>>> ditto. and I think we can enable sbcrc by default since it is a compat
>>> feature,
>>> and add "-O nosbcrc" to disable it.
>>>
>>>> 3)Calculate superblock checksum if feature is enabled.
>>>>
>>>> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
>>>
>>> And could you please also read my following comments and fix them.
>>> and I'd like to accept your erofs-utils modification in advance. :)
>>>
>>>
>>> But now you can see we are moving EROFS out of staging now as
>>> the "real" part of Linux, this is the fundamental stuff of other
>>> new features if we want to develop more actively... So we can wait
>>> for the final result and add this new feature to kernel then...
>>>
>>>> ---
>>>>  include/erofs/config.h |  1 +
>>>>  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
>>>>  mkfs/main.c            | 53
>>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  3 files changed, 76 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/include/erofs/config.h b/include/erofs/config.h
>>>> index 05fe6b2..40cd466 100644
>>>> --- a/include/erofs/config.h
>>>> +++ b/include/erofs/config.h
>>>> @@ -22,6 +22,7 @@ struct erofs_configure {
>>>>       char *c_src_path;
>>>>       char *c_compr_alg_master;
>>>>       int c_compr_level_master;
>>>> +     int c_feature_flags;
>>>
>>> we can add this to sbi like requirements...
>>>
>>>>  };
>>>>
>>>>  extern struct erofs_configure cfg;
>>>> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
>>>> index 601b477..c9ef057 100644
>>>> --- a/include/erofs_fs.h
>>>> +++ b/include/erofs_fs.h
>>>> @@ -20,25 +20,31 @@
>>>>  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
>>>>  #define EROFS_ALL_REQUIREMENTS
>>>  EROFS_REQUIREMENT_LZ4_0PADDING
>>>>
>>>> +/*
>>>> + * feature definations.
>>>> + */
>>>> +#define EROFS_FEATURE_SB_CHKSUM              0x0001
>>>> +
>>>> +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
>>>> +     ( le32_to_cpu((super)->features) & (mask) )
>>>> +
>>>>  struct erofs_super_block {
>>>>  /*  0 */__le32 magic;           /* in the little endian */
>>>> -/*  4 */__le32 checksum;        /* crc32c(super_block) */
>>>> -/*  8 */__le32 features;        /* (aka. feature_compat) */
>>>> -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
>>> */
>>>> -/* 13 */__u8 reserved;
>>>> -
>>>> -/* 14 */__le16 root_nid;
>>>> -/* 16 */__le64 inos;            /* total valid ino # (== f_files -
>>> f_favail) */
>>>> -
>>>> -/* 24 */__le64 build_time;      /* inode v1 time derivation */
>>>> -/* 32 */__le32 build_time_nsec;
>>>> -/* 36 */__le32 blocks;          /* used for statfs */
>>>> -/* 40 */__le32 meta_blkaddr;
>>>> -/* 44 */__le32 xattr_blkaddr;
>>>> -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
>>>> -/* 64 */__u8 volume_name[16];   /* volume name */
>>>> -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
>>>> -
>>>> +/*  4 */__le32 features;        /* (aka. feature_compat) */
>>>> +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
>>> */
>>>> +/*  9 */__u8 reserved;
>>>> +
>>>> +/* 10 */__le16 root_nid;
>>>> +/* 12 */__le64 inos;            /* total valid ino # (== f_files -
>>> f_favail) */
>>>> +/* 20 */__le64 build_time;      /* inode v1 time derivation */
>>>> +/* 28 */__le32 build_time_nsec;
>>>> +/* 32 */__le32 blocks;          /* used for statfs */
>>>> +/* 36 */__le32 meta_blkaddr;
>>>> +/* 40 */__le32 xattr_blkaddr;
>>>> +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
>>>> +/* 60 */__u8 volume_name[16];   /* volume name */
>>>> +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
>>>> +/* 80 */__le32 checksum;        /* crc32c(super_block) */
>>>>  /* 84 */__u8 reserved2[44];
>>>
>>> Why modifying the above?
>>>
>>>>  } __packed;                     /* 128 bytes */
>>>>
>>>> diff --git a/mkfs/main.c b/mkfs/main.c
>>>> index f127fe1..26e14a3 100644
>>>> --- a/mkfs/main.c
>>>> +++ b/mkfs/main.c
>>>> @@ -13,12 +13,14 @@
>>>>  #include <limits.h>
>>>>  #include <libgen.h>
>>>>  #include <sys/stat.h>
>>>> +#include <zlib.h>
>>>
>>> I have no idea that we should introduce "zlib" just for crc32c currently...
>>> Maybe we can add some independent crc32 function..
>>>
>>> Thanks,
>>> Gao Xiang
>>>
>>>>  #include "erofs/config.h"
>>>>  #include "erofs/print.h"
>>>>  #include "erofs/cache.h"
>>>>  #include "erofs/inode.h"
>>>>  #include "erofs/io.h"
>>>>  #include "erofs/compress.h"
>>>> +#include "erofs/defs.h"
>>>>
>>>>  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
>>> erofs_super_block))
>>>>
>>>> @@ -31,6 +33,28 @@ static void usage(void)
>>>>       fprintf(stderr, " -EX[,...] X=extended options\n");
>>>>  }
>>>>
>>>> +char *feature_opts[] = {
>>>> +     "sb_cksum", NULL
>>>> +};
>>>> +#define O_SB_CKSUM   0
>>>> +
>>>> +static int parse_feature_subopts(char *opts)
>>>> +{
>>>> +     char *arg;
>>>> +
>>>> +     while (*opts != '\0') {
>>>> +             switch(getsubopt(&opts, feature_opts, &arg)) {
>>>> +             case O_SB_CKSUM:
>>>> +                     cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
>>>> +                     break;
>>>> +             default:
>>>> +                     erofs_err("incorrect suboption");
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +     }
>>>> +     return 0;
>>>> +}
>>>> +
>>>>  static int parse_extended_opts(const char *opts)
>>>>  {
>>>>  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
>>>> @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char
>>> *argv[])
>>>>  {
>>>>       int opt, i;
>>>>
>>>> -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
>>>> +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
>>>>               switch (opt) {
>>>>               case 'z':
>>>>                       if (!optarg) {
>>>> @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char
>>> *argv[])
>>>>                               return opt;
>>>>                       break;
>>>>
>>>> +             case 'o':
>>>> +                     opt = parse_feature_subopts(optarg);
>>>> +                     if (opt)
>>>> +                             return opt;
>>>> +                     break;
>>>> +
>>>>               default: /* '?' */
>>>>                       return -EINVAL;
>>>>               }
>>>> @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char
>>> *argv[])
>>>>       return 0;
>>>>  }
>>>>
>>>> +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
>>>> +{
>>>> +     int offset;
>>>> +     u32 crc;
>>>> +
>>>> +     offset = offsetof(struct erofs_super_block, checksum);
>>>> +     if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
>>>> +             erofs_err("Invalid offset of checksum field: %d", offset);
>>>> +             return -1;
>>>> +     }
>>>> +     crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
>>>> +     erofs_dump("superblock checksum: 0x%x\n", crc);
>>>> +     return 0;
>>>> +}
>>>> +
>>>>  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>>>>                                 erofs_nid_t root_nid)
>>>>  {
>>>> @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct
>>> erofs_buffer_head *bh,
>>>>               .meta_blkaddr  = sbi.meta_blkaddr,
>>>>               .xattr_blkaddr = 0,
>>>>               .requirements = cpu_to_le32(sbi.requirements),
>>>> +             .features = cpu_to_le32(cfg.c_feature_flags),
>>>>       };
>>>>       const unsigned int sb_blksize =
>>>>               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
>>>> @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct
>>> erofs_buffer_head *bh,
>>>>       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
>>>>       sb.root_nid     = cpu_to_le16(root_nid);
>>>>
>>>> +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
>>>> +             u32 crc = erofs_superblock_checksum(&sb);
>>>> +             sb.checksum = cpu_to_le32(crc);
>>>> +     }
>>>> +
>>>>       buf = calloc(sb_blksize, 1);
>>>>       if (!buf) {
>>>>               erofs_err("Failed to allocate memory for sb: %s",
>>>> --
>>>> 2.9.3
>>>>
>>>

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang via Linux-erofs]

Hi Pratik,

On Sat, Aug 24, 2019 at 02:52:31PM +0530, Pratik Shinde wrote:
> Hi Gao,
> Yes I will make the suboption naming similar to that of mk2fs.

Great! :)

> 
> The reason I changed the position of 'checksum' field :
> 
> Since we are calculating the checksum of erofs_super_block structure and
> storing it in the same structure; we cannot include
> this field for actual crc calculations. Keeping it at the end makes it easy
> for me to calculate length of the data of which
> checksum needs to be calculated. I saw similar logic in other filesystems
> like ext4.

No, that is just they didn't add checksum field at the beginning of its design.
I think you can leave chksum to 0, and calculate chksum and fill it, to be specfic:

In the erofs-utils,
 1) fill .checksum to 0;
 2) calculate crc32c of the entire erofs_super_block;
 3) fill the real checksum to .checksum;

In the kernel,
 1) read .checksum to a variable;
 2) fill .checksum to 0;
 3) calculate crc32c of the entire erofs_super_block;
 4) compare the given one and the calculated one;

That is all :)

> 
> We can write our own crc32() function. :) There is no problem. I thought
> zlib already provides one & we can use it.

I think we can use crc32c() since I already wrote comments for erofs in 4.19.

Looking forward to your next version :)

Thanks,
Gao Xiang

> anyways , I will write.
> 
> --Pratik.
> 
> On Sat, Aug 24, 2019 at 2:16 PM Gao Xiang <hsiangkao@gmx.com> wrote:
> 
> > Hi Pratik,
> >
> > On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
> > > Adding code for superblock checksum calculation.
> > >
> > > This patch adds following things:
> > > 1)Handle suboptions('-o') to mkfs utility.
> >
> > Thanks for your patch. :)
> >
> > Can we use "-O feature" instead in order to keep in line with mke2fs?
> >
> > > 2)Add superblock checksum calculation(-o sb_cksum) as suboption.
> >
> > ditto. and I think we can enable sbcrc by default since it is a compat
> > feature,
> > and add "-O nosbcrc" to disable it.
> >
> > > 3)Calculate superblock checksum if feature is enabled.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> >
> > And could you please also read my following comments and fix them.
> > and I'd like to accept your erofs-utils modification in advance. :)
> >
> >
> > But now you can see we are moving EROFS out of staging now as
> > the "real" part of Linux, this is the fundamental stuff of other
> > new features if we want to develop more actively... So we can wait
> > for the final result and add this new feature to kernel then...
> >
> > > ---
> > >  include/erofs/config.h |  1 +
> > >  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
> > >  mkfs/main.c            | 53
> > +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  3 files changed, 76 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > > index 05fe6b2..40cd466 100644
> > > --- a/include/erofs/config.h
> > > +++ b/include/erofs/config.h
> > > @@ -22,6 +22,7 @@ struct erofs_configure {
> > >       char *c_src_path;
> > >       char *c_compr_alg_master;
> > >       int c_compr_level_master;
> > > +     int c_feature_flags;
> >
> > we can add this to sbi like requirements...
> >
> > >  };
> > >
> > >  extern struct erofs_configure cfg;
> > > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > > index 601b477..c9ef057 100644
> > > --- a/include/erofs_fs.h
> > > +++ b/include/erofs_fs.h
> > > @@ -20,25 +20,31 @@
> > >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> > >  #define EROFS_ALL_REQUIREMENTS
> >  EROFS_REQUIREMENT_LZ4_0PADDING
> > >
> > > +/*
> > > + * feature definations.
> > > + */
> > > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > > +
> > > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > > +     ( le32_to_cpu((super)->features) & (mask) )
> > > +
> > >  struct erofs_super_block {
> > >  /*  0 */__le32 magic;           /* in the little endian */
> > > -/*  4 */__le32 checksum;        /* crc32c(super_block) */
> > > -/*  8 */__le32 features;        /* (aka. feature_compat) */
> > > -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
> > */
> > > -/* 13 */__u8 reserved;
> > > -
> > > -/* 14 */__le16 root_nid;
> > > -/* 16 */__le64 inos;            /* total valid ino # (== f_files -
> > f_favail) */
> > > -
> > > -/* 24 */__le64 build_time;      /* inode v1 time derivation */
> > > -/* 32 */__le32 build_time_nsec;
> > > -/* 36 */__le32 blocks;          /* used for statfs */
> > > -/* 40 */__le32 meta_blkaddr;
> > > -/* 44 */__le32 xattr_blkaddr;
> > > -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > > -/* 64 */__u8 volume_name[16];   /* volume name */
> > > -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
> > > -
> > > +/*  4 */__le32 features;        /* (aka. feature_compat) */
> > > +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
> > */
> > > +/*  9 */__u8 reserved;
> > > +
> > > +/* 10 */__le16 root_nid;
> > > +/* 12 */__le64 inos;            /* total valid ino # (== f_files -
> > f_favail) */
> > > +/* 20 */__le64 build_time;      /* inode v1 time derivation */
> > > +/* 28 */__le32 build_time_nsec;
> > > +/* 32 */__le32 blocks;          /* used for statfs */
> > > +/* 36 */__le32 meta_blkaddr;
> > > +/* 40 */__le32 xattr_blkaddr;
> > > +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > > +/* 60 */__u8 volume_name[16];   /* volume name */
> > > +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
> > > +/* 80 */__le32 checksum;        /* crc32c(super_block) */
> > >  /* 84 */__u8 reserved2[44];
> >
> > Why modifying the above?
> >
> > >  } __packed;                     /* 128 bytes */
> > >
> > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > index f127fe1..26e14a3 100644
> > > --- a/mkfs/main.c
> > > +++ b/mkfs/main.c
> > > @@ -13,12 +13,14 @@
> > >  #include <limits.h>
> > >  #include <libgen.h>
> > >  #include <sys/stat.h>
> > > +#include <zlib.h>
> >
> > I have no idea that we should introduce "zlib" just for crc32c currently...
> > Maybe we can add some independent crc32 function..
> >
> > Thanks,
> > Gao Xiang
> >
> > >  #include "erofs/config.h"
> > >  #include "erofs/print.h"
> > >  #include "erofs/cache.h"
> > >  #include "erofs/inode.h"
> > >  #include "erofs/io.h"
> > >  #include "erofs/compress.h"
> > > +#include "erofs/defs.h"
> > >
> > >  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> > erofs_super_block))
> > >
> > > @@ -31,6 +33,28 @@ static void usage(void)
> > >       fprintf(stderr, " -EX[,...] X=extended options\n");
> > >  }
> > >
> > > +char *feature_opts[] = {
> > > +     "sb_cksum", NULL
> > > +};
> > > +#define O_SB_CKSUM   0
> > > +
> > > +static int parse_feature_subopts(char *opts)
> > > +{
> > > +     char *arg;
> > > +
> > > +     while (*opts != '\0') {
> > > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > > +             case O_SB_CKSUM:
> > > +                     cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
> > > +                     break;
> > > +             default:
> > > +                     erofs_err("incorrect suboption");
> > > +                     return -EINVAL;
> > > +             }
> > > +     }
> > > +     return 0;
> > > +}
> > > +
> > >  static int parse_extended_opts(const char *opts)
> > >  {
> > >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > > @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >  {
> > >       int opt, i;
> > >
> > > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> > >               switch (opt) {
> > >               case 'z':
> > >                       if (!optarg) {
> > > @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >                               return opt;
> > >                       break;
> > >
> > > +             case 'o':
> > > +                     opt = parse_feature_subopts(optarg);
> > > +                     if (opt)
> > > +                             return opt;
> > > +                     break;
> > > +
> > >               default: /* '?' */
> > >                       return -EINVAL;
> > >               }
> > > @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char
> > *argv[])
> > >       return 0;
> > >  }
> > >
> > > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > > +{
> > > +     int offset;
> > > +     u32 crc;
> > > +
> > > +     offset = offsetof(struct erofs_super_block, checksum);
> > > +     if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
> > > +             erofs_err("Invalid offset of checksum field: %d", offset);
> > > +             return -1;
> > > +     }
> > > +     crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
> > > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > > +     return 0;
> > > +}
> > > +
> > >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > >                                 erofs_nid_t root_nid)
> > >  {
> > > @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >               .meta_blkaddr  = sbi.meta_blkaddr,
> > >               .xattr_blkaddr = 0,
> > >               .requirements = cpu_to_le32(sbi.requirements),
> > > +             .features = cpu_to_le32(cfg.c_feature_flags),
> > >       };
> > >       const unsigned int sb_blksize =
> > >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> > >       sb.root_nid     = cpu_to_le16(root_nid);
> > >
> > > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > > +             u32 crc = erofs_superblock_checksum(&sb);
> > > +             sb.checksum = cpu_to_le32(crc);
> > > +     }
> > > +
> > >       buf = calloc(sb_blksize, 1);
> > >       if (!buf) {
> > >               erofs_err("Failed to allocate memory for sb: %s",
> > > --
> > > 2.9.3
> > >
> >

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

[-- Attachment #1: Type: text/plain, Size: 8934 bytes --]

Hi Gao,
Yes I will make the suboption naming similar to that of mk2fs.

The reason I changed the position of 'checksum' field :

Since we are calculating the checksum of erofs_super_block structure and
storing it in the same structure; we cannot include
this field for actual crc calculations. Keeping it at the end makes it easy
for me to calculate length of the data of which
checksum needs to be calculated. I saw similar logic in other filesystems
like ext4.

We can write our own crc32() function. :) There is no problem. I thought
zlib already provides one & we can use it.
anyways , I will write.

--Pratik.

On Sat, Aug 24, 2019 at 2:16 PM Gao Xiang <hsiangkao@gmx.com> wrote:

> Hi Pratik,
>
> On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
> > Adding code for superblock checksum calculation.
> >
> > This patch adds following things:
> > 1)Handle suboptions('-o') to mkfs utility.
>
> Thanks for your patch. :)
>
> Can we use "-O feature" instead in order to keep in line with mke2fs?
>
> > 2)Add superblock checksum calculation(-o sb_cksum) as suboption.
>
> ditto. and I think we can enable sbcrc by default since it is a compat
> feature,
> and add "-O nosbcrc" to disable it.
>
> > 3)Calculate superblock checksum if feature is enabled.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
>
> And could you please also read my following comments and fix them.
> and I'd like to accept your erofs-utils modification in advance. :)
>
>
> But now you can see we are moving EROFS out of staging now as
> the "real" part of Linux, this is the fundamental stuff of other
> new features if we want to develop more actively... So we can wait
> for the final result and add this new feature to kernel then...
>
> > ---
> >  include/erofs/config.h |  1 +
> >  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
> >  mkfs/main.c            | 53
> +++++++++++++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 76 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/erofs/config.h b/include/erofs/config.h
> > index 05fe6b2..40cd466 100644
> > --- a/include/erofs/config.h
> > +++ b/include/erofs/config.h
> > @@ -22,6 +22,7 @@ struct erofs_configure {
> >       char *c_src_path;
> >       char *c_compr_alg_master;
> >       int c_compr_level_master;
> > +     int c_feature_flags;
>
> we can add this to sbi like requirements...
>
> >  };
> >
> >  extern struct erofs_configure cfg;
> > diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> > index 601b477..c9ef057 100644
> > --- a/include/erofs_fs.h
> > +++ b/include/erofs_fs.h
> > @@ -20,25 +20,31 @@
> >  #define EROFS_REQUIREMENT_LZ4_0PADDING       0x00000001
> >  #define EROFS_ALL_REQUIREMENTS
>  EROFS_REQUIREMENT_LZ4_0PADDING
> >
> > +/*
> > + * feature definations.
> > + */
> > +#define EROFS_FEATURE_SB_CHKSUM              0x0001
> > +
> > +#define EROFS_HAS_COMPAT_FEATURE(super,mask) \
> > +     ( le32_to_cpu((super)->features) & (mask) )
> > +
> >  struct erofs_super_block {
> >  /*  0 */__le32 magic;           /* in the little endian */
> > -/*  4 */__le32 checksum;        /* crc32c(super_block) */
> > -/*  8 */__le32 features;        /* (aka. feature_compat) */
> > -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
> */
> > -/* 13 */__u8 reserved;
> > -
> > -/* 14 */__le16 root_nid;
> > -/* 16 */__le64 inos;            /* total valid ino # (== f_files -
> f_favail) */
> > -
> > -/* 24 */__le64 build_time;      /* inode v1 time derivation */
> > -/* 32 */__le32 build_time_nsec;
> > -/* 36 */__le32 blocks;          /* used for statfs */
> > -/* 40 */__le32 meta_blkaddr;
> > -/* 44 */__le32 xattr_blkaddr;
> > -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > -/* 64 */__u8 volume_name[16];   /* volume name */
> > -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
> > -
> > +/*  4 */__le32 features;        /* (aka. feature_compat) */
> > +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only
> */
> > +/*  9 */__u8 reserved;
> > +
> > +/* 10 */__le16 root_nid;
> > +/* 12 */__le64 inos;            /* total valid ino # (== f_files -
> f_favail) */
> > +/* 20 */__le64 build_time;      /* inode v1 time derivation */
> > +/* 28 */__le32 build_time_nsec;
> > +/* 32 */__le32 blocks;          /* used for statfs */
> > +/* 36 */__le32 meta_blkaddr;
> > +/* 40 */__le32 xattr_blkaddr;
> > +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
> > +/* 60 */__u8 volume_name[16];   /* volume name */
> > +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
> > +/* 80 */__le32 checksum;        /* crc32c(super_block) */
> >  /* 84 */__u8 reserved2[44];
>
> Why modifying the above?
>
> >  } __packed;                     /* 128 bytes */
> >
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index f127fe1..26e14a3 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -13,12 +13,14 @@
> >  #include <limits.h>
> >  #include <libgen.h>
> >  #include <sys/stat.h>
> > +#include <zlib.h>
>
> I have no idea that we should introduce "zlib" just for crc32c currently...
> Maybe we can add some independent crc32 function..
>
> Thanks,
> Gao Xiang
>
> >  #include "erofs/config.h"
> >  #include "erofs/print.h"
> >  #include "erofs/cache.h"
> >  #include "erofs/inode.h"
> >  #include "erofs/io.h"
> >  #include "erofs/compress.h"
> > +#include "erofs/defs.h"
> >
> >  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> erofs_super_block))
> >
> > @@ -31,6 +33,28 @@ static void usage(void)
> >       fprintf(stderr, " -EX[,...] X=extended options\n");
> >  }
> >
> > +char *feature_opts[] = {
> > +     "sb_cksum", NULL
> > +};
> > +#define O_SB_CKSUM   0
> > +
> > +static int parse_feature_subopts(char *opts)
> > +{
> > +     char *arg;
> > +
> > +     while (*opts != '\0') {
> > +             switch(getsubopt(&opts, feature_opts, &arg)) {
> > +             case O_SB_CKSUM:
> > +                     cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
> > +                     break;
> > +             default:
> > +                     erofs_err("incorrect suboption");
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int parse_extended_opts(const char *opts)
> >  {
> >  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> > @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >  {
> >       int opt, i;
> >
> > -     while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> > +     while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
> >               switch (opt) {
> >               case 'z':
> >                       if (!optarg) {
> > @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >                               return opt;
> >                       break;
> >
> > +             case 'o':
> > +                     opt = parse_feature_subopts(optarg);
> > +                     if (opt)
> > +                             return opt;
> > +                     break;
> > +
> >               default: /* '?' */
> >                       return -EINVAL;
> >               }
> > @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char
> *argv[])
> >       return 0;
> >  }
> >
> > +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> > +{
> > +     int offset;
> > +     u32 crc;
> > +
> > +     offset = offsetof(struct erofs_super_block, checksum);
> > +     if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
> > +             erofs_err("Invalid offset of checksum field: %d", offset);
> > +             return -1;
> > +     }
> > +     crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
> > +     erofs_dump("superblock checksum: 0x%x\n", crc);
> > +     return 0;
> > +}
> > +
> >  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> >                                 erofs_nid_t root_nid)
> >  {
> > @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> >               .meta_blkaddr  = sbi.meta_blkaddr,
> >               .xattr_blkaddr = 0,
> >               .requirements = cpu_to_le32(sbi.requirements),
> > +             .features = cpu_to_le32(cfg.c_feature_flags),
> >       };
> >       const unsigned int sb_blksize =
> >               round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> >       sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
> >       sb.root_nid     = cpu_to_le16(root_nid);
> >
> > +     if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> > +             u32 crc = erofs_superblock_checksum(&sb);
> > +             sb.checksum = cpu_to_le32(crc);
> > +     }
> > +
> >       buf = calloc(sb_blksize, 1);
> >       if (!buf) {
> >               erofs_err("Failed to allocate memory for sb: %s",
> > --
> > 2.9.3
> >
>

[-- Attachment #2: Type: text/html, Size: 11578 bytes --]

Re: [PATCH] erofs-utils: code for superblock checksum calculation.

[Gao Xiang]

Hi Pratik,

On Sat, Aug 24, 2019 at 01:11:58PM +0530, Pratik Shinde wrote:
> Adding code for superblock checksum calculation.
>
> This patch adds following things:
> 1)Handle suboptions('-o') to mkfs utility.

Thanks for your patch. :)

Can we use "-O feature" instead in order to keep in line with mke2fs?

> 2)Add superblock checksum calculation(-o sb_cksum) as suboption.

ditto. and I think we can enable sbcrc by default since it is a compat feature,
and add "-O nosbcrc" to disable it.

> 3)Calculate superblock checksum if feature is enabled.
>
> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>

And could you please also read my following comments and fix them.
and I'd like to accept your erofs-utils modification in advance. :)


But now you can see we are moving EROFS out of staging now as
the "real" part of Linux, this is the fundamental stuff of other
new features if we want to develop more actively... So we can wait
for the final result and add this new feature to kernel then...

> ---
>  include/erofs/config.h |  1 +
>  include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
>  mkfs/main.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 76 insertions(+), 18 deletions(-)
>
> diff --git a/include/erofs/config.h b/include/erofs/config.h
> index 05fe6b2..40cd466 100644
> --- a/include/erofs/config.h
> +++ b/include/erofs/config.h
> @@ -22,6 +22,7 @@ struct erofs_configure {
>  	char *c_src_path;
>  	char *c_compr_alg_master;
>  	int c_compr_level_master;
> +	int c_feature_flags;

we can add this to sbi like requirements...

>  };
>
>  extern struct erofs_configure cfg;
> diff --git a/include/erofs_fs.h b/include/erofs_fs.h
> index 601b477..c9ef057 100644
> --- a/include/erofs_fs.h
> +++ b/include/erofs_fs.h
> @@ -20,25 +20,31 @@
>  #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
>  #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
>
> +/*
> + * feature definations.
> + */
> +#define EROFS_FEATURE_SB_CHKSUM		0x0001
> +
> +#define EROFS_HAS_COMPAT_FEATURE(super,mask)	\
> +	( le32_to_cpu((super)->features) & (mask) )
> +
>  struct erofs_super_block {
>  /*  0 */__le32 magic;           /* in the little endian */
> -/*  4 */__le32 checksum;        /* crc32c(super_block) */
> -/*  8 */__le32 features;        /* (aka. feature_compat) */
> -/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
> -/* 13 */__u8 reserved;
> -
> -/* 14 */__le16 root_nid;
> -/* 16 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
> -
> -/* 24 */__le64 build_time;      /* inode v1 time derivation */
> -/* 32 */__le32 build_time_nsec;
> -/* 36 */__le32 blocks;          /* used for statfs */
> -/* 40 */__le32 meta_blkaddr;
> -/* 44 */__le32 xattr_blkaddr;
> -/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
> -/* 64 */__u8 volume_name[16];   /* volume name */
> -/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
> -
> +/*  4 */__le32 features;        /* (aka. feature_compat) */
> +/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
> +/*  9 */__u8 reserved;
> +
> +/* 10 */__le16 root_nid;
> +/* 12 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
> +/* 20 */__le64 build_time;      /* inode v1 time derivation */
> +/* 28 */__le32 build_time_nsec;
> +/* 32 */__le32 blocks;          /* used for statfs */
> +/* 36 */__le32 meta_blkaddr;
> +/* 40 */__le32 xattr_blkaddr;
> +/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
> +/* 60 */__u8 volume_name[16];   /* volume name */
> +/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
> +/* 80 */__le32 checksum;        /* crc32c(super_block) */
>  /* 84 */__u8 reserved2[44];

Why modifying the above?

>  } __packed;                     /* 128 bytes */
>
> diff --git a/mkfs/main.c b/mkfs/main.c
> index f127fe1..26e14a3 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -13,12 +13,14 @@
>  #include <limits.h>
>  #include <libgen.h>
>  #include <sys/stat.h>
> +#include <zlib.h>

I have no idea that we should introduce "zlib" just for crc32c currently...
Maybe we can add some independent crc32 function..

Thanks,
Gao Xiang

>  #include "erofs/config.h"
>  #include "erofs/print.h"
>  #include "erofs/cache.h"
>  #include "erofs/inode.h"
>  #include "erofs/io.h"
>  #include "erofs/compress.h"
> +#include "erofs/defs.h"
>
>  #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct erofs_super_block))
>
> @@ -31,6 +33,28 @@ static void usage(void)
>  	fprintf(stderr, " -EX[,...] X=extended options\n");
>  }
>
> +char *feature_opts[] = {
> +	"sb_cksum", NULL
> +};
> +#define O_SB_CKSUM	0
> +
> +static int parse_feature_subopts(char *opts)
> +{
> +	char *arg;
> +
> +	while (*opts != '\0') {
> +		switch(getsubopt(&opts, feature_opts, &arg)) {
> +		case O_SB_CKSUM:
> +			cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
> +			break;
> +		default:
> +			erofs_err("incorrect suboption");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int parse_extended_opts(const char *opts)
>  {
>  #define MATCH_EXTENTED_OPT(opt, token, keylen) \
> @@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  {
>  	int opt, i;
>
> -	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> +	while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
>  		switch (opt) {
>  		case 'z':
>  			if (!optarg) {
> @@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  				return opt;
>  			break;
>
> +		case 'o':
> +			opt = parse_feature_subopts(optarg);
> +			if (opt)
> +				return opt;
> +			break;
> +
>  		default: /* '?' */
>  			return -EINVAL;
>  		}
> @@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  	return 0;
>  }
>
> +u32 erofs_superblock_checksum(struct erofs_super_block *sb)
> +{
> +	int offset;
> +	u32 crc;
> +
> +	offset = offsetof(struct erofs_super_block, checksum);
> +	if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
> +		erofs_err("Invalid offset of checksum field: %d", offset);
> +		return -1;
> +	}
> +	crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
> +	erofs_dump("superblock checksum: 0x%x\n", crc);
> +	return 0;
> +}
> +
>  int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  				  erofs_nid_t root_nid)
>  {
> @@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  		.meta_blkaddr  = sbi.meta_blkaddr,
>  		.xattr_blkaddr = 0,
>  		.requirements = cpu_to_le32(sbi.requirements),
> +		.features = cpu_to_le32(cfg.c_feature_flags),
>  	};
>  	const unsigned int sb_blksize =
>  		round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> @@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
>  	sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
>  	sb.root_nid     = cpu_to_le16(root_nid);
>
> +	if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
> +		u32 crc = erofs_superblock_checksum(&sb);
> +		sb.checksum = cpu_to_le32(crc);
> +	}
> +
>  	buf = calloc(sb_blksize, 1);
>  	if (!buf) {
>  		erofs_err("Failed to allocate memory for sb: %s",
> --
> 2.9.3
>

[PATCH] erofs-utils: code for superblock checksum calculation.

[Pratik Shinde]

Adding code for superblock checksum calculation.

This patch adds following things:
1)Handle suboptions('-o') to mkfs utility.
2)Add superblock checksum calculation(-o sb_cksum) as suboption.
3)Calculate superblock checksum if feature is enabled.

Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
---
 include/erofs/config.h |  1 +
 include/erofs_fs.h     | 40 +++++++++++++++++++++----------------
 mkfs/main.c            | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/include/erofs/config.h b/include/erofs/config.h
index 05fe6b2..40cd466 100644
--- a/include/erofs/config.h
+++ b/include/erofs/config.h
@@ -22,6 +22,7 @@ struct erofs_configure {
 	char *c_src_path;
 	char *c_compr_alg_master;
 	int c_compr_level_master;
+	int c_feature_flags;
 };
 
 extern struct erofs_configure cfg;
diff --git a/include/erofs_fs.h b/include/erofs_fs.h
index 601b477..c9ef057 100644
--- a/include/erofs_fs.h
+++ b/include/erofs_fs.h
@@ -20,25 +20,31 @@
 #define EROFS_REQUIREMENT_LZ4_0PADDING	0x00000001
 #define EROFS_ALL_REQUIREMENTS		EROFS_REQUIREMENT_LZ4_0PADDING
 
+/*
+ * feature definations.
+ */
+#define EROFS_FEATURE_SB_CHKSUM		0x0001
+
+#define EROFS_HAS_COMPAT_FEATURE(super,mask)	\
+	( le32_to_cpu((super)->features) & (mask) )
+
 struct erofs_super_block {
 /*  0 */__le32 magic;           /* in the little endian */
-/*  4 */__le32 checksum;        /* crc32c(super_block) */
-/*  8 */__le32 features;        /* (aka. feature_compat) */
-/* 12 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
-/* 13 */__u8 reserved;
-
-/* 14 */__le16 root_nid;
-/* 16 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
-
-/* 24 */__le64 build_time;      /* inode v1 time derivation */
-/* 32 */__le32 build_time_nsec;
-/* 36 */__le32 blocks;          /* used for statfs */
-/* 40 */__le32 meta_blkaddr;
-/* 44 */__le32 xattr_blkaddr;
-/* 48 */__u8 uuid[16];          /* 128-bit uuid for volume */
-/* 64 */__u8 volume_name[16];   /* volume name */
-/* 80 */__le32 requirements;    /* (aka. feature_incompat) */
-
+/*  4 */__le32 features;        /* (aka. feature_compat) */
+/*  8 */__u8 blkszbits;         /* support block_size == PAGE_SIZE only */
+/*  9 */__u8 reserved;
+
+/* 10 */__le16 root_nid;
+/* 12 */__le64 inos;            /* total valid ino # (== f_files - f_favail) */
+/* 20 */__le64 build_time;      /* inode v1 time derivation */
+/* 28 */__le32 build_time_nsec;
+/* 32 */__le32 blocks;          /* used for statfs */
+/* 36 */__le32 meta_blkaddr;
+/* 40 */__le32 xattr_blkaddr;
+/* 44 */__u8 uuid[16];          /* 128-bit uuid for volume */
+/* 60 */__u8 volume_name[16];   /* volume name */
+/* 76 */__le32 requirements;    /* (aka. feature_incompat) */
+/* 80 */__le32 checksum;        /* crc32c(super_block) */
 /* 84 */__u8 reserved2[44];
 } __packed;                     /* 128 bytes */
 
diff --git a/mkfs/main.c b/mkfs/main.c
index f127fe1..26e14a3 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -13,12 +13,14 @@
 #include <limits.h>
 #include <libgen.h>
 #include <sys/stat.h>
+#include <zlib.h>
 #include "erofs/config.h"
 #include "erofs/print.h"
 #include "erofs/cache.h"
 #include "erofs/inode.h"
 #include "erofs/io.h"
 #include "erofs/compress.h"
+#include "erofs/defs.h"
 
 #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct erofs_super_block))
 
@@ -31,6 +33,28 @@ static void usage(void)
 	fprintf(stderr, " -EX[,...] X=extended options\n");
 }
 
+char *feature_opts[] = {
+	"sb_cksum", NULL
+};
+#define O_SB_CKSUM	0
+
+static int parse_feature_subopts(char *opts)
+{
+	char *arg;
+
+	while (*opts != '\0') {
+		switch(getsubopt(&opts, feature_opts, &arg)) {
+		case O_SB_CKSUM:
+			cfg.c_feature_flags |= EROFS_FEATURE_SB_CHKSUM;
+			break;
+		default:
+			erofs_err("incorrect suboption");
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static int parse_extended_opts(const char *opts)
 {
 #define MATCH_EXTENTED_OPT(opt, token, keylen) \
@@ -79,7 +103,7 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 {
 	int opt, i;
 
-	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
+	while ((opt = getopt(argc, argv, "d:z:E:o:")) != -1) {
 		switch (opt) {
 		case 'z':
 			if (!optarg) {
@@ -113,6 +137,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 				return opt;
 			break;
 
+		case 'o':
+			opt = parse_feature_subopts(optarg);
+			if (opt)
+				return opt;
+			break;
+
 		default: /* '?' */
 			return -EINVAL;
 		}
@@ -144,6 +174,21 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
 	return 0;
 }
 
+u32 erofs_superblock_checksum(struct erofs_super_block *sb)
+{
+	int offset;
+	u32 crc;
+
+	offset = offsetof(struct erofs_super_block, checksum);
+	if (offset < 0 || offset > sizeof(struct erofs_super_block)) {
+		erofs_err("Invalid offset of checksum field: %d", offset);
+		return -1;
+	}
+	crc = crc32(~0, (const unsigned char *)sb,(size_t)offset);
+	erofs_dump("superblock checksum: 0x%x\n", crc);
+	return 0;
+}
+
 int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 				  erofs_nid_t root_nid)
 {
@@ -155,6 +200,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 		.meta_blkaddr  = sbi.meta_blkaddr,
 		.xattr_blkaddr = 0,
 		.requirements = cpu_to_le32(sbi.requirements),
+		.features = cpu_to_le32(cfg.c_feature_flags),
 	};
 	const unsigned int sb_blksize =
 		round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
@@ -169,6 +215,11 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
 	sb.blocks       = cpu_to_le32(erofs_mapbh(NULL, true));
 	sb.root_nid     = cpu_to_le16(root_nid);
 
+	if (EROFS_HAS_COMPAT_FEATURE(&sb, EROFS_FEATURE_SB_CHKSUM)) {
+		u32 crc = erofs_superblock_checksum(&sb);
+		sb.checksum = cpu_to_le32(crc);
+	}
+
 	buf = calloc(sb_blksize, 1);
 	if (!buf) {
 		erofs_err("Failed to allocate memory for sb: %s",
-- 
2.9.3