Re: [PATCH v2] Ext4 documentation fixes.

Re: [PATCH v2] Ext4 documentation fixes.

[Theodore Y. Ts'o]

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@illinois.edu>

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

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@mit.edu> 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@illinois.edu>
> >
> > 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

Re: [PATCH v2] Ext4 documentation fixes.

[Jonathan Corbet]

On Thu, 15 Aug 2019 09:16:23 -0700
Andreas Dilger <adilger@dilger.ca> wrote:

> On Aug 15, 2019, at 09:11, Ayush Ranjan <ayushr2@illinois.edu> 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@illinois.edu>  
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

I've applied this to the docs tree.

However, Ayush: the patch was rather badly corrupted by your mail client.
I managed to fix it up, but please in the future verify that you can email
a patch to yourself and apply it before submitting it.  There may be some
useful hints in Documentation/process/email-clients.rst .

Thanks,

jon

Re: [PATCH v2] Ext4 documentation fixes.

[Theodore Y. Ts'o]

On Sat, Aug 24, 2019 at 03:24:53PM -0600, Jonathan Corbet wrote:
> I've applied this to the docs tree.
> 
> However, Ayush: the patch was rather badly corrupted by your mail client.
> I managed to fix it up, but please in the future verify that you can email
> a patch to yourself and apply it before submitting it.  There may be some
> useful hints in Documentation/process/email-clients.rst .

Hi Jon,

If you haven't pushed out your doc tree, can you please drop it?  I've
already applied an (improved) version of this patch to the ext4 tree,
and I actually have some plans to do further fixups of ext4 on-disk
format documentation in the ext4 tree.  So it would be easier if we
keep ext4 documentation updates in the ext4 git tree.

Thanks,

					- Ted

Re: [PATCH v2] Ext4 documentation fixes.

[Jonathan Corbet]

On Sat, 24 Aug 2019 19:09:30 -0400
"Theodore Y. Ts'o" <tytso@mit.edu> wrote:

> If you haven't pushed out your doc tree, can you please drop it?  I've
> already applied an (improved) version of this patch to the ext4 tree,
> and I actually have some plans to do further fixups of ext4 on-disk
> format documentation in the ext4 tree.  So it would be easier if we
> keep ext4 documentation updates in the ext4 git tree.

Sigh...I checked linux-next first and saw no signs of activity there.  Oh
well; I guess I can disappear that change.

jon