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, 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 > > 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 > > >