Re: [PATCH v2] Ext4 documentation fixes.

[Theodore Y. Ts'o]

On Fri, Aug 23, 2019 at 04:56:42AM +0000, Ayush Ranjan wrote:
> Hey Ted!
> Thanks for reviewing! The comment in fs/ext4/ext4.h:ext4_group_desc:bg_checksum
> says that the crc16 checksum formula should be crc16(sb_uuid+group+desc). I
> think group over here denotes group number.
> 
> Briefly looking through fs/ext4/super.c:ext4_group_desc_csum() suggests that:
> - For the new metadata_csum algorithm, only the group number and the block
> descriptor are included in the checksum. So the formula should be
> crc32c(group+desc) & 0xFFF (this looks like a bug as this should also include sb
> UUID?)
> - For the old crc16 algorithm, the sb UUID, group number and the block
> descriptor are included in the checksum. So the formula should be
> crc16(sb\_uuid+group+desc). (should remain unchanged)

Thanks for the research and explanation.  I think I'm going to change
that to be:

crc{16,32c}(sb_uuid + group_num + bg_desc)

That should make it clearer what is meant.

     	    	    	    	 - Ted








> 
> Ayush Ranjan
> University of Illinois - Urbana Champaign | May 2020
> Bachelors of Science in Computer Science and Mathematics
> Business Minor | Gies College of Business
> 
> 
> On Fri, Aug 23, 2019 at 8:48 AM Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
> >
> > On Thu, Aug 15, 2019 at 09:11:51AM -0700, Ayush Ranjan wrote:
> > > This commit aims to fix the following issues in ext4 documentation:
> > > - Flexible block group docs said that the aim was to group block
> > >   metadata together instead of block group metadata.
> > > - The documentation consistly uses "location" instead of "block number".
> > >   It is easy to confuse location to be an absolute offset on disk. Added
> > >   a line to clarify all location values are in terms of block numbers.
> > > - Dirent2 docs said that the rec_len field is shortened instead of the
> > >   name_len field.
> > > - Typo in bg_checksum description.
> > > - Inode size is 160 bytes now, and hence i_extra_isize is now 32.
> > > - Cluster size formula was incorrect, it did not include the +10 to
> > >   s_log_cluster_size value.
> > > - Typo: there were two s_wtime_hi in the superblock struct.
> > > - Superblock struct was outdated, added the new fields which were part
> > >   of s_reserved earlier.
> > > - Multiple mount protection seems to be implemented in fs/ext4/mmp.c.
> > >
> > > Signed-off-by: Ayush Ranjan <ayushr2@xxxxxxxxxxxx>
> >
> > Fixed with one minor typo fix:
> >
> > > diff --git a/Documentation/filesystems/ext4/group_descr.rst
> > > b/Documentation/filesystems/ext4/group_descr.rst
> > > index 0f783ed88..feb5c613d 100644
> > > --- a/Documentation/filesystems/ext4/group_descr.rst
> > > +++ b/Documentation/filesystems/ext4/group_descr.rst
> > > @@ -100,7 +100,7 @@ The block group descriptor is laid out in ``struct
> > > ext4_group_desc``.
> > >       - \_\_le16
> > >       - bg\_checksum
> > >       - Group descriptor checksum; crc16(sb\_uuid+group+desc) if the
> > > -       RO\_COMPAT\_GDT\_CSUM feature is set, or
> crc32c(sb\_uuid+group\_desc) &
> > > +       RO\_COMPAT\_GDT\_CSUM feature is set, or crc32c(sb\_uuid+group+desc)
> &
> > >         0xFFFF if the RO\_COMPAT\_METADATA\_CSUM feature is set.
> >
> > The correct checksum should be "crc16(sb\_uuid+group\_desc)" or
> > "crc32c(sb\_uuid+group\_desc)".  That is, it's previous line which
> > needed modification.
> >
> >                                         - Ted