Re: [PATCH 3/3] kill xfs_dinode_core_t

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 07, 2008 at 10:22:01PM +0200, Christoph Hellwig wrote:
> No that we have a separate xfs_icdinode_t for the in-core inode that
> get's logged there's absolutely no reason for the xfs_dinode vs
> xfs_dinode_core split - the fact that part of the structure gets logged
> through the inode log item and a small part not can better be described
> in a comment.  The few places that uses sizeof() on the dinode_core
> are replaced with macros that also prepare for the variable size inode
> core we'll need for adding checksums to the inodes.
> 
> Removing the data and attribute fork unions also has the advantage that
> xfs_dinode.h doesn't need to pull in every header under the sun.
> 
> While we're at it also add some more comments describing the dinode
> structure.

Nice. I haven't reviewed this fully yet, but a couple of things
stand out:

> @@ -359,7 +357,7 @@ xfs_dip_to_stat(
>  
>  	switch (dic->di_format) {
>  	case XFS_DINODE_FMT_DEV:
> -		buf->dt_rdev = be32_to_cpu(dip->di_u.di_dev);
> +		buf->dt_rdev = be32_to_cpu(*(__be32 *)XFS_DFORK_DPTR(mp, dic));

That's not particularly obvious where the rdev value is stored.
Perhaps a comment indicating that it's a direct dereference out
of the start of the data area in the inode?

>  /*
> - * Note: Coordinate changes to this structure with the XFS_DI_* #defines
> - * below, the offsets table in xfs_ialloc_log_di() and struct xfs_icdinode
> - * in xfs_inode.h.
> + * On-disk inode structure.
> + *
> + * This is just the header or "dinode core", the inode is expanded to fill a
> + * variable size the leftover area split into a data and an attribute fork.
> + * The format of the data and attribute fork depends on the format of the
> + * inode as indicated by di_format and di_aformat.  To access the data and
> + * attribute use the XFS_DFORK_PTR, XFS_DFORK_DPTR, and XFS_DFORK_PTR macro:

macros

> +#define XFS_DINODE_CORE_SIZE(mp)	(96)
> +#define XFS_DINODE_SIZE(mp)		(96 + sizeof(__be32))

probably shouldn't hard-code the second "96" there. Perhaps
the XFS_DINODE_SIZE() macro should be a sizeof(xfs_dinode_t)?

That's as far as I've looked....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com