From: Pratik Shinde <pratikshinde320@gmail.com>
To: Gao Xiang <hsiangkao@aol.com>
Cc: miaoxie@huawei.com, linux-erofs@lists.ozlabs.org
Subject: Re: [PATCH] erofs-utils: code for superblock checksum calculation.
Date: Sat, 24 Aug 2019 20:26:28 +0530 [thread overview]
Message-ID: <CAGu0czSznsz7w90okOKa4bXhfy3ou4X=7pHKLmWw4WffFVh5wQ@mail.gmail.com> (raw)
In-Reply-To: <20190824140012.GB19943@hsiangkao-HP-ZHAN-66-Pro-G1>
[-- 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 --]
next prev parent reply other threads:[~2019-08-24 14:57 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-24 12:38 [PATCH] erofs-utils: code for superblock checksum calculation Pratik Shinde
2019-08-24 14:00 ` Gao Xiang via Linux-erofs
2019-08-24 14:12 ` Gao Xiang via Linux-erofs
2019-08-24 14:56 ` Pratik Shinde [this message]
2019-08-24 15:16 ` Gao Xiang via Linux-erofs
2019-10-06 5:39 ` Gao Xiang via Linux-erofs
2019-10-09 6:29 ` Pratik Shinde
2019-10-09 6:57 ` Gao Xiang
2019-10-09 8:24 ` Pratik Shinde
2019-10-09 8:48 ` Gao Xiang
2019-10-09 14:14 ` Pratik Shinde
2019-10-09 14:27 ` Gao Xiang via Linux-erofs
-- strict thread matches above, loose matches on Subject: below --
2019-08-24 7:41 Pratik Shinde
2019-08-24 8:46 ` Gao Xiang
2019-08-24 9:22 ` Pratik Shinde
2019-08-24 9:49 ` Gao Xiang via Linux-erofs
2019-08-24 10:01 ` Chao Yu
2019-08-24 10:05 ` Pratik Shinde
2019-08-24 10:14 ` Gao Xiang via Linux-erofs
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGu0czSznsz7w90okOKa4bXhfy3ou4X=7pHKLmWw4WffFVh5wQ@mail.gmail.com' \
--to=pratikshinde320@gmail.com \
--cc=hsiangkao@aol.com \
--cc=linux-erofs@lists.ozlabs.org \
--cc=miaoxie@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).