<div dir="auto">Hi Gao, <div dir="auto"><br></div><div dir="auto">Understood your concern.</div><div dir="auto"><br></div><div dir="auto">Can we do something like :</div><div dir="auto"><br></div><div dir="auto">1) Allocate one buf of size EROFS_BLKSIZ</div><div dir="auto">2) Read one page at a time into buf(memcpy) .call crc32c for it.</div><div dir="auto"><br></div><div dir="auto">In this way we won't be writing directly into page data and will not do large allocation.</div><div dir="auto">What do you think?</div><div dir="auto"><br></div><div dir="auto">--Pratik</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 22 Oct, 2019, 10:04 PM Gao Xiang, <<a href="mailto:hsiangkao@aol.com" target="_blank" rel="noreferrer">hsiangkao@aol.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Pratik,<br>
<br>
Some comments as below...<br>
<br>
On Tue, Oct 22, 2019 at 09:09:56PM +0530, Pratik Shinde wrote:<br>
> Patch for kernel side changes of checksum feature.Used kernel's<br>
> crc32c library for calculating the checksum.<br>
> <br>
> Signed-off-by: Pratik Shinde <<a href="mailto:pratikshinde320@gmail.com" rel="noreferrer noreferrer" target="_blank">pratikshinde320@gmail.com</a>><br>
> ---<br>
> fs/erofs/erofs_fs.h | 5 +++--<br>
> fs/erofs/internal.h | 3 ++-<br>
> fs/erofs/super.c | 33 +++++++++++++++++++++++++++++++++<br>
> 3 files changed, 38 insertions(+), 3 deletions(-)<br>
> <br>
> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h<br>
> index b1ee565..4d8097a 100644<br>
> --- a/fs/erofs/erofs_fs.h<br>
> +++ b/fs/erofs/erofs_fs.h<br>
> @@ -17,6 +17,7 @@<br>
> */<br>
> #define EROFS_FEATURE_INCOMPAT_LZ4_0PADDING 0x00000001<br>
> #define EROFS_ALL_FEATURE_INCOMPAT EROFS_FEATURE_INCOMPAT_LZ4_0PADDING<br>
> +#define EROFS_FEATURE_COMPAT_SB_CHKSUM 0x00000001<br>
> <br>
> /* 128-byte erofs on-disk super block */<br>
> struct erofs_super_block {<br>
> @@ -37,8 +38,8 @@ struct erofs_super_block {<br>
> __u8 uuid[16]; /* 128-bit uuid for volume */<br>
> __u8 volume_name[16]; /* volume name */<br>
> __le32 feature_incompat;<br>
> -<br>
> - __u8 reserved2[44];<br>
> + __le32 chksum_blocks; /* number of blocks used for checksum */<br>
> + __u8 reserved2[40];<br>
> };<br>
> <br>
> /*<br>
> diff --git a/fs/erofs/internal.h b/fs/erofs/internal.h<br>
> index 544a453..cec27ca 100644<br>
> --- a/fs/erofs/internal.h<br>
> +++ b/fs/erofs/internal.h<br>
> @@ -86,7 +86,7 @@ struct erofs_sb_info {<br>
> u8 uuid[16]; /* 128-bit uuid for volume */<br>
> u8 volume_name[16]; /* volume name */<br>
> u32 feature_incompat;<br>
> -<br>
> + u32 feature_compat;<br>
> unsigned int mount_opt;<br>
> };<br>
> <br>
> @@ -426,6 +426,7 @@ static inline void z_erofs_exit_zip_subsystem(void) {}<br>
> #endif /* !CONFIG_EROFS_FS_ZIP */<br>
> <br>
> #define EFSCORRUPTED EUCLEAN /* Filesystem is corrupted */<br>
> +#define EFSBADCRC EBADMSG /* Bad crc found */<br>
> <br>
> #endif /* __EROFS_INTERNAL_H */<br>
> <br>
> diff --git a/fs/erofs/super.c b/fs/erofs/super.c<br>
> index 0e36949..9cda72d 100644<br>
> --- a/fs/erofs/super.c<br>
> +++ b/fs/erofs/super.c<br>
> @@ -9,6 +9,7 @@<br>
> #include <linux/statfs.h><br>
> #include <linux/parser.h><br>
> #include <linux/seq_file.h><br>
> +#include <linux/crc32c.h><br>
> #include "xattr.h"<br>
> <br>
> #define CREATE_TRACE_POINTS<br>
> @@ -46,6 +47,31 @@ void _erofs_info(struct super_block *sb, const char *function,<br>
> va_end(args);<br>
> }<br>
> <br>
> +static int erofs_validate_sb_chksum(struct erofs_super_block *dsb,<br>
> + struct super_block *sb)<br>
> +{<br>
> + u32 disk_chksum, nblocks, crc = 0;<br>
> + void *kaddr;<br>
> + struct page *page;<br>
> + int i;<br>
> +<br>
> + disk_chksum = le32_to_cpu(dsb->checksum);<br>
> + nblocks = le32_to_cpu(dsb->chksum_blocks);<br>
<br>
We cannot write the page data directly since the page cache should be kept in<br>
sync with ondisk data (or for read-write fs, if it's claimed as uptodated, and<br>
it is modified later, you should mark it dirty, and do writeback then, but<br>
that is not the erofs case.)<br>
<br>
> + dsb->checksum = 0;<br>
> + for (i = 0; i < nblocks; i++) {<br>
> + page = erofs_get_meta_page(sb, i);<br>
> + if (IS_ERR(page))<br>
> + return PTR_ERR(page);<br>
> + kaddr = kmap(page);<br>
<br>
Here kmap_atomic(page) is better. what I mean is kmap_atomic() in the caller<br>
erofs_read_superblock(), it should be replaced to kmap() instead.<br>
<br>
> + crc = crc32c(crc, kaddr, EROFS_BLKSIZ);<br>
> + kunmap(page);<br>
> + unlock_page(page);<br>
<br>
need<br>
put_page(page);<br>
<br>
<br>
I'm not sure whether I explained quite well, but this patch needs something<br>
to do. I'm now working on demonstrating new XZ algorithm and releasing<br>
erofs-utils v1.0.<br>
<br>
You can give more tries or I will help later. :-)<br>
<br>
Thanks,<br>
Gao Xiang<br>
<br>
<br>
> + }<br>
> + if (crc != disk_chksum)<br>
> + return -EFSBADCRC;<br>
> + return 0;<br>
> +}<br>
> +<br>
> static void erofs_inode_init_once(void *ptr)<br>
> {<br>
> struct erofs_inode *vi = ptr;<br>
> @@ -121,6 +147,13 @@ static int erofs_read_superblock(struct super_block *sb)<br>
> goto out;<br>
> }<br>
> <br>
> + if (dsb->feature_compat & EROFS_FEATURE_COMPAT_SB_CHKSUM) {<br>
> + ret = erofs_validate_sb_chksum(dsb, sb);<br>
> + if (ret < 0) {<br>
> + erofs_err(sb, "super block checksum incorrect");<br>
> + goto out;<br>
> + }<br>
> + }<br>
> blkszbits = dsb->blkszbits;<br>
> /* 9(512 bytes) + LOG_SECTORS_PER_BLOCK == LOG_BLOCK_SIZE */<br>
> if (blkszbits != LOG_BLOCK_SIZE) {<br>
> -- <br>
> 2.9.3<br>
> <br>
</blockquote></div>