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