[Top] [All Lists]

Re: [PATCH 3/3] kill xfs_dinode_core_t

To: Christoph Hellwig <hch@xxxxxx>
Subject: Re: [PATCH 3/3] kill xfs_dinode_core_t
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 8 Oct 2008 08:36:10 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20081007202201.GC16485@xxxxxx>
Mail-followup-to: Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
References: <20081007202201.GC16485@xxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
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:


> +#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....


Dave Chinner

<Prev in Thread] Current Thread [Next in Thread>