Hi Gao,

Understood your concern.

Can we do something like :

1) Allocate one buf of size EROFS_BLKSIZ
2) Read one page at a time into buf(memcpy) .call crc32c for it.

In this way we won't be writing directly into page data and will not do
large allocation.
What do you think?

--Pratik

On Tue, 22 Oct, 2019, 10:04 PM Gao Xiang, <hsiangkao@aol.com> wrote:

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