On Thu, Jan 14, 2016 at 05:09:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Now that the struct xfs_icdinode is not directly related to the
> on-disk format, we can cull things in it we really don't need to
> store:
>
> - magic number never changes
> - padding is not necessary
> - next_unlinked is never used
> - inode number is redundant
> - uuid is redundant
> - lsn is accessed directly from dinode
> - inode CRC is only accessed directly from dinode
>
> Hence we can remove these from the struct xfs_icdinode and redirect
> the code that uses them to the xfs_dinode appripriately. This
> reduces the size of the struct icdinode from 152 bytes to 88 bytes,
> and removes a fair chunk of unnecessary code, too.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
A couple nits and a minor potential bug: you have a semicolon in the
subject header rather than a colon...
> fs/xfs/libxfs/xfs_inode_buf.c | 39 +++++++++++++--------------------------
> fs/xfs/libxfs/xfs_inode_buf.h | 27 +++++++--------------------
> fs/xfs/xfs_inode.c | 19 +------------------
> fs/xfs/xfs_inode_item.c | 19 +++++++++++--------
> 4 files changed, 32 insertions(+), 72 deletions(-)
>
...
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index adcc9bf..69d626e 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -22,24 +22,22 @@ struct xfs_inode;
> struct xfs_dinode;
>
> /*
> - * In memory representation of the XFS inode. This is held in the in-core
> - * struct xfs_inode to represent the on disk values, but no longer needs to
> be
> - * identical to the on-disk structure as it is always translated to on-disk
> - * format specific structures at the appropriate time.
> + * In memory representation of the XFS inode. This is held in the in-core
> struct
> + * xfs_inode to represent the on disk values, but it's struct is in no way
Nit: its
> + * related to what is stored on disk. That is, this structure is always
> + * translated to on-disk format specific structures at the appropriate time.
FWIW, it might be more clear to say something like: "This is held in the
in-core struct xfs_inode and represents on-disk values, but the
structure is not in on-disk format. In other words, the structure is
always translated to on-disk format specific structures ... "
> */
> struct xfs_icdinode {
> - __uint16_t di_magic; /* inode magic # = XFS_DINODE_MAGIC */
> __uint16_t di_mode; /* mode and type of file */
> __int8_t di_version; /* inode version */
> __int8_t di_format; /* format of di_c data */
> __uint16_t di_onlink; /* old number of links to file */
> + __uint16_t di_flushiter; /* incremented on flush */
> __uint32_t di_uid; /* owner's user id */
> __uint32_t di_gid; /* owner's group id */
> __uint32_t di_nlink; /* number of links to file */
> __uint16_t di_projid_lo; /* lower part of owner's project id */
> __uint16_t di_projid_hi; /* higher part of owner's project id */
> - __uint8_t di_pad[6]; /* unused, zeroed space */
> - __uint16_t di_flushiter; /* incremented on flush */
> xfs_fsize_t di_size; /* number of bytes in file */
> xfs_rfsblock_t di_nblocks; /* # of direct & btree blocks used */
> xfs_extlen_t di_extsize; /* basic/minimum extent size for file */
...
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 9dcbf58..ae60087 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -325,12 +325,14 @@ xfs_inode_item_format_attr_fork(
> static void
> xfs_inode_to_log_dinode(
> struct xfs_inode *ip,
> - struct xfs_log_dinode *to)
> + struct xfs_log_dinode *to,
> + xfs_lsn_t lsn)
> {
> struct xfs_icdinode *from = &ip->i_d;
> struct inode *inode = VFS_I(ip);
>
> - to->di_magic = from->di_magic;
> + to->di_magic = XFS_DINODE_MAGIC;
> +
> to->di_mode = from->di_mode;
> to->di_version = from->di_version;
> to->di_format = from->di_format;
> @@ -340,8 +342,8 @@ xfs_inode_to_log_dinode(
> to->di_nlink = from->di_nlink;
> to->di_projid_lo = from->di_projid_lo;
> to->di_projid_hi = from->di_projid_hi;
> - memcpy(to->di_pad, from->di_pad, sizeof(to->di_pad));
>
> + memset(to->di_pad, 0, sizeof(to->di_pad));
> to->di_atime.t_sec = inode->i_atime.tv_sec;
> to->di_atime.t_nsec = inode->i_atime.tv_nsec;
> to->di_mtime.t_sec = inode->i_mtime.tv_sec;
> @@ -366,10 +368,11 @@ xfs_inode_to_log_dinode(
> to->di_crtime.t_sec = from->di_crtime.t_sec;
> to->di_crtime.t_nsec = from->di_crtime.t_nsec;
> to->di_flags2 = from->di_flags2;
> - to->di_ino = from->di_ino;
> - to->di_lsn = from->di_lsn;
> - memcpy(to->di_pad2, from->di_pad2, sizeof(to->di_pad2));
> - uuid_copy(&to->di_uuid, &from->di_uuid);
> +
> + to->di_ino = ip->i_ino;
> + to->di_lsn = lsn;
> + memset(to->di_pad2, 0, sizeof(to->di_pad2));
> + uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_uuid);
Shouldn't this be sb_meta_uuid?
Brian
> to->di_flushiter = 0;
> } else {
> to->di_flushiter = from->di_flushiter;
> @@ -390,7 +393,7 @@ xfs_inode_item_format_core(
> struct xfs_log_dinode *dic;
>
> dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
> - xfs_inode_to_log_dinode(ip, dic);
> + xfs_inode_to_log_dinode(ip, dic, ip->i_itemp->ili_item.li_lsn);
> xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_d.di_version));
> }
>
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
|