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