Re: [PATCH 02/16 v2] f2fs: add on-disk layout

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On Tue, 23 Oct 2012 11:26:00 +0900 Jaegeuk Kim <jaegeuk.kim@samsung.com>
wrote:

> This adds a header file describing the on-disk layout of f2fs.
> 


> +struct f2fs_inode {
> +	__le16 i_mode;			/* File mode */
> +	__u8 i_advise;			/* File hints */
> +	__u8 i_reserved;		/* Reserved */
> +	__le32 i_uid;			/* User ID */
> +	__le32 i_gid;			/* Group ID */
> +	__le32 i_links;			/* Links count */
> +	__le64 i_size;			/* File size in bytes */
> +	__le64 i_blocks;		/* File size in blocks */
> +	__le64 i_ctime;			/* Inode change time */
> +	__le64 i_mtime;			/* Modification time */
> +	__le32 i_ctime_nsec;
> +	__le32 i_mtime_nsec;
> +	__le32 current_depth;
> +	__le32 i_xattr_nid;		/* nid to save xattr */
> +	__le32 i_flags;			/* file attributes */
> +	__le32 i_pino;			/* parent inode number */
> +	__le32 i_namelen;		/* file name length */
> +	__u8 i_name[F2FS_MAX_NAME_LEN];	/* file name for SPOR */
> +
> +	struct f2fs_extent i_ext;	/* caching a largest extent */
> +
> +	__le32 i_addr[ADDRS_PER_INODE];	/* Pointers to data blocks */
> +
> +	__le32 i_nid[5];		/* direct(2), indirect(2),
> +						double_indirect(1) node id */
> +} __packed;
> +


You appear to have dropped i_btime - no big deal, you weren't using it anyway.
However if you ever want to support NFS export you will need some value which
is assigned when the inode is allocated and never changed until it is
de-allocated.  This is used to detect when an NFS file-handle refers to a
previous incarnation of an inode and so should be rejected as STALE.
i_btime could have possibly provided this, but not any more.  You might want
to add something back.
ext3 uses "i_generation" and has an 's_next_generation' in the superblock to
ensure that each new inode gets a new generation number.

You've also dropped i_atime.  I can certainly understand the desire to do
that, but I wonder if it is entirely wise.  There are some use-cases where
i_mtime is a poor substitute.

Also 'current_depth' looks a little odd without a 'i_' prefix.  It wouldn't
hurt to have a comment noting that it is for directories.

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]