* [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks.
@ 2019-10-14 14:59 Pratik Shinde
2019-10-14 23:45 ` Gao Xiang via Linux-erofs
0 siblings, 1 reply; 5+ messages in thread
From: Pratik Shinde @ 2019-10-14 14:59 UTC (permalink / raw)
To: linux-erofs, bluce.liguifu, miaoxie, fangwei1
Added code for calculating crc of erofs blocks (4K size).for now it calculates
checksum of first block. but can modified to calculate crc for any no. of blocks
modified patch based on review comments.
Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
---
include/erofs/internal.h | 1 +
include/erofs/io.h | 8 +++++
lib/io.c | 27 +++++++++++++++++
mkfs/main.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 112 insertions(+)
diff --git a/include/erofs/internal.h b/include/erofs/internal.h
index 5384946..53335bc 100644
--- a/include/erofs/internal.h
+++ b/include/erofs/internal.h
@@ -55,6 +55,7 @@ struct erofs_sb_info {
u32 feature_incompat;
u64 build_time;
u32 build_time_nsec;
+ u32 feature;
};
/* global sbi */
diff --git a/include/erofs/io.h b/include/erofs/io.h
index 9775047..e0ca8d9 100644
--- a/include/erofs/io.h
+++ b/include/erofs/io.h
@@ -19,6 +19,7 @@
int dev_open(const char *devname);
void dev_close(void);
int dev_write(const void *buf, u64 offset, size_t len);
+int dev_read(void *buf, u64 offset, size_t len);
int dev_fillzero(u64 offset, size_t len, bool padding);
int dev_fsync(void);
int dev_resize(erofs_blk_t nblocks);
@@ -31,5 +32,12 @@ static inline int blk_write(const void *buf, erofs_blk_t blkaddr,
blknr_to_addr(nblocks));
}
+static inline int blk_read(void *buf, erofs_blk_t start,
+ u32 nblocks)
+{
+ return dev_read(buf, blknr_to_addr(start),
+ blknr_to_addr(nblocks));
+}
+
#endif
diff --git a/lib/io.c b/lib/io.c
index 7f5f94d..52f9424 100644
--- a/lib/io.c
+++ b/lib/io.c
@@ -207,3 +207,30 @@ int dev_resize(unsigned int blocks)
return dev_fillzero(st.st_size, length, true);
}
+int dev_read(void *buf, u64 offset, size_t len)
+{
+ int ret;
+
+ if (cfg.c_dry_run)
+ return 0;
+
+ if (!buf) {
+ erofs_err("buf is NULL");
+ return -EINVAL;
+ }
+ if (offset >= erofs_devsz || len > erofs_devsz ||
+ offset > erofs_devsz - len) {
+ erofs_err("read posion[%" PRIu64 ", %zd] is too large beyond"
+ "the end of device(%" PRIu64 ").",
+ offset, len, erofs_devsz);
+ return -EINVAL;
+ }
+
+ ret = pread64(erofs_devfd, buf, len, (off64_t)offset);
+ if (ret != (int)len) {
+ erofs_err("Failed to read data from device - %s:[%" PRIu64 ", %zd].",
+ erofs_devname, offset, len);
+ return -errno;
+ }
+ return 0;
+}
diff --git a/mkfs/main.c b/mkfs/main.c
index 91a018f..baaf02a 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -22,6 +22,10 @@
#define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct erofs_super_block))
+/* number of blocks for calculating checksum */
+#define EROFS_CKSUM_BLOCKS 1
+#define EROFS_FEATURE_SB_CHKSUM 0x0001
+
static void usage(void)
{
fprintf(stderr, "usage: [options] FILE DIRECTORY\n\n");
@@ -85,6 +89,10 @@ static int parse_extended_opts(const char *opts)
return -EINVAL;
cfg.c_force_inodeversion = FORCE_INODE_EXTENDED;
}
+
+ if (MATCH_EXTENTED_OPT("nocrc", token, keylen)) {
+ sbi.feature &= ~EROFS_FEATURE_SB_CHKSUM;
+ }
}
return 0;
}
@@ -180,6 +188,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
.meta_blkaddr = sbi.meta_blkaddr,
.xattr_blkaddr = 0,
.feature_incompat = cpu_to_le32(sbi.feature_incompat),
+ .checksum = 0
};
const unsigned int sb_blksize =
round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
@@ -202,6 +211,70 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
return 0;
}
+#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);
+ }
+ }
+ return crc;
+}
+
+/* calculate checksum for first n blocks */
+u32 erofs_calc_blk_checksum(erofs_blk_t nblks, u32 *crc)
+{
+ char *buf;
+ int err = 0;
+
+ buf = malloc(nblks * EROFS_BLKSIZ);
+ err = blk_read(buf, 0, nblks);
+ if (err) {
+ erofs_err("Failed to calculate erofs checksum - %s",
+ erofs_strerror(err));
+ return err;
+ }
+ *crc = crc32c(0, (const unsigned char *)buf, nblks * EROFS_BLKSIZ);
+ free(buf);
+ return 0;
+}
+
+void erofs_write_checksum()
+{
+ struct erofs_super_block *sb;
+ char buf[EROFS_BLKSIZ];
+ int ret = 0;
+ u32 crc;
+
+ ret = erofs_calc_blk_checksum(EROFS_CKSUM_BLOCKS, &crc);
+ if (ret) {
+ return;
+ }
+ ret = blk_read(buf, 0, 1);
+ if (ret) {
+ erofs_err("error reading super-block structure");
+ return;
+ }
+
+ sb = (struct erofs_super_block *)((u8 *)buf + EROFS_SUPER_OFFSET);
+ if (le32_to_cpu(sb->magic) != EROFS_SUPER_MAGIC_V1) {
+ erofs_err("not an erofs image");
+ return;
+ }
+ sb->checksum = cpu_to_le32(crc);
+ ret = blk_write(buf, 0, 1);
+ if (ret) {
+ erofs_err("error writing 0th block to disk - %s",
+ erofs_strerror(ret));
+ return;
+ }
+}
+
int main(int argc, char **argv)
{
int err = 0;
@@ -217,6 +290,7 @@ int main(int argc, char **argv)
cfg.c_legacy_compress = false;
sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
+ sbi.feature = EROFS_FEATURE_SB_CHKSUM;
err = mkfs_parse_options_cfg(argc, argv);
if (err) {
@@ -301,6 +375,8 @@ int main(int argc, char **argv)
err = -EIO;
else
err = dev_resize(nblocks);
+ if (sbi.feature & EROFS_FEATURE_SB_CHKSUM)
+ erofs_write_checksum();
exit:
z_erofs_compress_exit();
dev_close();
--
2.9.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks.
2019-10-14 14:59 [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks Pratik Shinde
@ 2019-10-14 23:45 ` Gao Xiang via Linux-erofs
2019-10-15 1:33 ` Gao Xiang
2019-10-15 2:00 ` Pratik Shinde
0 siblings, 2 replies; 5+ messages in thread
From: Gao Xiang via Linux-erofs @ 2019-10-14 23:45 UTC (permalink / raw)
To: Pratik Shinde; +Cc: miaoxie, linux-erofs
Hi Pratik,
Some nitpick comments... Let me know if you have other thoughts
On Mon, Oct 14, 2019 at 08:29:43PM +0530, Pratik Shinde wrote:
> Added code for calculating crc of erofs blocks (4K size).for now it calculates
> checksum of first block. but can modified to calculate crc for any no. of blocks
>
> modified patch based on review comments.
>
> Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> ---
> include/erofs/internal.h | 1 +
> include/erofs/io.h | 8 +++++
> lib/io.c | 27 +++++++++++++++++
> mkfs/main.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 112 insertions(+)
>
> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> index 5384946..53335bc 100644
> --- a/include/erofs/internal.h
> +++ b/include/erofs/internal.h
> @@ -55,6 +55,7 @@ struct erofs_sb_info {
> u32 feature_incompat;
> u64 build_time;
> u32 build_time_nsec;
> + u32 feature;
> };
>
> /* global sbi */
> diff --git a/include/erofs/io.h b/include/erofs/io.h
> index 9775047..e0ca8d9 100644
> --- a/include/erofs/io.h
> +++ b/include/erofs/io.h
> @@ -19,6 +19,7 @@
> int dev_open(const char *devname);
> void dev_close(void);
> int dev_write(const void *buf, u64 offset, size_t len);
> +int dev_read(void *buf, u64 offset, size_t len);
> int dev_fillzero(u64 offset, size_t len, bool padding);
> int dev_fsync(void);
> int dev_resize(erofs_blk_t nblocks);
> @@ -31,5 +32,12 @@ static inline int blk_write(const void *buf, erofs_blk_t blkaddr,
> blknr_to_addr(nblocks));
> }
>
> +static inline int blk_read(void *buf, erofs_blk_t start,
> + u32 nblocks)
> +{
> + return dev_read(buf, blknr_to_addr(start),
> + blknr_to_addr(nblocks));
> +}
> +
> #endif
>
> diff --git a/lib/io.c b/lib/io.c
> index 7f5f94d..52f9424 100644
> --- a/lib/io.c
> +++ b/lib/io.c
> @@ -207,3 +207,30 @@ int dev_resize(unsigned int blocks)
> return dev_fillzero(st.st_size, length, true);
> }
>
> +int dev_read(void *buf, u64 offset, size_t len)
> +{
> + int ret;
> +
> + if (cfg.c_dry_run)
> + return 0;
> +
> + if (!buf) {
> + erofs_err("buf is NULL");
> + return -EINVAL;
> + }
> + if (offset >= erofs_devsz || len > erofs_devsz ||
> + offset > erofs_devsz - len) {
> + erofs_err("read posion[%" PRIu64 ", %zd] is too large beyond"
> + "the end of device(%" PRIu64 ").",
> + offset, len, erofs_devsz);
> + return -EINVAL;
> + }
> +
> + ret = pread64(erofs_devfd, buf, len, (off64_t)offset);
> + if (ret != (int)len) {
> + erofs_err("Failed to read data from device - %s:[%" PRIu64 ", %zd].",
> + erofs_devname, offset, len);
> + return -errno;
> + }
> + return 0;
> +}
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 91a018f..baaf02a 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -22,6 +22,10 @@
>
> #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct erofs_super_block))
>
> +/* number of blocks for calculating checksum */
> +#define EROFS_CKSUM_BLOCKS 1
> +#define EROFS_FEATURE_SB_CHKSUM 0x0001
How about Moving EROFS_FEATURE_SB_CHKSUM to erofs_fs.h since it's
an on-disk definition,
> +
> static void usage(void)
> {
> fprintf(stderr, "usage: [options] FILE DIRECTORY\n\n");
> @@ -85,6 +89,10 @@ static int parse_extended_opts(const char *opts)
> return -EINVAL;
> cfg.c_force_inodeversion = FORCE_INODE_EXTENDED;
> }
> +
> + if (MATCH_EXTENTED_OPT("nocrc", token, keylen)) {
> + sbi.feature &= ~EROFS_FEATURE_SB_CHKSUM;
> + }
> }
> return 0;
> }
> @@ -180,6 +188,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> .meta_blkaddr = sbi.meta_blkaddr,
> .xattr_blkaddr = 0,
> .feature_incompat = cpu_to_le32(sbi.feature_incompat),
> + .checksum = 0
> };
> const unsigned int sb_blksize =
> round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> @@ -202,6 +211,70 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> return 0;
> }
>
> +#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);
> + }
> + }
> + return crc;
> +}
> +
> +/* calculate checksum for first n blocks */
> +u32 erofs_calc_blk_checksum(erofs_blk_t nblks, u32 *crc)
> +{
> + char *buf;
> + int err = 0;
> +
> + buf = malloc(nblks * EROFS_BLKSIZ);
> + err = blk_read(buf, 0, nblks);
> + if (err) {
> + erofs_err("Failed to calculate erofs checksum - %s",
> + erofs_strerror(err));
> + return err;
> + }
> + *crc = crc32c(0, (const unsigned char *)buf, nblks * EROFS_BLKSIZ);
> + free(buf);
> + return 0;
> +}
> +
> +void erofs_write_checksum()
How about naming write_sb_checksum?
My idea is that this is a checksum in super block (rather than
a checksum only for super block [0th block])
Let me know if you have another thought...
> +{
> + struct erofs_super_block *sb;
> + char buf[EROFS_BLKSIZ];
> + int ret = 0;
> + u32 crc;
> +
> + ret = erofs_calc_blk_checksum(EROFS_CKSUM_BLOCKS, &crc);
> + if (ret) {
> + return;
> + }
> + ret = blk_read(buf, 0, 1);
> + if (ret) {
> + erofs_err("error reading super-block structure");
> + return;
> + }
> +
> + sb = (struct erofs_super_block *)((u8 *)buf + EROFS_SUPER_OFFSET);
> + if (le32_to_cpu(sb->magic) != EROFS_SUPER_MAGIC_V1) {
> + erofs_err("not an erofs image");
As the previous comments, I am little care about these print messages
since users will only see this and "error reading super-block structure"
"not an erofs image" makes confused for them... (They don't know what
the internal process is doing)
BTW, it looks good to me as a whole... Do you have some time on
kernel side as well? :)
Thanks,
Gao Xiang
> + return;
> + }
> + sb->checksum = cpu_to_le32(crc);
> + ret = blk_write(buf, 0, 1);
> + if (ret) {
> + erofs_err("error writing 0th block to disk - %s",
> + erofs_strerror(ret));
> + return;
> + }
> +}
> +
> int main(int argc, char **argv)
> {
> int err = 0;
> @@ -217,6 +290,7 @@ int main(int argc, char **argv)
>
> cfg.c_legacy_compress = false;
> sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
> + sbi.feature = EROFS_FEATURE_SB_CHKSUM;
>
> err = mkfs_parse_options_cfg(argc, argv);
> if (err) {
> @@ -301,6 +375,8 @@ int main(int argc, char **argv)
> err = -EIO;
> else
> err = dev_resize(nblocks);
> + if (sbi.feature & EROFS_FEATURE_SB_CHKSUM)
> + erofs_write_checksum();
> exit:
> z_erofs_compress_exit();
> dev_close();
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks.
2019-10-14 23:45 ` Gao Xiang via Linux-erofs
@ 2019-10-15 1:33 ` Gao Xiang
2019-10-15 2:00 ` Pratik Shinde
1 sibling, 0 replies; 5+ messages in thread
From: Gao Xiang @ 2019-10-15 1:33 UTC (permalink / raw)
To: Gao Xiang; +Cc: linux-erofs, miaoxie
On Tue, Oct 15, 2019 at 07:45:17AM +0800, Gao Xiang via Linux-erofs wrote:
> Hi Pratik,
>
> Some nitpick comments... Let me know if you have other thoughts
>
> On Mon, Oct 14, 2019 at 08:29:43PM +0530, Pratik Shinde wrote:
> > Added code for calculating crc of erofs blocks (4K size).for now it calculates
> > checksum of first block. but can modified to calculate crc for any no. of blocks
> >
> > modified patch based on review comments.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > ---
> > include/erofs/internal.h | 1 +
> > include/erofs/io.h | 8 +++++
> > lib/io.c | 27 +++++++++++++++++
> > mkfs/main.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 112 insertions(+)
> >
> > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > index 5384946..53335bc 100644
> > --- a/include/erofs/internal.h
> > +++ b/include/erofs/internal.h
> > @@ -55,6 +55,7 @@ struct erofs_sb_info {
> > u32 feature_incompat;
> > u64 build_time;
> > u32 build_time_nsec;
> > + u32 feature;
> > };
Oh, I missed one thing, we need to add another field to on-disk
erofs_super_block as well.
21 /* 128-byte erofs on-disk super block */
22 struct erofs_super_block {
23 __le32 magic; /* file system magic number */
24 __le32 checksum; /* crc32c(super_block) */
25 __le32 feature_compat;
26 __u8 blkszbits; /* support block_size == PAGE_SIZE only */
27 __u8 reserved;
28
29 __le16 root_nid; /* nid of root directory */
30 __le64 inos; /* total valid ino # (== f_files - f_favail) */
31
32 __le64 build_time; /* inode v1 time derivation */
33 __le32 build_time_nsec; /* inode v1 time derivation in nano scale */
34 __le32 blocks; /* used for statfs */
35 __le32 meta_blkaddr; /* start block address of metadata area */
36 __le32 xattr_blkaddr; /* start block address of shared xattr area */
37 __u8 uuid[16]; /* 128-bit uuid for volume */
38 __u8 volume_name[16]; /* volume name */
39 __le32 feature_incompat;
__le32 chksum_blocks;
In order for kernel to know how many blocks it needs to be checked...
40
41 __u8 reserved2[44];
42 };
Thanks,
Gao Xiang
> >
> > /* global sbi */
> > diff --git a/include/erofs/io.h b/include/erofs/io.h
> > index 9775047..e0ca8d9 100644
> > --- a/include/erofs/io.h
> > +++ b/include/erofs/io.h
> > @@ -19,6 +19,7 @@
> > int dev_open(const char *devname);
> > void dev_close(void);
> > int dev_write(const void *buf, u64 offset, size_t len);
> > +int dev_read(void *buf, u64 offset, size_t len);
> > int dev_fillzero(u64 offset, size_t len, bool padding);
> > int dev_fsync(void);
> > int dev_resize(erofs_blk_t nblocks);
> > @@ -31,5 +32,12 @@ static inline int blk_write(const void *buf, erofs_blk_t blkaddr,
> > blknr_to_addr(nblocks));
> > }
> >
> > +static inline int blk_read(void *buf, erofs_blk_t start,
> > + u32 nblocks)
> > +{
> > + return dev_read(buf, blknr_to_addr(start),
> > + blknr_to_addr(nblocks));
> > +}
> > +
> > #endif
> >
> > diff --git a/lib/io.c b/lib/io.c
> > index 7f5f94d..52f9424 100644
> > --- a/lib/io.c
> > +++ b/lib/io.c
> > @@ -207,3 +207,30 @@ int dev_resize(unsigned int blocks)
> > return dev_fillzero(st.st_size, length, true);
> > }
> >
> > +int dev_read(void *buf, u64 offset, size_t len)
> > +{
> > + int ret;
> > +
> > + if (cfg.c_dry_run)
> > + return 0;
> > +
> > + if (!buf) {
> > + erofs_err("buf is NULL");
> > + return -EINVAL;
> > + }
> > + if (offset >= erofs_devsz || len > erofs_devsz ||
> > + offset > erofs_devsz - len) {
> > + erofs_err("read posion[%" PRIu64 ", %zd] is too large beyond"
> > + "the end of device(%" PRIu64 ").",
> > + offset, len, erofs_devsz);
> > + return -EINVAL;
> > + }
> > +
> > + ret = pread64(erofs_devfd, buf, len, (off64_t)offset);
> > + if (ret != (int)len) {
> > + erofs_err("Failed to read data from device - %s:[%" PRIu64 ", %zd].",
> > + erofs_devname, offset, len);
> > + return -errno;
> > + }
> > + return 0;
> > +}
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index 91a018f..baaf02a 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -22,6 +22,10 @@
> >
> > #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct erofs_super_block))
> >
> > +/* number of blocks for calculating checksum */
> > +#define EROFS_CKSUM_BLOCKS 1
> > +#define EROFS_FEATURE_SB_CHKSUM 0x0001
>
> How about Moving EROFS_FEATURE_SB_CHKSUM to erofs_fs.h since it's
> an on-disk definition,
>
> > +
> > static void usage(void)
> > {
> > fprintf(stderr, "usage: [options] FILE DIRECTORY\n\n");
> > @@ -85,6 +89,10 @@ static int parse_extended_opts(const char *opts)
> > return -EINVAL;
> > cfg.c_force_inodeversion = FORCE_INODE_EXTENDED;
> > }
> > +
> > + if (MATCH_EXTENTED_OPT("nocrc", token, keylen)) {
> > + sbi.feature &= ~EROFS_FEATURE_SB_CHKSUM;
> > + }
> > }
> > return 0;
> > }
> > @@ -180,6 +188,7 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > .meta_blkaddr = sbi.meta_blkaddr,
> > .xattr_blkaddr = 0,
> > .feature_incompat = cpu_to_le32(sbi.feature_incompat),
> > + .checksum = 0
> > };
> > const unsigned int sb_blksize =
> > round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > @@ -202,6 +211,70 @@ int erofs_mkfs_update_super_block(struct erofs_buffer_head *bh,
> > return 0;
> > }
> >
> > +#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);
> > + }
> > + }
> > + return crc;
> > +}
> > +
> > +/* calculate checksum for first n blocks */
> > +u32 erofs_calc_blk_checksum(erofs_blk_t nblks, u32 *crc)
> > +{
> > + char *buf;
> > + int err = 0;
> > +
> > + buf = malloc(nblks * EROFS_BLKSIZ);
> > + err = blk_read(buf, 0, nblks);
> > + if (err) {
> > + erofs_err("Failed to calculate erofs checksum - %s",
> > + erofs_strerror(err));
> > + return err;
> > + }
> > + *crc = crc32c(0, (const unsigned char *)buf, nblks * EROFS_BLKSIZ);
> > + free(buf);
> > + return 0;
> > +}
> > +
> > +void erofs_write_checksum()
>
> How about naming write_sb_checksum?
> My idea is that this is a checksum in super block (rather than
> a checksum only for super block [0th block])
>
> Let me know if you have another thought...
>
> > +{
> > + struct erofs_super_block *sb;
> > + char buf[EROFS_BLKSIZ];
> > + int ret = 0;
> > + u32 crc;
> > +
> > + ret = erofs_calc_blk_checksum(EROFS_CKSUM_BLOCKS, &crc);
> > + if (ret) {
> > + return;
> > + }
> > + ret = blk_read(buf, 0, 1);
> > + if (ret) {
> > + erofs_err("error reading super-block structure");
> > + return;
> > + }
> > +
> > + sb = (struct erofs_super_block *)((u8 *)buf + EROFS_SUPER_OFFSET);
> > + if (le32_to_cpu(sb->magic) != EROFS_SUPER_MAGIC_V1) {
> > + erofs_err("not an erofs image");
>
> As the previous comments, I am little care about these print messages
> since users will only see this and "error reading super-block structure"
> "not an erofs image" makes confused for them... (They don't know what
> the internal process is doing)
>
> BTW, it looks good to me as a whole... Do you have some time on
> kernel side as well? :)
>
> Thanks,
> Gao Xiang
>
> > + return;
> > + }
> > + sb->checksum = cpu_to_le32(crc);
> > + ret = blk_write(buf, 0, 1);
> > + if (ret) {
> > + erofs_err("error writing 0th block to disk - %s",
> > + erofs_strerror(ret));
> > + return;
> > + }
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int err = 0;
> > @@ -217,6 +290,7 @@ int main(int argc, char **argv)
> >
> > cfg.c_legacy_compress = false;
> > sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
> > + sbi.feature = EROFS_FEATURE_SB_CHKSUM;
> >
> > err = mkfs_parse_options_cfg(argc, argv);
> > if (err) {
> > @@ -301,6 +375,8 @@ int main(int argc, char **argv)
> > err = -EIO;
> > else
> > err = dev_resize(nblocks);
> > + if (sbi.feature & EROFS_FEATURE_SB_CHKSUM)
> > + erofs_write_checksum();
> > exit:
> > z_erofs_compress_exit();
> > dev_close();
> > --
> > 2.9.3
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks.
2019-10-14 23:45 ` Gao Xiang via Linux-erofs
2019-10-15 1:33 ` Gao Xiang
@ 2019-10-15 2:00 ` Pratik Shinde
2019-10-15 2:09 ` Gao Xiang
1 sibling, 1 reply; 5+ messages in thread
From: Pratik Shinde @ 2019-10-15 2:00 UTC (permalink / raw)
To: Gao Xiang; +Cc: miaoxie, linux-erofs
[-- Attachment #1: Type: text/plain, Size: 8070 bytes --]
Hi Gao,
okay. I will make those changes. Yes I would like to do kernel side changes
too. Which branch to use for this? kernel's master?
-Pratik
On Tue, 15 Oct, 2019, 5:15 AM Gao Xiang, <hsiangkao@aol.com> wrote:
> Hi Pratik,
>
> Some nitpick comments... Let me know if you have other thoughts
>
> On Mon, Oct 14, 2019 at 08:29:43PM +0530, Pratik Shinde wrote:
> > Added code for calculating crc of erofs blocks (4K size).for now it
> calculates
> > checksum of first block. but can modified to calculate crc for any no.
> of blocks
> >
> > modified patch based on review comments.
> >
> > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > ---
> > include/erofs/internal.h | 1 +
> > include/erofs/io.h | 8 +++++
> > lib/io.c | 27 +++++++++++++++++
> > mkfs/main.c | 76
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 112 insertions(+)
> >
> > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > index 5384946..53335bc 100644
> > --- a/include/erofs/internal.h
> > +++ b/include/erofs/internal.h
> > @@ -55,6 +55,7 @@ struct erofs_sb_info {
> > u32 feature_incompat;
> > u64 build_time;
> > u32 build_time_nsec;
> > + u32 feature;
> > };
> >
> > /* global sbi */
> > diff --git a/include/erofs/io.h b/include/erofs/io.h
> > index 9775047..e0ca8d9 100644
> > --- a/include/erofs/io.h
> > +++ b/include/erofs/io.h
> > @@ -19,6 +19,7 @@
> > int dev_open(const char *devname);
> > void dev_close(void);
> > int dev_write(const void *buf, u64 offset, size_t len);
> > +int dev_read(void *buf, u64 offset, size_t len);
> > int dev_fillzero(u64 offset, size_t len, bool padding);
> > int dev_fsync(void);
> > int dev_resize(erofs_blk_t nblocks);
> > @@ -31,5 +32,12 @@ static inline int blk_write(const void *buf,
> erofs_blk_t blkaddr,
> > blknr_to_addr(nblocks));
> > }
> >
> > +static inline int blk_read(void *buf, erofs_blk_t start,
> > + u32 nblocks)
> > +{
> > + return dev_read(buf, blknr_to_addr(start),
> > + blknr_to_addr(nblocks));
> > +}
> > +
> > #endif
> >
> > diff --git a/lib/io.c b/lib/io.c
> > index 7f5f94d..52f9424 100644
> > --- a/lib/io.c
> > +++ b/lib/io.c
> > @@ -207,3 +207,30 @@ int dev_resize(unsigned int blocks)
> > return dev_fillzero(st.st_size, length, true);
> > }
> >
> > +int dev_read(void *buf, u64 offset, size_t len)
> > +{
> > + int ret;
> > +
> > + if (cfg.c_dry_run)
> > + return 0;
> > +
> > + if (!buf) {
> > + erofs_err("buf is NULL");
> > + return -EINVAL;
> > + }
> > + if (offset >= erofs_devsz || len > erofs_devsz ||
> > + offset > erofs_devsz - len) {
> > + erofs_err("read posion[%" PRIu64 ", %zd] is too large
> beyond"
> > + "the end of device(%" PRIu64 ").",
> > + offset, len, erofs_devsz);
> > + return -EINVAL;
> > + }
> > +
> > + ret = pread64(erofs_devfd, buf, len, (off64_t)offset);
> > + if (ret != (int)len) {
> > + erofs_err("Failed to read data from device - %s:[%" PRIu64
> ", %zd].",
> > + erofs_devname, offset, len);
> > + return -errno;
> > + }
> > + return 0;
> > +}
> > diff --git a/mkfs/main.c b/mkfs/main.c
> > index 91a018f..baaf02a 100644
> > --- a/mkfs/main.c
> > +++ b/mkfs/main.c
> > @@ -22,6 +22,10 @@
> >
> > #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> erofs_super_block))
> >
> > +/* number of blocks for calculating checksum */
> > +#define EROFS_CKSUM_BLOCKS 1
> > +#define EROFS_FEATURE_SB_CHKSUM 0x0001
>
> How about Moving EROFS_FEATURE_SB_CHKSUM to erofs_fs.h since it's
> an on-disk definition,
>
> > +
> > static void usage(void)
> > {
> > fprintf(stderr, "usage: [options] FILE DIRECTORY\n\n");
> > @@ -85,6 +89,10 @@ static int parse_extended_opts(const char *opts)
> > return -EINVAL;
> > cfg.c_force_inodeversion = FORCE_INODE_EXTENDED;
> > }
> > +
> > + if (MATCH_EXTENTED_OPT("nocrc", token, keylen)) {
> > + sbi.feature &= ~EROFS_FEATURE_SB_CHKSUM;
> > + }
> > }
> > return 0;
> > }
> > @@ -180,6 +188,7 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> > .meta_blkaddr = sbi.meta_blkaddr,
> > .xattr_blkaddr = 0,
> > .feature_incompat = cpu_to_le32(sbi.feature_incompat),
> > + .checksum = 0
> > };
> > const unsigned int sb_blksize =
> > round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > @@ -202,6 +211,70 @@ int erofs_mkfs_update_super_block(struct
> erofs_buffer_head *bh,
> > return 0;
> > }
> >
> > +#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);
> > + }
> > + }
> > + return crc;
> > +}
> > +
> > +/* calculate checksum for first n blocks */
> > +u32 erofs_calc_blk_checksum(erofs_blk_t nblks, u32 *crc)
> > +{
> > + char *buf;
> > + int err = 0;
> > +
> > + buf = malloc(nblks * EROFS_BLKSIZ);
> > + err = blk_read(buf, 0, nblks);
> > + if (err) {
> > + erofs_err("Failed to calculate erofs checksum - %s",
> > + erofs_strerror(err));
> > + return err;
> > + }
> > + *crc = crc32c(0, (const unsigned char *)buf, nblks * EROFS_BLKSIZ);
> > + free(buf);
> > + return 0;
> > +}
> > +
> > +void erofs_write_checksum()
>
> How about naming write_sb_checksum?
> My idea is that this is a checksum in super block (rather than
> a checksum only for super block [0th block])
>
> Let me know if you have another thought...
>
> > +{
> > + struct erofs_super_block *sb;
> > + char buf[EROFS_BLKSIZ];
> > + int ret = 0;
> > + u32 crc;
> > +
> > + ret = erofs_calc_blk_checksum(EROFS_CKSUM_BLOCKS, &crc);
> > + if (ret) {
> > + return;
> > + }
> > + ret = blk_read(buf, 0, 1);
> > + if (ret) {
> > + erofs_err("error reading super-block structure");
> > + return;
> > + }
> > +
> > + sb = (struct erofs_super_block *)((u8 *)buf + EROFS_SUPER_OFFSET);
> > + if (le32_to_cpu(sb->magic) != EROFS_SUPER_MAGIC_V1) {
> > + erofs_err("not an erofs image");
>
> As the previous comments, I am little care about these print messages
> since users will only see this and "error reading super-block structure"
> "not an erofs image" makes confused for them... (They don't know what
> the internal process is doing)
>
> BTW, it looks good to me as a whole... Do you have some time on
> kernel side as well? :)
>
> Thanks,
> Gao Xiang
>
> > + return;
> > + }
> > + sb->checksum = cpu_to_le32(crc);
> > + ret = blk_write(buf, 0, 1);
> > + if (ret) {
> > + erofs_err("error writing 0th block to disk - %s",
> > + erofs_strerror(ret));
> > + return;
> > + }
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > int err = 0;
> > @@ -217,6 +290,7 @@ int main(int argc, char **argv)
> >
> > cfg.c_legacy_compress = false;
> > sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
> > + sbi.feature = EROFS_FEATURE_SB_CHKSUM;
> >
> > err = mkfs_parse_options_cfg(argc, argv);
> > if (err) {
> > @@ -301,6 +375,8 @@ int main(int argc, char **argv)
> > err = -EIO;
> > else
> > err = dev_resize(nblocks);
> > + if (sbi.feature & EROFS_FEATURE_SB_CHKSUM)
> > + erofs_write_checksum();
> > exit:
> > z_erofs_compress_exit();
> > dev_close();
> > --
> > 2.9.3
> >
>
[-- Attachment #2: Type: text/html, Size: 10708 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks.
2019-10-15 2:00 ` Pratik Shinde
@ 2019-10-15 2:09 ` Gao Xiang
0 siblings, 0 replies; 5+ messages in thread
From: Gao Xiang @ 2019-10-15 2:09 UTC (permalink / raw)
To: Pratik Shinde; +Cc: linux-erofs, miaoxie
Hi Pratik,
On Tue, Oct 15, 2019 at 07:30:38AM +0530, Pratik Shinde wrote:
> Hi Gao,
>
> okay. I will make those changes. Yes I would like to do kernel side changes
> too. Which branch to use for this? kernel's master?
erofs development tree is better,
git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git -b dev
erofs is now an independent tree, and I will collect all kernel patches
(including features, bugfixes and cleanups) for linux-next then.
p.s. kind reminder, don't forgot add a chksum_blocks field to on-disk
erofs_super_block
as well.
Thanks,
Goa Xiang
>
> -Pratik
>
> On Tue, 15 Oct, 2019, 5:15 AM Gao Xiang, <hsiangkao@aol.com> wrote:
>
> > Hi Pratik,
> >
> > Some nitpick comments... Let me know if you have other thoughts
> >
> > On Mon, Oct 14, 2019 at 08:29:43PM +0530, Pratik Shinde wrote:
> > > Added code for calculating crc of erofs blocks (4K size).for now it
> > calculates
> > > checksum of first block. but can modified to calculate crc for any no.
> > of blocks
> > >
> > > modified patch based on review comments.
> > >
> > > Signed-off-by: Pratik Shinde <pratikshinde320@gmail.com>
> > > ---
> > > include/erofs/internal.h | 1 +
> > > include/erofs/io.h | 8 +++++
> > > lib/io.c | 27 +++++++++++++++++
> > > mkfs/main.c | 76
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 112 insertions(+)
> > >
> > > diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> > > index 5384946..53335bc 100644
> > > --- a/include/erofs/internal.h
> > > +++ b/include/erofs/internal.h
> > > @@ -55,6 +55,7 @@ struct erofs_sb_info {
> > > u32 feature_incompat;
> > > u64 build_time;
> > > u32 build_time_nsec;
> > > + u32 feature;
> > > };
> > >
> > > /* global sbi */
> > > diff --git a/include/erofs/io.h b/include/erofs/io.h
> > > index 9775047..e0ca8d9 100644
> > > --- a/include/erofs/io.h
> > > +++ b/include/erofs/io.h
> > > @@ -19,6 +19,7 @@
> > > int dev_open(const char *devname);
> > > void dev_close(void);
> > > int dev_write(const void *buf, u64 offset, size_t len);
> > > +int dev_read(void *buf, u64 offset, size_t len);
> > > int dev_fillzero(u64 offset, size_t len, bool padding);
> > > int dev_fsync(void);
> > > int dev_resize(erofs_blk_t nblocks);
> > > @@ -31,5 +32,12 @@ static inline int blk_write(const void *buf,
> > erofs_blk_t blkaddr,
> > > blknr_to_addr(nblocks));
> > > }
> > >
> > > +static inline int blk_read(void *buf, erofs_blk_t start,
> > > + u32 nblocks)
> > > +{
> > > + return dev_read(buf, blknr_to_addr(start),
> > > + blknr_to_addr(nblocks));
> > > +}
> > > +
> > > #endif
> > >
> > > diff --git a/lib/io.c b/lib/io.c
> > > index 7f5f94d..52f9424 100644
> > > --- a/lib/io.c
> > > +++ b/lib/io.c
> > > @@ -207,3 +207,30 @@ int dev_resize(unsigned int blocks)
> > > return dev_fillzero(st.st_size, length, true);
> > > }
> > >
> > > +int dev_read(void *buf, u64 offset, size_t len)
> > > +{
> > > + int ret;
> > > +
> > > + if (cfg.c_dry_run)
> > > + return 0;
> > > +
> > > + if (!buf) {
> > > + erofs_err("buf is NULL");
> > > + return -EINVAL;
> > > + }
> > > + if (offset >= erofs_devsz || len > erofs_devsz ||
> > > + offset > erofs_devsz - len) {
> > > + erofs_err("read posion[%" PRIu64 ", %zd] is too large
> > beyond"
> > > + "the end of device(%" PRIu64 ").",
> > > + offset, len, erofs_devsz);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = pread64(erofs_devfd, buf, len, (off64_t)offset);
> > > + if (ret != (int)len) {
> > > + erofs_err("Failed to read data from device - %s:[%" PRIu64
> > ", %zd].",
> > > + erofs_devname, offset, len);
> > > + return -errno;
> > > + }
> > > + return 0;
> > > +}
> > > diff --git a/mkfs/main.c b/mkfs/main.c
> > > index 91a018f..baaf02a 100644
> > > --- a/mkfs/main.c
> > > +++ b/mkfs/main.c
> > > @@ -22,6 +22,10 @@
> > >
> > > #define EROFS_SUPER_END (EROFS_SUPER_OFFSET + sizeof(struct
> > erofs_super_block))
> > >
> > > +/* number of blocks for calculating checksum */
> > > +#define EROFS_CKSUM_BLOCKS 1
> > > +#define EROFS_FEATURE_SB_CHKSUM 0x0001
> >
> > How about Moving EROFS_FEATURE_SB_CHKSUM to erofs_fs.h since it's
> > an on-disk definition,
> >
> > > +
> > > static void usage(void)
> > > {
> > > fprintf(stderr, "usage: [options] FILE DIRECTORY\n\n");
> > > @@ -85,6 +89,10 @@ static int parse_extended_opts(const char *opts)
> > > return -EINVAL;
> > > cfg.c_force_inodeversion = FORCE_INODE_EXTENDED;
> > > }
> > > +
> > > + if (MATCH_EXTENTED_OPT("nocrc", token, keylen)) {
> > > + sbi.feature &= ~EROFS_FEATURE_SB_CHKSUM;
> > > + }
> > > }
> > > return 0;
> > > }
> > > @@ -180,6 +188,7 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > > .meta_blkaddr = sbi.meta_blkaddr,
> > > .xattr_blkaddr = 0,
> > > .feature_incompat = cpu_to_le32(sbi.feature_incompat),
> > > + .checksum = 0
> > > };
> > > const unsigned int sb_blksize =
> > > round_up(EROFS_SUPER_END, EROFS_BLKSIZ);
> > > @@ -202,6 +211,70 @@ int erofs_mkfs_update_super_block(struct
> > erofs_buffer_head *bh,
> > > return 0;
> > > }
> > >
> > > +#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);
> > > + }
> > > + }
> > > + return crc;
> > > +}
> > > +
> > > +/* calculate checksum for first n blocks */
> > > +u32 erofs_calc_blk_checksum(erofs_blk_t nblks, u32 *crc)
> > > +{
> > > + char *buf;
> > > + int err = 0;
> > > +
> > > + buf = malloc(nblks * EROFS_BLKSIZ);
> > > + err = blk_read(buf, 0, nblks);
> > > + if (err) {
> > > + erofs_err("Failed to calculate erofs checksum - %s",
> > > + erofs_strerror(err));
> > > + return err;
> > > + }
> > > + *crc = crc32c(0, (const unsigned char *)buf, nblks * EROFS_BLKSIZ);
> > > + free(buf);
> > > + return 0;
> > > +}
> > > +
> > > +void erofs_write_checksum()
> >
> > How about naming write_sb_checksum?
> > My idea is that this is a checksum in super block (rather than
> > a checksum only for super block [0th block])
> >
> > Let me know if you have another thought...
> >
> > > +{
> > > + struct erofs_super_block *sb;
> > > + char buf[EROFS_BLKSIZ];
> > > + int ret = 0;
> > > + u32 crc;
> > > +
> > > + ret = erofs_calc_blk_checksum(EROFS_CKSUM_BLOCKS, &crc);
> > > + if (ret) {
> > > + return;
> > > + }
> > > + ret = blk_read(buf, 0, 1);
> > > + if (ret) {
> > > + erofs_err("error reading super-block structure");
> > > + return;
> > > + }
> > > +
> > > + sb = (struct erofs_super_block *)((u8 *)buf + EROFS_SUPER_OFFSET);
> > > + if (le32_to_cpu(sb->magic) != EROFS_SUPER_MAGIC_V1) {
> > > + erofs_err("not an erofs image");
> >
> > As the previous comments, I am little care about these print messages
> > since users will only see this and "error reading super-block structure"
> > "not an erofs image" makes confused for them... (They don't know what
> > the internal process is doing)
> >
> > BTW, it looks good to me as a whole... Do you have some time on
> > kernel side as well? :)
> >
> > Thanks,
> > Gao Xiang
> >
> > > + return;
> > > + }
> > > + sb->checksum = cpu_to_le32(crc);
> > > + ret = blk_write(buf, 0, 1);
> > > + if (ret) {
> > > + erofs_err("error writing 0th block to disk - %s",
> > > + erofs_strerror(ret));
> > > + return;
> > > + }
> > > +}
> > > +
> > > int main(int argc, char **argv)
> > > {
> > > int err = 0;
> > > @@ -217,6 +290,7 @@ int main(int argc, char **argv)
> > >
> > > cfg.c_legacy_compress = false;
> > > sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
> > > + sbi.feature = EROFS_FEATURE_SB_CHKSUM;
> > >
> > > err = mkfs_parse_options_cfg(argc, argv);
> > > if (err) {
> > > @@ -301,6 +375,8 @@ int main(int argc, char **argv)
> > > err = -EIO;
> > > else
> > > err = dev_resize(nblocks);
> > > + if (sbi.feature & EROFS_FEATURE_SB_CHKSUM)
> > > + erofs_write_checksum();
> > > exit:
> > > z_erofs_compress_exit();
> > > dev_close();
> > > --
> > > 2.9.3
> > >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-10-15 2:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-14 14:59 [PATCH-v2] erofs-utils:code for calculating crc checksum of erofs blocks Pratik Shinde
2019-10-14 23:45 ` Gao Xiang via Linux-erofs
2019-10-15 1:33 ` Gao Xiang
2019-10-15 2:00 ` Pratik Shinde
2019-10-15 2:09 ` Gao Xiang
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).