On Sun, Oct 20, 2019 at 10:21:45AM +0200, Christoph Hellwig wrote:
> There is no point in having this sub-structure except for historical
> reasons. Remove it and just fold the fields into struct xfs_inode.
This is too big to be reviewable. And because it changes stuff like
i_d.di_size, it could affect on-disk inode size updates. Hence I
think this needs to be broken down into smaller patches...
FWIW, A quick glance reveals:
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index ec302b7e48f3..df755de3705c 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -62,12 +62,34 @@ xfs_inode_alloc(
> memset(&ip->i_imap, 0, sizeof(struct xfs_imap));
> ip->i_afp = NULL;
> ip->i_cowfp = NULL;
> - ip->i_cnextents = 0;
> - ip->i_cformat = XFS_DINODE_FMT_EXTENTS;
> memset(&ip->i_df, 0, sizeof(ip->i_df));
> +
> ip->i_flags = 0;
> ip->i_delayed_blks = 0;
> - memset(&ip->i_d, 0, sizeof(ip->i_d));
> +
> + ip->i_version = 0;
> + ip->i_format = 0;
> + ip->i_flushiter = 0;
> + ip->i_uid = 0;
> + ip->i_gid = 0;
> + ip->i_projid = 0;
> + ip->i_disk_size = 0;
> + ip->i_nblocks = 0;
> + ip->i_extsize = 0;
> + ip->i_nextents = 0;
> + ip->i_anextents = 0;
> + ip->i_forkoff = 0;
> + ip->i_aformat = 0;
> + ip->i_dmevmask = 0;
> + ip->i_dmstate = 0;
> + ip->i_diflags = 0;
> + ip->i_diflags2 = 0;
> + ip->i_cowextsize = 0;
> + ip->i_crtime.tv_sec = 0;
> + ip->i_crtime.tv_nsec = 0;
> + ip->i_cnextents = 0;
> + ip->i_cformat = XFS_DINODE_FMT_EXTENTS;
This is, IMO, a step backards. We're going to end up failing to
initialise new fields correctly with this...
> diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
> index a0ca7ded3ab8..32fbe8feeb0e 100644
> --- a/fs/xfs/xfs_inode.h
> +++ b/fs/xfs/xfs_inode.h
> @@ -58,8 +58,25 @@ typedef struct xfs_inode {
> unsigned long i_flags; /* see defined flags below */
> uint64_t i_delayed_blks; /* count of delay alloc blks */
>
> - struct xfs_icdinode i_d; /* most of ondisk inode */
> -
> + int8_t i_version; /* inode version */
> + int8_t i_format; /* data fork format */
> + uint16_t i_flushiter; /* incremented on flush */
> + uint32_t i_uid; /* owner's user id */
> + uint32_t i_gid; /* owner's group id */
> + uint16_t i_projid; /* owner's project id */
This is a bug and should make all the 32-bit project ID tests fail.
If it doesn't them we've got a problem with our test coverage. If it
does fail, then I'm not sure this patchset has been adequately
tested...
It also introduces a hole in the structure.
> + xfs_fsize_t i_disk_size; /* number of bytes in file */
> + xfs_rfsblock_t i_nblocks; /* direct & btree blocks used */
> + xfs_extlen_t i_extsize; /* extent size hint */
> + xfs_extnum_t i_nextents; /* # of extents in data fork */
> + xfs_aextnum_t i_anextents; /* # of extents in attr fork */
> + uint8_t i_forkoff; /* attr fork offset */
> + int8_t i_aformat; /* attr fork format */
> + uint32_t i_dmevmask; /* DMIG event mask */
> + uint16_t i_dmstate; /* DMIG state info */
If we are cleaning up the icdinode, why do these still exist in
memory?
> + uint16_t i_diflags; /* random flags */
> + uint64_t i_diflags2; /* more random flags */
> + uint32_t i_cowextsize; /* cow extent size hint */
> + struct timespec64 i_crtime; /* time created */
And there's another new hole in the structure there, maybe more than
one....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
