xfs: metadata CRCs, third version

From: Dave Chinner <[email protected]>

Currently xfs_corruption_error() dumps the first 16 bytes of the buffer that is passed to it when a corruption occurs. This is not large enough to see the entire state of the header of the block that was determined to be corrupt. increase the output to 64 bytes to capture the majority of all headers in all types of metadata blocks.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_error.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_error.c +0 -0

index 07bf3b9..35d3f5b 100644

Quoted 13 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Currently xfs_corruption_error() dumps the first 16 bytes of the buffer that is passed to it when a corruption occurs. This is not large enough to see the entire state of the header of the block that was determined to be corrupt. increase the output to 64 bytes to capture the majority of all headers in all types of metadata blocks.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_error.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_error.c xfs_corruption_error( · line 181

Quoted 10 lines

fs/xfs/xfs_error.c +1 -1

index 07bf3b9..35d3f5b 100644
@@ -178,7 +178,7 @@ xfs_corruption_error(
 	inst_t			*ra)
 {
 	if (level <= xfs_error_level)
-		xfs_hex_dump(p, 16);
+		xfs_hex_dump(p, 64);

Are you sure we don't have any structures which are shorter than that? I guess we'll have to take a look.

From: Christoph Hellwig <[email protected]>

Add a new inode version with a larger core. The primary objective is to allow for a crc of the inode, and location information (uuid and ino) to verify it was written in the right place. We also extend it by:

	a creation time (for Samba);
	a changecount (for NFSv4);
	a flush sequence (in LSN format for recovery);
	an additional inode flags field; and
	some additional padding.

These additional fields are not implemented yet, but already laid out in the structure.

[[email protected]] Added LSN and flags field, some factoring and rework to capture all the necessary information in the crc calculation.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_buf_item.h    |    4 +-
 fs/xfs/xfs_dinode.h      |   33 +++++++--
 fs/xfs/xfs_ialloc.c      |   23 +++++-
 fs/xfs/xfs_inode.c       |  179 ++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_inode.h       |   26 +++++++
 fs/xfs/xfs_inode_item.c  |    2 +-
 fs/xfs/xfs_log_recover.c |   35 +++++++--
 fs/xfs/xfs_trans_buf.c   |    5 +-
 8 files changed, 237 insertions(+), 70 deletions(-)

fs/xfs/xfs_buf_item.h +3 -1

index c256606..abae8c8 100644
@@ -48,6 +48,7 @@ extern kmem_zone_t	*xfs_buf_item_zone;
 #define XFS_BLF_AGF_BUF		(1<<6)
 #define XFS_BLF_AGFL_BUF	(1<<7)
 #define XFS_BLF_AGI_BUF		(1<<8)
+#define XFS_BLF_DINO_BUF	(1<<9)
 
 #define XFS_BLF_TYPE_MASK	\
 		(XFS_BLF_UDQUOT_BUF | \

@@ -56,7 +57,8 @@ extern kmem_zone_t	*xfs_buf_item_zone;
 		 XFS_BLF_BTREE_BUF | \
 		 XFS_BLF_AGF_BUF | \
 		 XFS_BLF_AGFL_BUF | \
-		 XFS_BLF_AGI_BUF)
+		 XFS_BLF_AGI_BUF | \
+		 XFS_BLF_DINO_BUF)
 
 #define	XFS_BLF_CHUNK		128
 #define	XFS_BLF_SHIFT		7

fs/xfs/xfs_dinode.h +29 -4

index 6b5bd17..bdc946a 100644
@@ -19,7 +19,7 @@
 #define	__XFS_DINODE_H__
 
 #define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
-#define XFS_DINODE_GOOD_VERSION(v)	(((v) == 1 || (v) == 2))
+#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
 
 typedef struct xfs_timestamp {
 	__be32		t_sec;		/* timestamp seconds */

@@ -70,11 +70,36 @@ typedef struct xfs_dinode {
 
 	/* di_next_unlinked is the only non-core field in the old dinode */
 	__be32		di_next_unlinked;/* agi unlinked list ptr */
-} __attribute__((packed)) xfs_dinode_t;
+
+	/* start of the extended dinode, writable fields */
+	__be32		di_crc;		/* CRC of the inode */
+	__be64		di_changecount;	/* number of attribute changes */
+	__be64		di_lsn;		/* flush sequence */
+	__be64		di_flags2;	/* more random flags */
+	__u8		di_pad2[16];	/* more padding for future expansion */
+
+	/* fields only written to during inode creation */
+	xfs_timestamp_t	di_crtime;	/* time created */
+	__be64		di_ino;		/* inode number */
+	uuid_t		di_uuid;	/* UUID of the filesystem */
+
+	/* structure must be padded to 64 bit alignment */
+} xfs_dinode_t;
 
 #define DI_MAX_FLUSH 0xffff
 
 /*
+ * Size of the core inode on disk.  Version 1 and 2 inodes have
+ * the same size, but version 3 has grown a few additional fields.
+ */
+static inline uint xfs_dinode_size(int version)
+{
+	if (version == 3)
+		return sizeof(struct xfs_dinode);
+	return offsetof(struct xfs_dinode, di_crc);
+}
+
+/*
  * The 32 bit link count in the inode theoretically maxes out at UINT_MAX.
  * Since the pathconf interface is signed, we use 2^31 - 1 instead.
  * The old inode format had a 16 bit link count, so its maximum is USHRT_MAX.

@@ -105,7 +130,7 @@ typedef enum xfs_dinode_fmt {
  * Inode size for given fs.
  */
 #define XFS_LITINO(mp, version) \
-	((int)(((mp)->m_sb.sb_inodesize) - sizeof(struct xfs_dinode)))
+	((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
 
 #define XFS_BROOT_SIZE_ADJ(ip) \
 	(XFS_BMBT_BLOCK_LEN((ip)->i_mount) - sizeof(xfs_bmdr_block_t))

@@ -133,7 +158,7 @@ typedef enum xfs_dinode_fmt {
  * Return pointers to the data or attribute forks.
  */
 #define XFS_DFORK_DPTR(dip) \
-	((char *)(dip) + sizeof(struct xfs_dinode))
+	((char *)dip + xfs_dinode_size(dip->di_version))
 #define XFS_DFORK_APTR(dip)	\
 	(XFS_DFORK_DPTR(dip) + XFS_DFORK_BOFF(dip))
 #define XFS_DFORK_PTR(dip,w)	\

fs/xfs/xfs_ialloc.c +1 -0

index 6d0a495..8ceaa11 100644
@@ -167,6 +167,7 @@ xfs_ialloc_inode_init(
 	int			version;
 	int			i, j;
 	xfs_daddr_t		d;
+	xfs_ino_t		ino = 0;
 
 	/*
 	 * Loop over the new block(s), filling in the inodes.

fs/xfs/xfs_ialloc.c +1 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);

fs/xfs/xfs_ialloc.c +0 -0

 		}
 		xfs_trans_inode_alloc_buf(tp, fbuf);

fs/xfs/xfs_inode.c +125 -54

index 202ce37..558ef49 100644
@@ -44,6 +44,7 @@
 #include "xfs_quota.h"
 #include "xfs_filestream.h"
 #include "xfs_vnodeops.h"
+#include "xfs_cksum.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
 

@@ -866,6 +867,17 @@ xfs_dinode_from_disk(
 	to->di_dmstate	= be16_to_cpu(from->di_dmstate);
 	to->di_flags	= be16_to_cpu(from->di_flags);
 	to->di_gen	= be32_to_cpu(from->di_gen);
+
+	if (to->di_version == 3) {
+		to->di_changecount = be64_to_cpu(from->di_changecount);
+		to->di_crtime.t_sec = be32_to_cpu(from->di_crtime.t_sec);
+		to->di_crtime.t_nsec = be32_to_cpu(from->di_crtime.t_nsec);
+		to->di_flags2 = be64_to_cpu(from->di_flags2);
+		to->di_ino = be64_to_cpu(from->di_ino);
+		to->di_lsn = be64_to_cpu(from->di_lsn);
+		memcpy(to->di_pad2, from->di_pad2, sizeof(to->di_pad2));
+		uuid_copy(&to->di_uuid, &from->di_uuid);
+	}
 }
 
 void

@@ -902,6 +914,17 @@ xfs_dinode_to_disk(
 	to->di_dmstate = cpu_to_be16(from->di_dmstate);
 	to->di_flags = cpu_to_be16(from->di_flags);
 	to->di_gen = cpu_to_be32(from->di_gen);
+
+	if (from->di_version == 3) {
+		to->di_changecount = cpu_to_be64(from->di_changecount);
+		to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.t_sec);
+		to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.t_nsec);
+		to->di_flags2 = cpu_to_be64(from->di_flags2);
+		to->di_ino = cpu_to_be64(from->di_ino);
+		to->di_lsn = cpu_to_be64(from->di_lsn);
+		memcpy(to->di_pad2, from->di_pad2, sizeof(to->di_pad2));
+		uuid_copy(&to->di_uuid, &from->di_uuid);
+	}
 }
 
 STATIC uint

@@ -962,6 +985,47 @@ xfs_dic2xflags(
 				(XFS_DFORK_Q(dip) ? XFS_XFLAG_HASATTR : 0);
 }
 
+static bool
+xfs_dinode_verify(
+	struct xfs_mount	*mp,
+	struct xfs_inode	*ip,
+	struct xfs_dinode	*dip)
+{
+	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))
+		return false;
+
+	/* only version 3 or greater inodes are extensively verified here */
+	if (dip->di_version < 3)
+		return true;
+
+	if (!xfs_sb_version_hascrc(&mp->m_sb))
+		return false;
+	if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
+			      offsetof(struct xfs_dinode, di_crc)))
+		return false;
+	if (be64_to_cpu(dip->di_ino) != ip->i_ino)
+		return false;
+	if (!uuid_equal(&dip->di_uuid, &mp->m_sb.sb_uuid))
+		return false;
+	return true;
+}
+
+void
+xfs_dinode_calc_crc(
+	struct xfs_mount	*mp,
+	struct xfs_dinode	*dip)
+{
+	__uint32_t		crc;
+
+	if (dip->di_version < 3)
+		return;
+
+	ASSERT(xfs_sb_version_hascrc(&mp->m_sb));
+	crc = xfs_start_cksum((char *)dip, mp->m_sb.sb_inodesize,
+			      offsetof(struct xfs_dinode, di_crc));
+	dip->di_crc = xfs_end_cksum(crc);
+}
+
 /*
  * Read the disk inode attributes into the in-core inode structure.
  */

@@ -990,17 +1054,13 @@ xfs_iread(
 	if (error)
 		return error;
 
-	/*
-	 * If we got something that isn't an inode it means someone
-	 * (nfs or dmi) has a stale handle.
-	 */
-	if (dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC)) {
-#ifdef DEBUG
-		xfs_alert(mp,
-			"%s: dip->di_magic (0x%x) != XFS_DINODE_MAGIC (0x%x)",
-			__func__, be16_to_cpu(dip->di_magic), XFS_DINODE_MAGIC);
-#endif /* DEBUG */
-		error = XFS_ERROR(EINVAL);
+	/* even unallocated inodes are verified */
+	if (!xfs_dinode_verify(mp, ip, dip)) {
+		xfs_alert(mp, "%s: validation failed for inode %lld failed",
+				__func__, ip->i_ino);
+
+		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp, dip);
+		error = XFS_ERROR(EFSCORRUPTED);
 		goto out_brelse;
 	}
 

@@ -1022,10 +1082,20 @@ xfs_iread(
 			goto out_brelse;
 		}
 	} else {
+		/*
+		 * Partial initialisation of the in-core inode. Just the bits
+		 * that xfs_ialloc won't overwrite or relies on being correct.
+		 */
 		ip->i_d.di_magic = be16_to_cpu(dip->di_magic);
 		ip->i_d.di_version = dip->di_version;
 		ip->i_d.di_gen = be32_to_cpu(dip->di_gen);
 		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
+
+		if (dip->di_version == 3) {
+			ip->i_d.di_ino = be64_to_cpu(dip->di_ino);
+			uuid_copy(&ip->i_d.di_uuid, &dip->di_uuid);
+		}
+
 		/*
 		 * Make sure to pull in the mode here as well in
 		 * case the inode is released without being used.

@@ -1161,6 +1231,7 @@ xfs_ialloc(
 	xfs_buf_t	**ialloc_context,
 	xfs_inode_t	**ipp)
 {
+	struct xfs_mount *mp = tp->t_mountp;
 	xfs_ino_t	ino;
 	xfs_inode_t	*ip;
 	uint		flags;

@@ -1187,7 +1258,7 @@ xfs_ialloc(
 	 * This is because we're setting fields here we need
 	 * to prevent others from looking at until we're done.
 	 */
-	error = xfs_iget(tp->t_mountp, tp, ino, XFS_IGET_CREATE,
+	error = xfs_iget(mp, tp, ino, XFS_IGET_CREATE,
 			 XFS_ILOCK_EXCL, &ip);
 	if (error)
 		return error;

@@ -1208,7 +1279,7 @@ xfs_ialloc(
 	 * the inode version number now.  This way we only do the conversion
 	 * here rather than here and in the flush/logging code.
 	 */
-	if (xfs_sb_version_hasnlink(&tp->t_mountp->m_sb) &&
+	if (xfs_sb_version_hasnlink(&mp->m_sb) &&
 	    ip->i_d.di_version == 1) {
 		ip->i_d.di_version = 2;
 		/*

@@ -1258,6 +1329,19 @@ xfs_ialloc(
 	ip->i_d.di_dmevmask = 0;
 	ip->i_d.di_dmstate = 0;
 	ip->i_d.di_flags = 0;
+
+	if (ip->i_d.di_version == 3) {
+		ASSERT(ip->i_d.di_ino == ino);
+		ASSERT(uuid_equal(&ip->i_d.di_uuid, &mp->m_sb.sb_uuid));
+		ip->i_d.di_crc = 0;
+		ip->i_d.di_changecount = 1;
+		ip->i_d.di_lsn = 0;
+		ip->i_d.di_flags2 = 0;
+		memset(&(ip->i_d.di_pad2[0]), 0, sizeof(ip->i_d.di_pad2));
+		ip->i_d.di_crtime = ip->i_d.di_mtime;
+	}
+
+
 	flags = XFS_ILOG_CORE;
 	switch (mode & S_IFMT) {
 	case S_IFIFO:

@@ -2716,20 +2800,18 @@ abort_out:
 
 STATIC int
 xfs_iflush_int(
-	xfs_inode_t		*ip,
-	xfs_buf_t		*bp)
+	struct xfs_inode	*ip,
+	struct xfs_buf		*bp)
 {
-	xfs_inode_log_item_t	*iip;
-	xfs_dinode_t		*dip;
-	xfs_mount_t		*mp;
+	struct xfs_inode_log_item *iip = ip->i_itemp;
+	struct xfs_dinode	*dip;
+	struct xfs_mount	*mp = ip->i_mount;
 
 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL|XFS_ILOCK_SHARED));
 	ASSERT(xfs_isiflocked(ip));
 	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
 	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
-
-	iip = ip->i_itemp;
-	mp = ip->i_mount;
+	ASSERT(iip != NULL && iip->ili_fields != 0);
 
 	/* set *dip = inode's place in the buffer */
 	dip = (xfs_dinode_t *)xfs_buf_offset(bp, ip->i_imap.im_boffset);

@@ -2790,9 +2872,9 @@ xfs_iflush_int(
 	}
 	/*
 	 * bump the flush iteration count, used to detect flushes which
-	 * postdate a log record during recovery.
+	 * postdate a log record during recovery. This is redundant as we now
+	 * log every change and hence this can't happen. Still, it doesn't hurt.
 	 */
-
 	ip->i_d.di_flushiter++;
 
 	/*

@@ -2868,41 +2950,30 @@ xfs_iflush_int(
 	 * need the AIL lock, because it is a 64 bit value that cannot be read
 	 * atomically.
 	 */
-	if (iip != NULL && iip->ili_fields != 0) {
-		iip->ili_last_fields = iip->ili_fields;
-		iip->ili_fields = 0;
-		iip->ili_logged = 1;
+	iip->ili_last_fields = iip->ili_fields;
+	iip->ili_fields = 0;
+	iip->ili_logged = 1;
 
-		xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
-					&iip->ili_item.li_lsn);
+	xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
+				&iip->ili_item.li_lsn);
 
-		/*
-		 * Attach the function xfs_iflush_done to the inode's
-		 * buffer.  This will remove the inode from the AIL
-		 * and unlock the inode's flush lock when the inode is
-		 * completely written to disk.
-		 */
-		xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
+	/*
+	 * Attach the function xfs_iflush_done to the inode's
+	 * buffer.  This will remove the inode from the AIL
+	 * and unlock the inode's flush lock when the inode is
+	 * completely written to disk.
+	 */
+	xfs_buf_attach_iodone(bp, xfs_iflush_done, &iip->ili_item);
 
-		ASSERT(bp->b_fspriv != NULL);
-		ASSERT(bp->b_iodone != NULL);
-	} else {
-		/*
-		 * We're flushing an inode which is not in the AIL and has
-		 * not been logged.  For this case we can immediately drop
-		 * the inode flush lock because we can avoid the whole
-		 * AIL state thing.  It's OK to drop the flush lock now,
-		 * because we've already locked the buffer and to do anything
-		 * you really need both.
-		 */
-		if (iip != NULL) {
-			ASSERT(iip->ili_logged == 0);
-			ASSERT(iip->ili_last_fields == 0);
-			ASSERT((iip->ili_item.li_flags & XFS_LI_IN_AIL) == 0);
-		}
-		xfs_ifunlock(ip);
-	}
+	/* update the lsn in the on disk inode if required */
+	if (ip->i_d.di_version == 3)
+		dip->di_lsn = cpu_to_be64(iip->ili_item.li_lsn);
+
+	/* generate the checksum. */
+	xfs_dinode_calc_crc(mp, dip);
 
+	ASSERT(bp->b_fspriv != NULL);
+	ASSERT(bp->b_iodone != NULL);
 	return 0;
 
 corrupt_out:

fs/xfs/xfs_inode.h +26 -0

index b8520b5..9112979 100644
@@ -150,13 +150,38 @@ typedef struct xfs_icdinode {
 	__uint16_t	di_dmstate;	/* DMIG state info */
 	__uint16_t	di_flags;	/* random flags, XFS_DIFLAG_... */
 	__uint32_t	di_gen;		/* generation number */
+
+	/* di_next_unlinked is the only non-core field in the old dinode */
+	xfs_agino_t	di_next_unlinked;/* agi unlinked list ptr */
+
+	/* start of the extended dinode, writable fields */
+	__uint32_t	di_crc;		/* CRC of the inode */
+	__uint64_t	di_changecount;	/* number of attribute changes */
+	xfs_lsn_t	di_lsn;		/* flush sequence */
+	__uint64_t	di_flags2;	/* more random flags */
+	__uint8_t	di_pad2[16];	/* more padding for future expansion */
+
+	/* fields only written to during inode creation */
+	xfs_ictimestamp_t di_crtime;	/* time created */
+	xfs_ino_t	di_ino;		/* inode number */
+	uuid_t		di_uuid;	/* UUID of the filesystem */
+
+	/* structure must be padded to 64 bit alignment */
 } xfs_icdinode_t;
 
+static inline uint xfs_icdinode_size(int version)
+{
+	if (version == 3)
+		return sizeof(struct xfs_icdinode);
+	return offsetof(struct xfs_icdinode, di_next_unlinked);
+}
+
 /*
  * Flags for xfs_ichgtime().
  */
 #define	XFS_ICHGTIME_MOD	0x1	/* data fork modification timestamp */
 #define	XFS_ICHGTIME_CHG	0x2	/* inode field change timestamp */
+#define	XFS_ICHGTIME_CREATE	0x4	/* inode create timestamp */
 
 /*
  * Per-fork incore inode flags.

@@ -556,6 +581,7 @@ int		xfs_imap_to_bp(struct xfs_mount *, struct xfs_trans *,
 			       struct xfs_buf **, uint, uint);
 int		xfs_iread(struct xfs_mount *, struct xfs_trans *,
 			  struct xfs_inode *, uint);
+void		xfs_dinode_calc_crc(struct xfs_mount *, struct xfs_dinode *);
 void		xfs_dinode_to_disk(struct xfs_dinode *,
 				   struct xfs_icdinode *);
 void		xfs_idestroy_fork(struct xfs_inode *, int);

fs/xfs/xfs_inode_item.c +1 -1

index f034bd1..f76ff52 100644
@@ -179,7 +179,7 @@ xfs_inode_item_format(
 	nvecs	     = 1;
 
 	vecp->i_addr = &ip->i_d;
-	vecp->i_len  = sizeof(struct xfs_icdinode);
+	vecp->i_len  = xfs_icdinode_size(ip->i_d.di_version);
 	vecp->i_type = XLOG_REG_TYPE_ICORE;
 	vecp++;
 	nvecs++;

fs/xfs/xfs_log_recover.c +1 -0

index d750c34..6d08eaa 100644
@@ -1786,6 +1786,7 @@ xlog_recover_do_inode_buffer(
 	xfs_agino_t		*buffer_nextp;
 
 	trace_xfs_log_recover_buf_inode_buf(mp->m_log, buf_f);
+	bp->b_ops = &xfs_inode_buf_ops;
 
 	inodes_per_buf = BBTOB(bp->b_io_length) >> mp->m_sb.sb_inodelog;
 	for (i = 0; i < inodes_per_buf; i++) {

Dave,

Quoted 12 lines · Dave Chinner

From: Christoph Hellwig <[email protected]>

Add a new inode version with a larger core. The primary objective is to allow for a crc of the inode, and location information (uuid and ino) to verify it was written in the right place. We also extend it by:

	a creation time (for Samba);
	a changecount (for NFSv4);
	a flush sequence (in LSN format for recovery);
	an additional inode flags field; and
	some additional padding.

Quoted 5 lines

These additional fields are not implemented yet, but already laid out in the structure.

[[email protected]] Added LSN and flags field, some factoring and rework to capture all the necessary information in the crc calculation.

Comments and questions below.

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 199

Quoted 10 lines

fs/xfs/xfs_ialloc.c +6 -0

@@ -190,8 +191,18 @@ xfs_ialloc_inode_init(
 	 * the new inode format, then use the new inode version.  Otherwise
 	 * use the old version so that old kernels will continue to be
 	 * able to use the file system.
+	 *
+	 * For v3 inodes, we also need to write the inode number into the inode,
+	 * so calculate the first inode number of the chunk here as
+	 * XFS_OFFBNO_TO_AGINO() only works on filesystem block boundaries, not
+	 * cluster boundaries and so cannot be used in the cluster buffer loop
+	 * below.

I'm having some trouble understanding your comment. Maybe you can help me:

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init(

Quoted 11 lines

fs/xfs/xfs_ialloc.c +5 -1

 	 */
-	if (xfs_sb_version_hasnlink(&mp->m_sb))
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		version = 3;
+		ino = XFS_AGINO_TO_INO(mp, agno,
+				       XFS_OFFBNO_TO_AGINO(mp, agbno, 0));
+	} else if (xfs_sb_version_hasnlink(&mp->m_sb))
 		version = 2;
 	else
 		version = 1;

@@ -217,13 +228,21 @@ xfs_ialloc_inode_init(

My reading of the loop here is ...

210 for (j = 0; j < nbufs; j++) {

for each inode cluster, j

211 /*
212 * Get the block.
213 */
214 d = XFS_AGB_TO_DADDR(mp, agno, agbno + (j * blks_per_cluster));

convert to disk address ( this AG, the AGBLOCK of the initial inode cluster plus
(current cluster j * blocks per cluster))

215 fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
216 mp->m_bsize * blks_per_cluster,
217 XBF_UNMAPPED);

get a buffer at that disk address of length (filesystem block size times the number of blocks per cluster)

which is the full length of the inode cluster

218 if (!fbuf)
219 return ENOMEM;
220 /*

221                  * Initialize all inodes in this buffer and then log them.
222                  *
223                  * XXX: It would be much better if we had just one transaction
224                  *      to log a whole cluster of inodes instead of all the
225                  *      individual transactions causing a lot of log traffic.
226                  */

227 fbuf->b_ops = &xfs_inode_buf_ops;
228 xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);

Zero the whole cluster, including literal areas

229 for (i = 0; i < ninodes; i++) {

for each inode, i

230                         int     ioffset = i << mp->m_sb.sb_inodelog;
231                         uint    isize = xfs_dinode_size(version);

232
233 free = xfs_make_iptr(mp, fbuf, i);

get a pointer into the buf to the beginning of i's inode core

234                         free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
235                         free->di_version = version;
236                         free->di_gen = cpu_to_be32(gen);
237                         free->di_next_unlinked = cpu_to_be32(NULLAGINO);

initialize some important stuff

238
239 if (version == 3) {
240 free->di_ino = cpu_to_be64(ino);
241 ino++;

initialize ino on verion 3 inodes. and add one to ino for the next run of this loop.

It appears that for subsequent clusters where j > 1 this would stamp the wrong

Dave,

Quoted 12 lines · Dave Chinner

From: Christoph Hellwig <[email protected]>

Add a new inode version with a larger core. The primary objective is to allow for a crc of the inode, and location information (uuid and ino) to verify it was written in the right place. We also extend it by:

	a creation time (for Samba);
	a changecount (for NFSv4);
	a flush sequence (in LSN format for recovery);
	an additional inode flags field; and
	some additional padding.

Quoted 5 lines

These additional fields are not implemented yet, but already laid out in the structure.

[[email protected]] Added LSN and flags field, some factoring and rework to capture all the necessary information in the crc calculation.

Comments and questions below.

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 199

Quoted 10 lines

fs/xfs/xfs_ialloc.c +6 -0

@@ -190,8 +191,18 @@ xfs_ialloc_inode_init(
 	 * the new inode format, then use the new inode version.  Otherwise
 	 * use the old version so that old kernels will continue to be
 	 * able to use the file system.
+	 *
+	 * For v3 inodes, we also need to write the inode number into the inode,
+	 * so calculate the first inode number of the chunk here as
+	 * XFS_OFFBNO_TO_AGINO() only works on filesystem block boundaries, not
+	 * cluster boundaries and so cannot be used in the cluster buffer loop
+	 * below.

I'm having some trouble understanding your comment. Maybe you can help me:

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init(

Quoted 11 lines

fs/xfs/xfs_ialloc.c +5 -1

 	 */
-	if (xfs_sb_version_hasnlink(&mp->m_sb))
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		version = 3;
+		ino = XFS_AGINO_TO_INO(mp, agno,
+				       XFS_OFFBNO_TO_AGINO(mp, agbno, 0));
+	} else if (xfs_sb_version_hasnlink(&mp->m_sb))
 		version = 2;
 	else
 		version = 1;

@@ -217,13 +228,21 @@ xfs_ialloc_inode_init(

My reading of the loop here is ...

210 for (j = 0; j < nbufs; j++) {

for each inode cluster, j

211 /*
212 * Get the block.
213 */
214 d = XFS_AGB_TO_DADDR(mp, agno, agbno + (j * blks_per_cluster));

convert to disk address ( this AG, the AGBLOCK of the initial inode cluster plus
(current cluster j * blocks per cluster))

215 fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
216 mp->m_bsize * blks_per_cluster,
217 XBF_UNMAPPED);

get a buffer at that disk address of length (filesystem block size times the number of blocks per cluster)

which is the full length of the inode cluster

218 if (!fbuf)
219 return ENOMEM;
220 /*

221                  * Initialize all inodes in this buffer and then log them.
222                  *
223                  * XXX: It would be much better if we had just one transaction
224                  *      to log a whole cluster of inodes instead of all the
225                  *      individual transactions causing a lot of log traffic.
226                  */

227 fbuf->b_ops = &xfs_inode_buf_ops;
228 xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);

Zero the whole cluster, including literal areas

229 for (i = 0; i < ninodes; i++) {

for each inode, i

230                         int     ioffset = i << mp->m_sb.sb_inodelog;
231                         uint    isize = xfs_dinode_size(version);

232
233 free = xfs_make_iptr(mp, fbuf, i);

get a pointer into the buf to the beginning of i's inode core

234                         free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
235                         free->di_version = version;
236                         free->di_gen = cpu_to_be32(gen);
237                         free->di_next_unlinked = cpu_to_be32(NULLAGINO);

initialize some important stuff

238
239 if (version == 3) {
240 free->di_ino = cpu_to_be64(ino);
241 ino++;

initialize ino on verion 3 inodes. and add one to ino for the next run of this loop.

It appears that for subsequent clusters where j > 1 this would stamp the wrong ino into the inode.

Something like this would be better:
ino = XFS_AGINO_TO_INO(mp, agno,

		XFS_OFFBNO_TO_AGINO(mp, agbno + (j * blks_per_cluster), i));

free->di_ino = cpu_to_be64(ino);

sudo xfs_db -c "inode $i" -c "p v3.inumber" /dev/vdc done

Quoted 16 lines

fs/xfs/xfs_ialloc.c +6 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);
 			free->di_next_unlinked = cpu_to_be32(NULLAGINO);
+
+			if (version == 3) {
+				free->di_ino = cpu_to_be64(ino);
+				ino++;
+				uuid_copy(&free->di_uuid, &mp->m_sb.sb_uuid);

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 246

Quoted 4 lines

fs/xfs/xfs_ialloc.c +3 -0

+				xfs_dinode_calc_crc(mp, free);
+			}
+
 			xfs_trans_log_buf(tp, fbuf, ioffset, ioffset + isize - 1);

If I have it right, it's ok not to log the literal are here (even though the crc was calculated including the literal area) because the log is protected by its own crcs and recovery will recalculate the crc.

What do we have in the
literal area after log replay in that case?

Quoted 16 lines

fs/xfs/xfs_log_recover.c +1 -0

index d750c34..6d08eaa 100644
@@ -1786,6 +1786,7 @@ xlog_recover_do_inode_buffer(
 	xfs_agino_t		*buffer_nextp;
 
 	trace_xfs_log_recover_buf_inode_buf(mp->m_log, buf_f);
+	bp->b_ops = &xfs_inode_buf_ops;
 
 	inodes_per_buf = BBTOB(bp->b_io_length) >> mp->m_sb.sb_inodelog;
 	for (i = 0; i < inodes_per_buf; i++) {

@@ -1930,6 +1931,9 @@ xlog_recover_do_reg_buffer(
 	/* Shouldn't be any more regions */
 	ASSERT(i == item->ri_total);
 

fs/xfs/xfs_log_recover.c xlog_recover_do_reg_buffer( · line 1936

Quoted 3 lines

fs/xfs/xfs_log_recover.c +3 -0

+	/* Shouldn't be any more regions */
+	ASSERT(i == item->ri_total);
+

That appears to be duplicate of the assert above it.

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_ialloc.c +6 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);
 			free->di_next_unlinked = cpu_to_be32(NULLAGINO);
+
+			if (version == 3) {
+				free->di_ino = cpu_to_be64(ino);
+				ino++;
+				uuid_copy(&free->di_uuid, &mp->m_sb.sb_uuid);

Quoted 8 lines

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 246

Quoted 4 lines

fs/xfs/xfs_ialloc.c +3 -0

+				xfs_dinode_calc_crc(mp, free);
+			}
+
 			xfs_trans_log_buf(tp, fbuf, ioffset, ioffset + isize - 1);

If I have it right, it's ok not to log the literal are here (even though the crc was calculated including the literal area) because the log is protected by its own crcs and recovery will recalculate the crc.

Prior to CRCs it's OK not to log the literal areas because the contents really don't matter. The entire buffer is zeroed because it's faster than zeroing individual inode cores one by one and it ensures that we can always tell a freshly allocated inode block with xfs_db because the literal areas are all zero (i.e. good for debugging). But these are conveniences, not a necessity, and hence the advantage of not logging the literal areas reduces the overhead of logging inode allocations *significantly*.

Quoted 2 lines

What do we have in the
literal area after log replay in that case?

For non-CRC inode buffers, it doesn't matter.

But you are right that it does matter for CRC enabled inode buffers as it will result in the CRC in the inode core being incorrect. I'l havea think about this - there are a couple of potential ways of solving the problem, and I need to think about them a bit first.

Quoted 39 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_ialloc.c +6 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);
 			free->di_next_unlinked = cpu_to_be32(NULLAGINO);
+
+			if (version == 3) {
+				free->di_ino = cpu_to_be64(ino);
+				ino++;
+				uuid_copy(&free->di_uuid, &mp->m_sb.sb_uuid);

Quoted 8 lines

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 246

Quoted 4 lines

fs/xfs/xfs_ialloc.c +3 -0

+				xfs_dinode_calc_crc(mp, free);
+			}
+
 			xfs_trans_log_buf(tp, fbuf, ioffset, ioffset + isize - 1);

If I have it right, it's ok not to log the literal are here (even though the crc was calculated including the literal area) because the log is protected by its own crcs and recovery will recalculate the crc.

Prior to CRCs it's OK not to log the literal areas because the contents really don't matter. The entire buffer is zeroed because it's faster than zeroing individual inode cores one by one and it ensures that we can always tell a freshly allocated inode block with xfs_db because the literal areas are all zero (i.e. good for debugging). But these are conveniences, not a necessity, and hence the advantage of not logging the literal areas reduces the overhead of logging inode allocations *significantly*.

Quoted 2 lines

What do we have in the
literal area after log replay in that case?

Quoted 6 lines

For non-CRC inode buffers, it doesn't matter.

But you are right that it does matter for CRC enabled inode buffers as it will result in the CRC in the inode core being incorrect. I'l havea think about this - there are a couple of potential ways of solving the problem, and I need to think about them a bit first.

Ben, FYI: I've taken the easy way out for this - log the entire inode buffer rather than just the inode core. The CRC means we are dependent on having all the inode logged so that seems to be the simplest way to deal with this problem overall, even though it increases the amount of metadata logged for inode creates substantially.

I'll address this potential performance issue in future with new inode create and unlink transactions that allow us to avoid logging buffers for all inode modifications. There are other good reasons for doing this as well (e.g. avoid the subtly broken special handling of physical inode buffer logging vs logical inode logging in log recovery), so I think this is best to just take the simple option here....

Hi Dave,

Quoted 54 lines · Dave Chinner

Quoted 39 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_ialloc.c +6 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);
 			free->di_next_unlinked = cpu_to_be32(NULLAGINO);
+
+			if (version == 3) {
+				free->di_ino = cpu_to_be64(ino);
+				ino++;
+				uuid_copy(&free->di_uuid, &mp->m_sb.sb_uuid);

Quoted 8 lines

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 246

Quoted 4 lines

fs/xfs/xfs_ialloc.c +3 -0

+				xfs_dinode_calc_crc(mp, free);
+			}
+
 			xfs_trans_log_buf(tp, fbuf, ioffset, ioffset + isize - 1);

If I have it right, it's ok not to log the literal are here (even though the crc was calculated including the literal area) because the log is protected by its own crcs and recovery will recalculate the crc.

Prior to CRCs it's OK not to log the literal areas because the contents really don't matter. The entire buffer is zeroed because it's faster than zeroing individual inode cores one by one and it ensures that we can always tell a freshly allocated inode block with xfs_db because the literal areas are all zero (i.e. good for debugging). But these are conveniences, not a necessity, and hence the advantage of not logging the literal areas reduces the overhead of logging inode allocations *significantly*.

Quoted 2 lines

What do we have in the
literal area after log replay in that case?

Quoted 6 lines

For non-CRC inode buffers, it doesn't matter.

But you are right that it does matter for CRC enabled inode buffers as it will result in the CRC in the inode core being incorrect. I'l havea think about this - there are a couple of potential ways of solving the problem, and I need to think about them a bit first.

Ben, FYI: I've taken the easy way out for this - log the entire inode buffer rather than just the inode core. The CRC means we are dependent on having all the inode logged so that seems to be the simplest way to deal with this problem overall, even though it increases the amount of metadata logged for inode creates substantially.

Quoted 7 lines

I'll address this potential performance issue in future with new inode create and unlink transactions that allow us to avoid logging buffers for all inode modifications. There are other good reasons for doing this as well (e.g. avoid the subtly broken special handling of physical inode buffer logging vs logical inode logging in log recovery), so I think this is best to just take the simple option here....

It seems like this is a more general problem with fresh on-disk structures. When we calculate crc and log only part of a buffer we are prone to the crc being incorrect after log replay because the unlogged portions of the buffer are still undefined. They aren't the 0s we calculated crcs with.

I have a couple suggestions:

1) We could read the undefined garbage from disk before we initialize the structure and then calculate the crc. That way if we log only parts of the result the crc would still match after a crash.

2) Create a new transaction to write a known pattern over the entire buffer, then initialize the buffer with that pattern, calculate the crc, and still log only the parts of the buffer which were modified. In the non-crash case we still need to arrange for the buffer to be patterned after the log wraps, but it has the advantage of not having to log large structures just to zero them.

Quoted 63 lines · Ben Myers

Quoted 54 lines · Dave Chinner

Quoted 39 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 16 lines · Dave Chinner

fs/xfs/xfs_ialloc.c +6 -1

 		xfs_buf_zero(fbuf, 0, ninodes << mp->m_sb.sb_inodelog);
 		for (i = 0; i < ninodes; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = sizeof(struct xfs_dinode);
+			uint	isize = xfs_dinode_size(version);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 			free->di_version = version;
 			free->di_gen = cpu_to_be32(gen);
 			free->di_next_unlinked = cpu_to_be32(NULLAGINO);
+
+			if (version == 3) {
+				free->di_ino = cpu_to_be64(ino);
+				ino++;
+				uuid_copy(&free->di_uuid, &mp->m_sb.sb_uuid);

Quoted 8 lines

fs/xfs/xfs_ialloc.c xfs_ialloc_inode_init( · line 246

Quoted 4 lines

fs/xfs/xfs_ialloc.c +3 -0

+				xfs_dinode_calc_crc(mp, free);
+			}
+
 			xfs_trans_log_buf(tp, fbuf, ioffset, ioffset + isize - 1);

If I have it right, it's ok not to log the literal are here (even though the crc was calculated including the literal area) because the log is protected by its own crcs and recovery will recalculate the crc.

Prior to CRCs it's OK not to log the literal areas because the contents really don't matter. The entire buffer is zeroed because it's faster than zeroing individual inode cores one by one and it ensures that we can always tell a freshly allocated inode block with xfs_db because the literal areas are all zero (i.e. good for debugging). But these are conveniences, not a necessity, and hence the advantage of not logging the literal areas reduces the overhead of logging inode allocations *significantly*.

Quoted 2 lines

What do we have in the
literal area after log replay in that case?

Quoted 6 lines

For non-CRC inode buffers, it doesn't matter.

But you are right that it does matter for CRC enabled inode buffers as it will result in the CRC in the inode core being incorrect. I'l havea think about this - there are a couple of potential ways of solving the problem, and I need to think about them a bit first.

Ben, FYI: I've taken the easy way out for this - log the entire inode buffer rather than just the inode core. The CRC means we are dependent on having all the inode logged so that seems to be the simplest way to deal with this problem overall, even though it increases the amount of metadata logged for inode creates substantially.

Quoted 7 lines

I'll address this potential performance issue in future with new inode create and unlink transactions that allow us to avoid logging buffers for all inode modifications. There are other good reasons for doing this as well (e.g. avoid the subtly broken special handling of physical inode buffer logging vs logical inode logging in log recovery), so I think this is best to just take the simple option here....

Quoted 5 lines

It seems like this is a more general problem with fresh on-disk structures. When we calculate crc and log only part of a buffer we are prone to the crc being incorrect after log replay because the unlogged portions of the buffer are still undefined. They aren't the 0s we calculated crcs with.

But it doesn't matter for all other metadata as we don't log CRC fields except in the inode/dquot at allocation. It is the exception rather than the rule.

Quoted 2 lines

I have a couple suggestions:

Quoted 3 lines

1) We could read the undefined garbage from disk before we initialize the structure and then calculate the crc. That way if we log only parts of the result the crc would still match after a crash.

The overhead of reading every inode cluster from disk during allocation will drop create performance by orders of magnitude. i.e. far worse in terms of performance than logging the entire buffer.

Quoted 7 lines

2) Create a new transaction to write a known pattern over the entire buffer, then initialize the buffer with that pattern, calculate the crc, and still log only the parts of the buffer which were modified. In the non-crash case we still need to arrange for the buffer to be patterned after the log wraps, but it has the advantage of not having to log large structures just to zero them.

We need to ensure we log the entire object if we are logging the CRC of the object.

In this case, the initialisation and calculation of
the CRC needs to be atomic, so it needs to be a single transactions.

That's what logging the entire buffer does.

Quoted 27 lines · Dave Chinner

Quoted 17 lines · Ben Myers

Quoted 8 lines · Dave Chinner

On Fri, Mar 15, 2013 at 12:11:04PM +1100, Dave Chinner wrote:
Ben, FYI: I've taken the easy way out for this - log the entire inode buffer rather than just the inode core. The CRC means we are dependent on having all the inode logged so that seems to be the simplest way to deal with this problem overall, even though it increases the amount of metadata logged for inode creates substantially.

Quoted 7 lines

I'll address this potential performance issue in future with new inode create and unlink transactions that allow us to avoid logging buffers for all inode modifications. There are other good reasons for doing this as well (e.g. avoid the subtly broken special handling of physical inode buffer logging vs logical inode logging in log recovery), so I think this is best to just take the simple option here....

Quoted 5 lines

It seems like this is a more general problem with fresh on-disk structures. When we calculate crc and log only part of a buffer we are prone to the crc being incorrect after log replay because the unlogged portions of the buffer are still undefined. They aren't the 0s we calculated crcs with.

But it doesn't matter for all other metadata as we don't log CRC fields except in the inode/dquot at allocation. It is the exception rather than the rule.

Quoted 8 lines

Quoted 7 lines

2) Create a new transaction to write a known pattern over the entire buffer, then initialize the buffer with that pattern, calculate the crc, and still log only the parts of the buffer which were modified. In the non-crash case we still need to arrange for the buffer to be patterned after the log wraps, but it has the advantage of not having to log large structures just to zero them.

Quoted 2 lines

We need to ensure we log the entire object if we are logging the CRC of the object.

We don't need to log the entire object if we can arrange for the contents of the buffer to be a known pattern after recovery and then calculate the CRC against that. It's just the initialization that is problematic. The rest of the time the contents are already cached anyway.

Quoted 2 lines

In this case, the initialisation and calculation of
the CRC needs to be atomic, so it needs to be a single transactions.

I agree that the initialisation of the block and the calculation of the crc must be in the same transaction. It would need to be a new log item type that specifies a pattern (normally zero) and a length to be written to the buffer. I used the wrong terminology, as usual.

Quoted 1 line

That's what logging the entire buffer does.

Yep. I'm just pointing out that if logging the entire structure becomes an issue we have some other options.

This could be useful for other reasons too, e.g. to prevent stale data exposure after a crash.

From: Christoph Hellwig <[email protected]>

Add a new inode version with a larger core. The primary objective is to allow for a crc of the inode, and location information (uuid and ino) to verify it was written in the right place. We also extend it by:

	a creation time (for Samba);
	a changecount (for NFSv4);
	a flush sequence (in LSN format for recovery);
	an additional inode flags field; and
	some additional padding.

These additional fields are not implemented yet, but already laid out in the structure.

[[email protected]] Added LSN and flags field, some factoring and rework to capture all the necessary information in the crc calculation.

Signed-off-by Christoph Hellwig <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/xfs_buf_item.h    |    4 +-
 fs/xfs/xfs_dinode.h      |   33 +++++++--
 fs/xfs/xfs_ialloc.c      |   23 +++++-
 fs/xfs/xfs_inode.c       |  179 ++++++++++++++++++++++++++++++++--------------
 fs/xfs/xfs_inode.h       |   26 +++++++
 fs/xfs/xfs_inode_item.c  |    2 +-
 fs/xfs/xfs_log_recover.c |   35 +++++++--
 fs/xfs/xfs_trans_buf.c   |    5 +-
 8 files changed, 237 insertions(+), 70 deletions(-)

fs/xfs/xfs_dinode.h +1 -1

index 6b5bd17..bdc946a 100644
@@ -19,7 +19,7 @@
 #define	__XFS_DINODE_H__
 
 #define	XFS_DINODE_MAGIC		0x494e	/* 'IN' */
-#define XFS_DINODE_GOOD_VERSION(v)	(((v) == 1 || (v) == 2))
+#define XFS_DINODE_GOOD_VERSION(v)	((v) >= 1 && (v) <= 3)
 
 typedef struct xfs_timestamp {
 	__be32		t_sec;		/* timestamp seconds */

@@ -70,11 +70,36 @@ typedef struct xfs_dinode {
 

From: Dave Chinner <[email protected]>

Add a header to the remote symlink block, containing location and owner information, as well as CRCs and LSN fields. This requires verifiers to be added to the remote symlink buffers for CRC enabled filesystems.

This also fixes a bug reading multiple block symlinks, where the second block overwrites the first block when copying out the link name.

Hi Dave,

Quoted 7 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Add a header to the remote symlink block, containing location and owner information, as well as CRCs and LSN fields. This requires verifiers to be added to the remote symlink buffers for CRC enabled filesystems.

Quoted 2 lines

This also fixes a bug reading multiple block symlinks, where the second block overwrites the first block when copying out the link name.

Huh. This commit message doesn't say anything about moving symlink related code from xfs_vnodeops.c to a new file xfs_symlink.c.

If you're going to cut
and paste code en masse from one file to the next, that's perfectly fine with me. But please do that in a separate patch than the code you're adding for a feature so that I can focus on the new code in the review.

Looks like you need to add a GPL notice.

Quoted 13 lines · Ben Myers

Hi Dave,

Quoted 9 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Add a header to the remote symlink block, containing location and owner information, as well as CRCs and LSN fields. This requires verifiers to be added to the remote symlink buffers for CRC enabled filesystems.

This also fixes a bug reading multiple block symlinks, where the second block overwrites the first block when copying out the link name.

Quoted 2 lines

Huh. This commit message doesn't say anything about moving symlink related code from xfs_vnodeops.c to a new file xfs_symlink.c.

I'm sure I added it at one point. Maybe I lost it when folding and splitting various patches as I originally wrote this code in the userspace libxfs tree...

I'll split it back out.

Quoted 6 lines

If you're going to cut
and paste code en masse from one file to the next, that's perfectly fine with me. But please do that in a separate patch than the code you're adding for a feature so that I can focus on the new code in the review.

Looks like you need to add a GPL notice.

It isn't actually necessary. All code in the kernel is covered by the GPLv2, and so notices on individual files are not technically necessary. Just because it isn't explicitly stated in the file, it doesn't mean the GPL does not apply to the code - the kernel is licensed according to what is in the COPYING file (i.e. the full GPLv2 license w/ clarifications noted by Linus), and so that is the license that applies to the new file. All that actually matters in individual files is the copyright statements themselves...

But, seeing as you are concerned, I'll add the boiler plates.

From: Dave Chinner <[email protected]>

Add a header to the remote symlink block, containing location and owner information, as well as CRCs and LSN fields. This requires verifiers to be added to the remote symlink buffers for CRC enabled filesystems.

This also fixes a bug reading multiple block symlinks, where the second block overwrites the first block when copying out the link name.

Signed-off-by Dave Chinner <[email protected]>

---
 fs/xfs/Makefile          |    5 +-
 fs/xfs/xfs_bmap.c        |   21 +-
 fs/xfs/xfs_buf_item.h    |    4 +-
 fs/xfs/xfs_log_recover.c |    9 +
 fs/xfs/xfs_symlink.c     |  719 ++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_symlink.h     |   53 ++++
 fs/xfs/xfs_vnodeops.c    |  480 +------------------------------
 7 files changed, 797 insertions(+), 494 deletions(-)
 create mode 100644 fs/xfs/xfs_symlink.c
 create mode 100644 fs/xfs/xfs_symlink.h

fs/xfs/Makefile +3 -2

index d02201d..062f25c 100644
@@ -45,11 +45,11 @@ xfs-y				+= xfs_aops.o \
 				   xfs_itable.o \
 				   xfs_message.o \
 				   xfs_mru_cache.o \
-				   xfs_super.o \
-				   xfs_xattr.o \
 				   xfs_rename.o \
+				   xfs_super.o \
 				   xfs_utils.o \
 				   xfs_vnodeops.o \
+				   xfs_xattr.o \
 				   kmem.o \
 				   uuid.o
 

@@ -73,6 +73,7 @@ xfs-y				+= xfs_alloc.o \
 				   xfs_inode.o \
 				   xfs_log_recover.o \
 				   xfs_mount.o \
+				   xfs_symlink.o \
 				   xfs_trans.o
 
 # low-level transaction/log code

fs/xfs/xfs_bmap.c +1 -0

index 0531cd3..0fcb7f4 100644
@@ -47,6 +47,7 @@
 #include "xfs_filestream.h"
 #include "xfs_vnodeops.h"
 #include "xfs_trace.h"
+#include "xfs_symlink.h"
 
 
 kmem_zone_t		*xfs_bmap_free_item_zone;

fs/xfs/xfs_bmap.c +0 -12

 	bp->b_ops = &xfs_bmbt_buf_ops;
 	memcpy(bp->b_addr, ifp->if_u1.if_data, ifp->if_bytes);
 	xfs_trans_buf_set_type(tp, bp, XFS_BLF_BTREE_BUF);
 }
 
-STATIC void
-xfs_symlink_local_to_remote(
-	struct xfs_trans	*tp,
-	struct xfs_buf		*bp,
-	struct xfs_inode	*ip,
-	struct xfs_ifork	*ifp)
-{
-	/* remote symlink blocks are not verifiable until CRCs come along */
-	bp->b_ops = NULL;
-	memcpy(bp->b_addr, ifp->if_u1.if_data, ifp->if_bytes);
-}
-
 /*
  * Called from xfs_bmap_add_attrfork to handle local format files. Each
  * different data fork content type needs a different callout to do the

fs/xfs/xfs_buf_item.h +3 -1

index abae8c8..09cab4e 100644
@@ -49,6 +49,7 @@ extern kmem_zone_t	*xfs_buf_item_zone;
 #define XFS_BLF_AGFL_BUF	(1<<7)
 #define XFS_BLF_AGI_BUF		(1<<8)
 #define XFS_BLF_DINO_BUF	(1<<9)
+#define XFS_BLF_SYMLINK_BUF	(1<<10)
 
 #define XFS_BLF_TYPE_MASK	\
 		(XFS_BLF_UDQUOT_BUF | \

@@ -58,7 +59,8 @@ extern kmem_zone_t	*xfs_buf_item_zone;
 		 XFS_BLF_AGF_BUF | \
 		 XFS_BLF_AGFL_BUF | \
 		 XFS_BLF_AGI_BUF | \
-		 XFS_BLF_DINO_BUF)
+		 XFS_BLF_DINO_BUF | \
+		 XFS_BLF_SYMLINK_BUF)
 
 #define	XFS_BLF_CHUNK		128
 #define	XFS_BLF_SHIFT		7

fs/xfs/xfs_log_recover.c +9 -0

index 6d08eaa..352d794 100644
@@ -45,6 +45,7 @@
 #include "xfs_cksum.h"
 #include "xfs_trace.h"
 #include "xfs_icache.h"
+#include "xfs_symlink.h"
 
 STATIC int
 xlog_find_zeroed(

@@ -2005,6 +2006,14 @@ xlog_recover_do_reg_buffer(
 		}
 		bp->b_ops = &xfs_inode_buf_ops;
 		break;
+	case XFS_BLF_SYMLINK_BUF:
+		if (*(__be32 *)bp->b_addr != cpu_to_be32(XFS_SYMLINK_MAGIC)) {
+			xfs_warn(mp, "Bad symlink block magic!");
+			ASSERT(0);
+			break;
+		}
+		bp->b_ops = &xfs_symlink_buf_ops;
+		break;
 	default:
 		break;
 	}

fs/xfs/xfs_symlink.c new file +97 -0

new file mode 100644
index 0000000..33e8f13
@@ -0,0 +1,719 @@
+/*
+ * Copyright (c) 2000-2006 Silicon Graphics, Inc.
+ * Copyright (c) 2012-2013 Red Hat, Inc.
+ * All rights reserved.
+ */
+
+#include "xfs.h"
+#include "xfs_fs.h"
+#include "xfs_types.h"
+#include "xfs_bit.h"
+#include "xfs_log.h"
+#include "xfs_trans.h"
+#include "xfs_sb.h"
+#include "xfs_ag.h"
+#include "xfs_dir2.h"
+#include "xfs_mount.h"
+#include "xfs_da_btree.h"
+#include "xfs_bmap_btree.h"
+#include "xfs_ialloc_btree.h"
+#include "xfs_dinode.h"
+#include "xfs_inode.h"
+#include "xfs_inode_item.h"
+#include "xfs_itable.h"
+#include "xfs_ialloc.h"
+#include "xfs_alloc.h"
+#include "xfs_bmap.h"
+#include "xfs_error.h"
+#include "xfs_quota.h"
+#include "xfs_utils.h"
+#include "xfs_trans_space.h"
+#include "xfs_log_priv.h"
+#include "xfs_trace.h"
+#include "xfs_symlink.h"
+#include "xfs_cksum.h"
+#include "xfs_buf_item.h"
+
+
+/*
+ * Each contiguous block has a header, so it is not just a simple pathlen
+ * to FSB conversion.
+ */
+int
+xfs_symlink_blocks(
+	struct xfs_mount *mp,
+	int		pathlen)
+{
+	int		fsblocks = 0;
+	int		len = pathlen;
+
+	do {
+		fsblocks++;
+		len -= XFS_SYMLINK_BUF_SPACE(mp, mp->m_sb.sb_blocksize);
+	} while (len > 0);
+
+	ASSERT(fsblocks <= XFS_SYMLINK_MAPS);
+	return fsblocks;
+}
+
+static int
+xfs_symlink_hdr_set(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	uint32_t		offset,
+	uint32_t		size,
+	struct xfs_buf		*bp)
+{
+	struct xfs_dsymlink_hdr	*dsl = bp->b_addr;
+
+	if (!xfs_sb_version_hascrc(&mp->m_sb))
+		return 0;
+
+	dsl->sl_magic = cpu_to_be32(XFS_SYMLINK_MAGIC);
+	dsl->sl_offset = cpu_to_be32(offset);
+	dsl->sl_bytes = cpu_to_be32(size);
+	uuid_copy(&dsl->sl_uuid, &mp->m_sb.sb_uuid);
+	dsl->sl_owner = cpu_to_be64(ino);
+	dsl->sl_blkno = cpu_to_be64(bp->b_bn);
+	bp->b_ops = &xfs_symlink_buf_ops;
+
+	return sizeof(struct xfs_dsymlink_hdr);
+}
+
+/*
+ * Checking of the symlink header is split into two parts. the verifier does
+ * CRC, location and bounds checking, the unpacking function checks the path
+ * parameters and owner.
+ */
+bool
+xfs_symlink_hdr_ok(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	uint32_t		offset,
+	uint32_t		size,
+	struct xfs_buf		*bp)
+{
+	struct xfs_dsymlink_hdr *dsl = bp->b_addr;
+

Quoted 9 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Add a header to the remote symlink block, containing location and owner information, as well as CRCs and LSN fields. This requires verifiers to be added to the remote symlink buffers for CRC enabled filesystems.

This also fixes a bug reading multiple block symlinks, where the second block overwrites the first block when copying out the link name.

More comments below.

fs/xfs/xfs_bmap.c xfs_bmap_local_to_extents_init_fn( · line 1337

Quoted 9 lines

*/
STATIC void
xfs_bmap_local_to_extents_init_fn(

fs/xfs/xfs_bmap.c +1 -0

@@ -1332,23 +1334,12 @@ xfs_bmap_local_to_extents_init_fn(
 	struct xfs_inode	*ip,
 	struct xfs_ifork	*ifp)
 {
+	ASSERT(0);

That has nothing to do with crcs.