remove the di_version icdicone field v2

remove the di_version icdicone field v2

[Christoph Hellwig]

Hi all,

this series remove the not really needed ic_version field from
the icdinode structure.

Changes since v1:
 - split into multiple patches
 - add a new xfs_sb_version_has_large_dinode helper instead of using
   xfs_sb_version_hascrc
 - add a comment about the relationship of file system and dinode
   versions
 - add a few more trivial cleanups

[PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper

[Christoph Hellwig]

Add a new wrapper to check if a file system supports the newer large
dinode format.  Previously we uses xfs_sb_version_hascrc for that,
which is technically correct but a little confusing to read.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_format.h     |  5 +++++
 fs/xfs/libxfs/xfs_ialloc.c     |  4 ++--
 fs/xfs/libxfs/xfs_inode_buf.c  | 12 ++++++++----
 fs/xfs/libxfs/xfs_trans_resv.c |  2 +-
 fs/xfs/xfs_buf_item.c          |  2 +-
 fs/xfs/xfs_log_recover.c       |  2 +-
 6 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index cd814f99da28..a28bf6a978ad 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
 	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
 }
 
+static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
+}
+
 static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
 {
 	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index b4a404278935..6adffaa68fb8 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
 	 * That means for v3 inode we log the entire buffer rather than just the
 	 * inode cores.
 	 */
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
 		version = 3;
 		ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
 
@@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
 	 * cannot change the behavior.
 	 */
 	igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
 		int	new_size = igeo->inode_cluster_size_raw;
 
 		new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 17e88a8c8353..a5aa2f220c28 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -44,14 +44,18 @@ xfs_inobp_check(
 }
 #endif
 
+/*
+ * v4 and earlier file systems only support the small dinode, and must use the
+ * v1 or v2 inode formats.  v5 file systems support a larger dinode, and must
+ * use the v3 inode format.
+ */
 bool
 xfs_dinode_good_version(
 	struct xfs_mount *mp,
 	__u8		version)
 {
-	if (xfs_sb_version_hascrc(&mp->m_sb))
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb))
 		return version == 3;
-
 	return version == 1 || version == 2;
 }
 
@@ -454,7 +458,7 @@ xfs_dinode_verify(
 
 	/* Verify v3 integrity information first */
 	if (dip->di_version >= 3) {
-		if (!xfs_sb_version_hascrc(&mp->m_sb))
+		if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
 			return __this_address;
 		if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
 				      XFS_DINODE_CRC_OFF))
@@ -629,7 +633,7 @@ xfs_iread(
 
 	/* shortcut IO on inode allocation if possible */
 	if ((iget_flags & XFS_IGET_CREATE) &&
-	    xfs_sb_version_hascrc(&mp->m_sb) &&
+	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
 	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
 		VFS_I(ip)->i_generation = prandom_u32();
 		ip->i_d.di_version = 3;
diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
index 7a9c04920505..294e23d47912 100644
--- a/fs/xfs/libxfs/xfs_trans_resv.c
+++ b/fs/xfs/libxfs/xfs_trans_resv.c
@@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
 			       XFS_FSB_TO_B(mp, 1));
 	if (alloc) {
 		/* icreate tx uses ordered buffers */
-		if (xfs_sb_version_hascrc(&mp->m_sb))
+		if (xfs_sb_version_has_large_dinode(&mp->m_sb))
 			return res;
 		size = XFS_FSB_TO_B(mp, 1);
 	}
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 663810e6cd59..d004ae3455d7 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -345,7 +345,7 @@ xfs_buf_item_format(
 	 * occurs during recovery.
 	 */
 	if (bip->bli_flags & XFS_BLI_INODE_BUF) {
-		if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
+		if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
 		    !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
 		      xfs_log_item_in_current_chkpt(lip)))
 			bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 6abc0863c9c3..e5e976b5cc11 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
 	 * superblock flag to determine whether we need to look at di_flushiter
 	 * to skip replay when the on disk inode is newer than the log one
 	 */
-	if (!xfs_sb_version_hascrc(&mp->m_sb) &&
+	if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
 	    ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
 		/*
 		 * Deal with the wrap case, DI_MAX_FLUSH is less
-- 
2.24.1

[PATCH 2/5] xfs: only check the superblock version for dinode size calculation

[Christoph Hellwig]

The size of the dinode structure is only dependent on the file system
version, so instead of checking the individual inode version just use
the newly added xfs_sb_version_has_large_dinode helper, and simplify
various calling conventions.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_attr_leaf.c  |  5 ++---
 fs/xfs/libxfs/xfs_bmap.c       | 10 ++++------
 fs/xfs/libxfs/xfs_format.h     | 15 +++++++--------
 fs/xfs/libxfs/xfs_ialloc.c     |  2 +-
 fs/xfs/libxfs/xfs_inode_buf.c  |  2 +-
 fs/xfs/libxfs/xfs_inode_fork.c |  2 +-
 fs/xfs/libxfs/xfs_inode_fork.h |  9 ++-------
 fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
 fs/xfs/xfs_inode_item.c        |  4 ++--
 fs/xfs/xfs_log_recover.c       |  2 +-
 fs/xfs/xfs_symlink.c           |  2 +-
 11 files changed, 26 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 4be04aeee278..a4757e5e64ca 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
 	int			offset;
 
 	/* rounded down */
-	offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
+	offset = (XFS_LITINO(mp) - bytes) >> 3;
 
 	if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
 		minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
@@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
 	minforkoff = roundup(minforkoff, 8) >> 3;
 
 	/* attr fork btree root can have at least this many key/ptr pairs */
-	maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
-			XFS_BMDR_SPACE_CALC(MINABTPTRS);
+	maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
 	maxforkoff = maxforkoff >> 3;	/* rounded down */
 
 	if (offset >= maxforkoff)
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 43ae2ab21084..a2fe8a585100 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -193,14 +193,12 @@ xfs_default_attroffset(
 	struct xfs_mount	*mp = ip->i_mount;
 	uint			offset;
 
-	if (mp->m_sb.sb_inodesize == 256) {
-		offset = XFS_LITINO(mp, ip->i_d.di_version) -
-				XFS_BMDR_SPACE_CALC(MINABTPTRS);
-	} else {
+	if (mp->m_sb.sb_inodesize == 256)
+		offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
+	else
 		offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
-	}
 
-	ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
+	ASSERT(offset < XFS_LITINO(mp));
 	return offset;
 }
 
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index a28bf6a978ad..aeca184d63ee 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -942,8 +942,11 @@ enum xfs_dinode_fmt {
 /*
  * Inode size for given fs.
  */
-#define XFS_LITINO(mp, version) \
-	((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
+#define XFS_DINODE_SIZE(mp) \
+       (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
+        sizeof(struct xfs_dinode) : offsetof(struct xfs_dinode, di_crc))
+#define XFS_LITINO(mp) \
+	((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(mp))
 
 /*
  * Inode data & attribute fork sizes, per inode.
@@ -952,13 +955,9 @@ enum xfs_dinode_fmt {
 #define XFS_DFORK_BOFF(dip)		((int)((dip)->di_forkoff << 3))
 
 #define XFS_DFORK_DSIZE(dip,mp) \
-	(XFS_DFORK_Q(dip) ? \
-		XFS_DFORK_BOFF(dip) : \
-		XFS_LITINO(mp, (dip)->di_version))
+	(XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
 #define XFS_DFORK_ASIZE(dip,mp) \
-	(XFS_DFORK_Q(dip) ? \
-		XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
-		0)
+	(XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
 #define XFS_DFORK_SIZE(dip,mp,w) \
 	((w) == XFS_DATA_FORK ? \
 		XFS_DFORK_DSIZE(dip, mp) : \
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 6adffaa68fb8..26de817351fa 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
 		xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
 		for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
 			int	ioffset = i << mp->m_sb.sb_inodelog;
-			uint	isize = xfs_dinode_size(version);
+			uint	isize = XFS_DINODE_SIZE(mp);
 
 			free = xfs_make_iptr(mp, fbuf, i);
 			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index a5aa2f220c28..34ccf162abe1 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -432,7 +432,7 @@ xfs_dinode_verify_forkoff(
 	case XFS_DINODE_FMT_LOCAL:	/* fall through ... */
 	case XFS_DINODE_FMT_EXTENTS:    /* fall through ... */
 	case XFS_DINODE_FMT_BTREE:
-		if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
+		if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
 			return __this_address;
 		break;
 	default:
diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
index ad2b9c313fd2..518c6f0ec3a6 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.c
+++ b/fs/xfs/libxfs/xfs_inode_fork.c
@@ -183,7 +183,7 @@ xfs_iformat_local(
 	 */
 	if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
 		xfs_warn(ip->i_mount,
-	"corrupt inode %Lu (bad size %d for local fork, size = %d).",
+	"corrupt inode %Lu (bad size %d for local fork, size = %zd).",
 			(unsigned long long) ip->i_ino, size,
 			XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED,
diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
index 500333d0101e..668ee942be22 100644
--- a/fs/xfs/libxfs/xfs_inode_fork.h
+++ b/fs/xfs/libxfs/xfs_inode_fork.h
@@ -46,14 +46,9 @@ struct xfs_ifork {
 			(ip)->i_afp : \
 			(ip)->i_cowfp))
 #define XFS_IFORK_DSIZE(ip) \
-	(XFS_IFORK_Q(ip) ? \
-		XFS_IFORK_BOFF(ip) : \
-		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
+	(XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
 #define XFS_IFORK_ASIZE(ip) \
-	(XFS_IFORK_Q(ip) ? \
-		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
-			XFS_IFORK_BOFF(ip) : \
-		0)
+	(XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
 #define XFS_IFORK_SIZE(ip,w) \
 	((w) == XFS_DATA_FORK ? \
 		XFS_IFORK_DSIZE(ip) : \
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 9bac0d2e56dc..76defbea8000 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -424,12 +424,10 @@ struct xfs_log_dinode {
 	/* structure must be padded to 64 bit alignment */
 };
 
-static inline uint xfs_log_dinode_size(int version)
-{
-	if (version == 3)
-		return sizeof(struct xfs_log_dinode);
-	return offsetof(struct xfs_log_dinode, di_next_unlinked);
-}
+#define xfs_log_dinode_size(mp)						\
+	(xfs_sb_version_has_large_dinode(&(mp)->m_sb) ?				\
+		sizeof(struct xfs_log_dinode) :				\
+		offsetof(struct xfs_log_dinode, di_next_unlinked))
 
 /*
  * Buffer Log Format definitions
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index f021b55a0301..451f9b6b2806 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -125,7 +125,7 @@ xfs_inode_item_size(
 
 	*nvecs += 2;
 	*nbytes += sizeof(struct xfs_inode_log_format) +
-		   xfs_log_dinode_size(ip->i_d.di_version);
+		   xfs_log_dinode_size(ip->i_mount);
 
 	xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
 	if (XFS_IFORK_Q(ip))
@@ -370,7 +370,7 @@ xfs_inode_item_format_core(
 
 	dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
 	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));
+	xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
 }
 
 /*
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index e5e976b5cc11..79fc85a4ff08 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
 		error = -EFSCORRUPTED;
 		goto out_release;
 	}
-	isize = xfs_log_dinode_size(ldip->di_version);
+	isize = xfs_log_dinode_size(mp);
 	if (unlikely(item->ri_buf[1].i_len > isize)) {
 		XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
 				     XFS_ERRLEVEL_LOW, mp, ldip,
diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
index ea42e25ec1bf..fa0fa3c70f1a 100644
--- a/fs/xfs/xfs_symlink.c
+++ b/fs/xfs/xfs_symlink.c
@@ -192,7 +192,7 @@ xfs_symlink(
 	 * The symlink will fit into the inode data fork?
 	 * There can't be any attributes so we get the whole variable part.
 	 */
-	if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
+	if (pathlen <= XFS_LITINO(mp))
 		fs_blocks = 0;
 	else
 		fs_blocks = xfs_symlink_blocks(mp, pathlen);
-- 
2.24.1

[PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize

[Christoph Hellwig]

Only v5 file systems can have the reflink feature, and those will
always use the large dinode format.  Remove the extra check for the
inode version.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_ioctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 5a1d2b9cb05a..ad825ffa7e4c 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
 	if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
 		return 0;
 
-	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
-	    ip->i_d.di_version != 3)
+	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
 		return -EINVAL;
 
 	if (fa->fsx_cowextsize == 0)
-- 
2.24.1

[PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc

[Christoph Hellwig]

di_flags2 is initialized to zero for v4 and earlier file systems.  This
means di_flags2 can only be non-zero for a v5 file systems, in which
case both the parent and child inodes can store the filed.  Remove the
extra di_version check, and also remove the rather pointless local
di_flags2 variable while at it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index addc3ee0cb73..ebfd8efb0efa 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -907,20 +907,13 @@ xfs_ialloc(
 
 			ip->i_d.di_flags |= di_flags;
 		}
-		if (pip &&
-		    (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
-		    pip->i_d.di_version == 3 &&
-		    ip->i_d.di_version == 3) {
-			uint64_t	di_flags2 = 0;
-
+		if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
 			if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
-				di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
+				ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
 				ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
 			}
 			if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
-				di_flags2 |= XFS_DIFLAG2_DAX;
-
-			ip->i_d.di_flags2 |= di_flags2;
+				ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
 		}
 		/* FALLTHROUGH */
 	case S_IFLNK:
-- 
2.24.1

[PATCH 5/5] xfs: remove the di_version field from struct icdinode

[Christoph Hellwig]

We know the version is 3 if on a v5 file system.   For earlier file
systems formats we always upgrade the remaining v1 inodes to v2 and
thus only use v2 inodes.  Use the xfs_sb_version_has_large_dinode
helper to check if we deal with small or large dinodes, and thus
remove the need for the di_version field in struct icdinode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
 fs/xfs/libxfs/xfs_inode_buf.h |  1 -
 fs/xfs/xfs_bmap_util.c        | 16 ++++++++--------
 fs/xfs/xfs_inode.c            | 16 ++--------------
 fs/xfs/xfs_inode_item.c       |  8 +++-----
 fs/xfs/xfs_ioctl.c            |  5 ++---
 fs/xfs/xfs_iops.c             |  2 +-
 fs/xfs/xfs_itable.c           |  2 +-
 fs/xfs/xfs_log_recover.c      |  2 +-
 9 files changed, 24 insertions(+), 44 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 34ccf162abe1..7384b9194922 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -209,16 +209,14 @@ xfs_inode_from_disk(
 	struct xfs_icdinode	*to = &ip->i_d;
 	struct inode		*inode = VFS_I(ip);
 
-
 	/*
 	 * Convert v1 inodes immediately to v2 inode format as this is the
 	 * minimum inode version format we support in the rest of the code.
+	 * They will also be unconditionally written back to disk as v2 inodes.
 	 */
-	to->di_version = from->di_version;
-	if (to->di_version == 1) {
+	if (unlikely(from->di_version == 1)) {
 		set_nlink(inode, be16_to_cpu(from->di_onlink));
 		to->di_projid = 0;
-		to->di_version = 2;
 	} else {
 		set_nlink(inode, be32_to_cpu(from->di_nlink));
 		to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
@@ -256,7 +254,7 @@ xfs_inode_from_disk(
 	to->di_dmstate	= be16_to_cpu(from->di_dmstate);
 	to->di_flags	= be16_to_cpu(from->di_flags);
 
-	if (to->di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
 		inode_set_iversion_queried(inode,
 					   be64_to_cpu(from->di_changecount));
 		to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
@@ -278,7 +276,6 @@ xfs_inode_to_disk(
 	to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
 	to->di_onlink = 0;
 
-	to->di_version = from->di_version;
 	to->di_format = from->di_format;
 	to->di_uid = cpu_to_be32(i_uid_read(inode));
 	to->di_gid = cpu_to_be32(i_gid_read(inode));
@@ -307,7 +304,8 @@ xfs_inode_to_disk(
 	to->di_dmstate = cpu_to_be16(from->di_dmstate);
 	to->di_flags = cpu_to_be16(from->di_flags);
 
-	if (from->di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+		to->di_version = 3;
 		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
 		to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
 		to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
@@ -319,6 +317,7 @@ xfs_inode_to_disk(
 		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
 		to->di_flushiter = 0;
 	} else {
+		to->di_version = 2;
 		to->di_flushiter = cpu_to_be16(from->di_flushiter);
 	}
 }
@@ -636,7 +635,6 @@ xfs_iread(
 	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
 	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
 		VFS_I(ip)->i_generation = prandom_u32();
-		ip->i_d.di_version = 3;
 		return 0;
 	}
 
@@ -678,7 +676,6 @@ xfs_iread(
 		 * 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_version = dip->di_version;
 		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
 		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
 
@@ -692,7 +689,6 @@ xfs_iread(
 		VFS_I(ip)->i_mode = 0;
 	}
 
-	ASSERT(ip->i_d.di_version >= 2);
 	ip->i_delayed_blks = 0;
 
 	/*
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index 2683e1e2c4a6..4e24fad3deb0 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -16,7 +16,6 @@ struct xfs_dinode;
  * format specific structures at the appropriate time.
  */
 struct xfs_icdinode {
-	int8_t		di_version;	/* inode version */
 	int8_t		di_format;	/* format of di_c data */
 	uint16_t	di_flushiter;	/* incremented on flush */
 	uint32_t	di_projid;	/* owner's project id */
diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 3df4d0af9f22..6ca37944d71f 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
 	 * event of a crash. Set the owner change log flags now and leave the
 	 * bmbt scan as the last step.
 	 */
-	if (ip->i_d.di_version == 3 &&
-	    ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
-		(*target_log_flags) |= XFS_ILOG_DOWNER;
-	if (tip->i_d.di_version == 3 &&
-	    tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
-		(*src_log_flags) |= XFS_ILOG_DOWNER;
+	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+		if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
+			(*target_log_flags) |= XFS_ILOG_DOWNER;
+		if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
+			(*src_log_flags) |= XFS_ILOG_DOWNER;
+	}
 
 	/*
 	 * Swap the data forks of the inodes
@@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
 		(*src_log_flags) |= XFS_ILOG_DEXT;
 		break;
 	case XFS_DINODE_FMT_BTREE:
-		ASSERT(ip->i_d.di_version < 3 ||
+		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
 		       (*src_log_flags & XFS_ILOG_DOWNER));
 		(*src_log_flags) |= XFS_ILOG_DBROOT;
 		break;
@@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
 		break;
 	case XFS_DINODE_FMT_BTREE:
 		(*target_log_flags) |= XFS_ILOG_DBROOT;
-		ASSERT(tip->i_d.di_version < 3 ||
+		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
 		       (*target_log_flags & XFS_ILOG_DOWNER));
 		break;
 	}
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index ebfd8efb0efa..600c0b59bdd7 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -801,15 +801,6 @@ xfs_ialloc(
 		return error;
 	ASSERT(ip != NULL);
 	inode = VFS_I(ip);
-
-	/*
-	 * We always convert v1 inodes to v2 now - we only support filesystems
-	 * with >= v2 inode capability, so there is no reason for ever leaving
-	 * an inode in v1 format.
-	 */
-	if (ip->i_d.di_version == 1)
-		ip->i_d.di_version = 2;
-
 	inode->i_mode = mode;
 	set_nlink(inode, nlink);
 	inode->i_uid = current_fsuid();
@@ -847,14 +838,13 @@ xfs_ialloc(
 	ip->i_d.di_dmstate = 0;
 	ip->i_d.di_flags = 0;
 
-	if (ip->i_d.di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
 		inode_set_iversion(inode, 1);
 		ip->i_d.di_flags2 = 0;
 		ip->i_d.di_cowextsize = 0;
 		ip->i_d.di_crtime = tv;
 	}
 
-
 	flags = XFS_ILOG_CORE;
 	switch (mode & S_IFMT) {
 	case S_IFIFO:
@@ -1115,7 +1105,6 @@ xfs_bumplink(
 {
 	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
 
-	ASSERT(ip->i_d.di_version > 1);
 	inc_nlink(VFS_I(ip));
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 }
@@ -3798,7 +3787,6 @@ xfs_iflush_int(
 	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
 	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
 	ASSERT(iip != NULL && iip->ili_fields != 0);
-	ASSERT(ip->i_d.di_version > 1);
 
 	/* set *dip = inode's place in the buffer */
 	dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
@@ -3859,7 +3847,7 @@ xfs_iflush_int(
 	 * backwards compatibility with old kernels that predate logging all
 	 * inode changes.
 	 */
-	if (ip->i_d.di_version < 3)
+	if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
 		ip->i_d.di_flushiter++;
 
 	/* Check the inline fork data before we write out. */
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index 451f9b6b2806..5800bfda96b2 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
 	struct inode		*inode = VFS_I(ip);
 
 	to->di_magic = XFS_DINODE_MAGIC;
-
-	to->di_version = from->di_version;
 	to->di_format = from->di_format;
 	to->di_uid = i_uid_read(inode);
 	to->di_gid = i_gid_read(inode);
@@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
 	/* log a dummy value to ensure log structure is fully initialised */
 	to->di_next_unlinked = NULLAGINO;
 
-	if (from->di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
+		to->di_version = 3;
 		to->di_changecount = inode_peek_iversion(inode);
 		to->di_crtime.t_sec = from->di_crtime.tv_sec;
 		to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
@@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
 		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
 		to->di_flushiter = 0;
 	} else {
+		to->di_version = 2;
 		to->di_flushiter = from->di_flushiter;
 	}
 }
@@ -395,8 +395,6 @@ xfs_inode_item_format(
 	struct xfs_log_iovec	*vecp = NULL;
 	struct xfs_inode_log_format *ilf;
 
-	ASSERT(ip->i_d.di_version > 1);
-
 	ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
 	ilf->ilf_type = XFS_LI_INODE;
 	ilf->ilf_ino = ip->i_ino;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index ad825ffa7e4c..a98909cc7ecb 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
 
 	/* diflags2 only valid for v3 inodes. */
 	di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
-	if (di_flags2 && ip->i_d.di_version < 3)
+	if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
 		return -EINVAL;
 
 	ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
@@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
 			olddquot = xfs_qm_vop_chown(tp, ip,
 						&ip->i_pdquot, pdqp);
 		}
-		ASSERT(ip->i_d.di_version > 1);
 		ip->i_d.di_projid = fa->fsx_projid;
 	}
 
@@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
 		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
 	else
 		ip->i_d.di_extsize = 0;
-	if (ip->i_d.di_version == 3 &&
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
 	    (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
 		ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
 				mp->m_sb.sb_blocklog;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 87093a05aad7..df2cd57e984e 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -557,7 +557,7 @@ xfs_vn_getattr(
 	stat->blocks =
 		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
 
-	if (ip->i_d.di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
 		if (request_mask & STATX_BTIME) {
 			stat->result_mask |= STATX_BTIME;
 			stat->btime = ip->i_d.di_crtime;
diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index d10660469884..4d4dad557c2f 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
 	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
 	buf->bs_version = XFS_BULKSTAT_VERSION_V5;
 
-	if (dic->di_version == 3) {
+	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
 		if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
 			buf->bs_cowextsize_blks = dic->di_cowextsize;
 	}
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 79fc85a4ff08..9db6003d125b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
 		return -ENOMEM;
 
 	/* instantiate the inode */
+	ASSERT(dip->di_version >= 3);
 	xfs_inode_from_disk(ip, dip);
-	ASSERT(ip->i_d.di_version >= 3);
 
 	error = xfs_iformat_fork(ip, dip);
 	if (error)
-- 
2.24.1

Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper

[Brian Foster]

On Thu, Mar 12, 2020 at 03:22:31PM +0100, Christoph Hellwig wrote:
> Add a new wrapper to check if a file system supports the newer large
> dinode format.  Previously we uses xfs_sb_version_hascrc for that,
> which is technically correct but a little confusing to read.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_format.h     |  5 +++++
>  fs/xfs/libxfs/xfs_ialloc.c     |  4 ++--
>  fs/xfs/libxfs/xfs_inode_buf.c  | 12 ++++++++----
>  fs/xfs/libxfs/xfs_trans_resv.c |  2 +-
>  fs/xfs/xfs_buf_item.c          |  2 +-
>  fs/xfs/xfs_log_recover.c       |  2 +-
>  6 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index cd814f99da28..a28bf6a978ad 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
>  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
>  }
>  

A comment would be useful here to indicate what this means (i.e., we can
assume v3 inode format). The function name is a little vague too I
suppose (will the inode ever get larger than large? :P). I wonder if
something like _has_v3_inode() is more clear, but we can always change
it easily enough and either way is better than hascrc() IMO.

Brian

> +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
> +{
> +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> +}
> +
>  static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
>  {
>  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index b4a404278935..6adffaa68fb8 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
>  	 * That means for v3 inode we log the entire buffer rather than just the
>  	 * inode cores.
>  	 */
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		version = 3;
>  		ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
>  
> @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
>  	 * cannot change the behavior.
>  	 */
>  	igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		int	new_size = igeo->inode_cluster_size_raw;
>  
>  		new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 17e88a8c8353..a5aa2f220c28 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -44,14 +44,18 @@ xfs_inobp_check(
>  }
>  #endif
>  
> +/*
> + * v4 and earlier file systems only support the small dinode, and must use the
> + * v1 or v2 inode formats.  v5 file systems support a larger dinode, and must
> + * use the v3 inode format.
> + */
>  bool
>  xfs_dinode_good_version(
>  	struct xfs_mount *mp,
>  	__u8		version)
>  {
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		return version == 3;
> -
>  	return version == 1 || version == 2;
>  }
>  
> @@ -454,7 +458,7 @@ xfs_dinode_verify(
>  
>  	/* Verify v3 integrity information first */
>  	if (dip->di_version >= 3) {
> -		if (!xfs_sb_version_hascrc(&mp->m_sb))
> +		if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
>  			return __this_address;
>  		if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
>  				      XFS_DINODE_CRC_OFF))
> @@ -629,7 +633,7 @@ xfs_iread(
>  
>  	/* shortcut IO on inode allocation if possible */
>  	if ((iget_flags & XFS_IGET_CREATE) &&
> -	    xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
>  		VFS_I(ip)->i_generation = prandom_u32();
>  		ip->i_d.di_version = 3;
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 7a9c04920505..294e23d47912 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
>  			       XFS_FSB_TO_B(mp, 1));
>  	if (alloc) {
>  		/* icreate tx uses ordered buffers */
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> +		if (xfs_sb_version_has_large_dinode(&mp->m_sb))
>  			return res;
>  		size = XFS_FSB_TO_B(mp, 1);
>  	}
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 663810e6cd59..d004ae3455d7 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -345,7 +345,7 @@ xfs_buf_item_format(
>  	 * occurs during recovery.
>  	 */
>  	if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> -		if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> +		if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
>  		    !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
>  		      xfs_log_item_in_current_chkpt(lip)))
>  			bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 6abc0863c9c3..e5e976b5cc11 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
>  	 * superblock flag to determine whether we need to look at di_flushiter
>  	 * to skip replay when the on disk inode is newer than the log one
>  	 */
> -	if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> +	if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
>  		/*
>  		 * Deal with the wrap case, DI_MAX_FLUSH is less
> -- 
> 2.24.1
> 

Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation

[Brian Foster]

On Thu, Mar 12, 2020 at 03:22:32PM +0100, Christoph Hellwig wrote:
> The size of the dinode structure is only dependent on the file system
> version, so instead of checking the individual inode version just use
> the newly added xfs_sb_version_has_large_dinode helper, and simplify
> various calling conventions.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c  |  5 ++---
>  fs/xfs/libxfs/xfs_bmap.c       | 10 ++++------
>  fs/xfs/libxfs/xfs_format.h     | 15 +++++++--------
>  fs/xfs/libxfs/xfs_ialloc.c     |  2 +-
>  fs/xfs/libxfs/xfs_inode_buf.c  |  2 +-
>  fs/xfs/libxfs/xfs_inode_fork.c |  2 +-
>  fs/xfs/libxfs/xfs_inode_fork.h |  9 ++-------
>  fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
>  fs/xfs/xfs_inode_item.c        |  4 ++--
>  fs/xfs/xfs_log_recover.c       |  2 +-
>  fs/xfs/xfs_symlink.c           |  2 +-
>  11 files changed, 26 insertions(+), 37 deletions(-)
> 
...
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index ad2b9c313fd2..518c6f0ec3a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,7 +183,7 @@ xfs_iformat_local(
>  	 */
>  	if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
>  		xfs_warn(ip->i_mount,
> -	"corrupt inode %Lu (bad size %d for local fork, size = %d).",
> +	"corrupt inode %Lu (bad size %d for local fork, size = %zd).",

Is this here intentionally? Otherwise seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  			(unsigned long long) ip->i_ino, size,
>  			XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 500333d0101e..668ee942be22 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -46,14 +46,9 @@ struct xfs_ifork {
>  			(ip)->i_afp : \
>  			(ip)->i_cowfp))
>  #define XFS_IFORK_DSIZE(ip) \
> -	(XFS_IFORK_Q(ip) ? \
> -		XFS_IFORK_BOFF(ip) : \
> -		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
> +	(XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
>  #define XFS_IFORK_ASIZE(ip) \
> -	(XFS_IFORK_Q(ip) ? \
> -		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
> -			XFS_IFORK_BOFF(ip) : \
> -		0)
> +	(XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
>  #define XFS_IFORK_SIZE(ip,w) \
>  	((w) == XFS_DATA_FORK ? \
>  		XFS_IFORK_DSIZE(ip) : \
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 9bac0d2e56dc..76defbea8000 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -424,12 +424,10 @@ struct xfs_log_dinode {
>  	/* structure must be padded to 64 bit alignment */
>  };
>  
> -static inline uint xfs_log_dinode_size(int version)
> -{
> -	if (version == 3)
> -		return sizeof(struct xfs_log_dinode);
> -	return offsetof(struct xfs_log_dinode, di_next_unlinked);
> -}
> +#define xfs_log_dinode_size(mp)						\
> +	(xfs_sb_version_has_large_dinode(&(mp)->m_sb) ?				\
> +		sizeof(struct xfs_log_dinode) :				\
> +		offsetof(struct xfs_log_dinode, di_next_unlinked))
>  
>  /*
>   * Buffer Log Format definitions
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f021b55a0301..451f9b6b2806 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -125,7 +125,7 @@ xfs_inode_item_size(
>  
>  	*nvecs += 2;
>  	*nbytes += sizeof(struct xfs_inode_log_format) +
> -		   xfs_log_dinode_size(ip->i_d.di_version);
> +		   xfs_log_dinode_size(ip->i_mount);
>  
>  	xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
>  	if (XFS_IFORK_Q(ip))
> @@ -370,7 +370,7 @@ xfs_inode_item_format_core(
>  
>  	dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
>  	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));
> +	xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e5e976b5cc11..79fc85a4ff08 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
>  		error = -EFSCORRUPTED;
>  		goto out_release;
>  	}
> -	isize = xfs_log_dinode_size(ldip->di_version);
> +	isize = xfs_log_dinode_size(mp);
>  	if (unlikely(item->ri_buf[1].i_len > isize)) {
>  		XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
>  				     XFS_ERRLEVEL_LOW, mp, ldip,
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index ea42e25ec1bf..fa0fa3c70f1a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -192,7 +192,7 @@ xfs_symlink(
>  	 * The symlink will fit into the inode data fork?
>  	 * There can't be any attributes so we get the whole variable part.
>  	 */
> -	if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
> +	if (pathlen <= XFS_LITINO(mp))
>  		fs_blocks = 0;
>  	else
>  		fs_blocks = xfs_symlink_blocks(mp, pathlen);
> -- 
> 2.24.1
> 

Re: [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize

[Brian Foster]

On Thu, Mar 12, 2020 at 03:22:33PM +0100, Christoph Hellwig wrote:
> Only v5 file systems can have the reflink feature, and those will
> always use the large dinode format.  Remove the extra check for the
> inode version.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_ioctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a1d2b9cb05a..ad825ffa7e4c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
>  	if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
>  		return 0;
>  
> -	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
> -	    ip->i_d.di_version != 3)
> +	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
>  		return -EINVAL;
>  
>  	if (fa->fsx_cowextsize == 0)
> -- 
> 2.24.1
> 

Re: [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc

[Brian Foster]

On Thu, Mar 12, 2020 at 03:22:34PM +0100, Christoph Hellwig wrote:
> di_flags2 is initialized to zero for v4 and earlier file systems.  This
> means di_flags2 can only be non-zero for a v5 file systems, in which
> case both the parent and child inodes can store the filed.  Remove the

filed?

> extra di_version check, and also remove the rather pointless local
> di_flags2 variable while at it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Otherwise seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_inode.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index addc3ee0cb73..ebfd8efb0efa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -907,20 +907,13 @@ xfs_ialloc(
>  
>  			ip->i_d.di_flags |= di_flags;
>  		}
> -		if (pip &&
> -		    (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
> -		    pip->i_d.di_version == 3 &&
> -		    ip->i_d.di_version == 3) {
> -			uint64_t	di_flags2 = 0;
> -
> +		if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
>  			if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
> -				di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> +				ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  				ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
>  			}
>  			if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
> -				di_flags2 |= XFS_DIFLAG2_DAX;
> -
> -			ip->i_d.di_flags2 |= di_flags2;
> +				ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
>  		}
>  		/* FALLTHROUGH */
>  	case S_IFLNK:
> -- 
> 2.24.1
> 

Re: [PATCH 5/5] xfs: remove the di_version field from struct icdinode

[Brian Foster]

On Thu, Mar 12, 2020 at 03:22:35PM +0100, Christoph Hellwig wrote:
> We know the version is 3 if on a v5 file system.   For earlier file
> systems formats we always upgrade the remaining v1 inodes to v2 and
> thus only use v2 inodes.  Use the xfs_sb_version_has_large_dinode
> helper to check if we deal with small or large dinodes, and thus
> remove the need for the di_version field in struct icdinode.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
>  fs/xfs/libxfs/xfs_inode_buf.h |  1 -
>  fs/xfs/xfs_bmap_util.c        | 16 ++++++++--------
>  fs/xfs/xfs_inode.c            | 16 ++--------------
>  fs/xfs/xfs_inode_item.c       |  8 +++-----
>  fs/xfs/xfs_ioctl.c            |  5 ++---
>  fs/xfs/xfs_iops.c             |  2 +-
>  fs/xfs/xfs_itable.c           |  2 +-
>  fs/xfs/xfs_log_recover.c      |  2 +-
>  9 files changed, 24 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 34ccf162abe1..7384b9194922 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -209,16 +209,14 @@ xfs_inode_from_disk(
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
>  
> -
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
>  	 * minimum inode version format we support in the rest of the code.
> +	 * They will also be unconditionally written back to disk as v2 inodes.
>  	 */
> -	to->di_version = from->di_version;
> -	if (to->di_version == 1) {
> +	if (unlikely(from->di_version == 1)) {
>  		set_nlink(inode, be16_to_cpu(from->di_onlink));
>  		to->di_projid = 0;
> -		to->di_version = 2;
>  	} else {
>  		set_nlink(inode, be32_to_cpu(from->di_nlink));
>  		to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
> @@ -256,7 +254,7 @@ xfs_inode_from_disk(
>  	to->di_dmstate	= be16_to_cpu(from->di_dmstate);
>  	to->di_flags	= be16_to_cpu(from->di_flags);
>  
> -	if (to->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
>  		inode_set_iversion_queried(inode,
>  					   be64_to_cpu(from->di_changecount));
>  		to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
> @@ -278,7 +276,6 @@ xfs_inode_to_disk(
>  	to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
>  	to->di_onlink = 0;
>  
> -	to->di_version = from->di_version;
>  	to->di_format = from->di_format;
>  	to->di_uid = cpu_to_be32(i_uid_read(inode));
>  	to->di_gid = cpu_to_be32(i_gid_read(inode));
> @@ -307,7 +304,8 @@ xfs_inode_to_disk(
>  	to->di_dmstate = cpu_to_be16(from->di_dmstate);
>  	to->di_flags = cpu_to_be16(from->di_flags);
>  
> -	if (from->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		to->di_version = 3;
>  		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
>  		to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
>  		to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
> @@ -319,6 +317,7 @@ xfs_inode_to_disk(
>  		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
>  		to->di_flushiter = 0;
>  	} else {
> +		to->di_version = 2;
>  		to->di_flushiter = cpu_to_be16(from->di_flushiter);
>  	}
>  }
> @@ -636,7 +635,6 @@ xfs_iread(
>  	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
>  		VFS_I(ip)->i_generation = prandom_u32();
> -		ip->i_d.di_version = 3;
>  		return 0;
>  	}
>  
> @@ -678,7 +676,6 @@ xfs_iread(
>  		 * 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_version = dip->di_version;
>  		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
>  		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
>  
> @@ -692,7 +689,6 @@ xfs_iread(
>  		VFS_I(ip)->i_mode = 0;
>  	}
>  
> -	ASSERT(ip->i_d.di_version >= 2);
>  	ip->i_delayed_blks = 0;
>  
>  	/*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 2683e1e2c4a6..4e24fad3deb0 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -16,7 +16,6 @@ struct xfs_dinode;
>   * format specific structures at the appropriate time.
>   */
>  struct xfs_icdinode {
> -	int8_t		di_version;	/* inode version */
>  	int8_t		di_format;	/* format of di_c data */
>  	uint16_t	di_flushiter;	/* incremented on flush */
>  	uint32_t	di_projid;	/* owner's project id */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 3df4d0af9f22..6ca37944d71f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
>  	 * event of a crash. Set the owner change log flags now and leave the
>  	 * bmbt scan as the last step.
>  	 */
> -	if (ip->i_d.di_version == 3 &&
> -	    ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> -		(*target_log_flags) |= XFS_ILOG_DOWNER;
> -	if (tip->i_d.di_version == 3 &&
> -	    tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> -		(*src_log_flags) |= XFS_ILOG_DOWNER;
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> +			(*target_log_flags) |= XFS_ILOG_DOWNER;
> +		if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> +			(*src_log_flags) |= XFS_ILOG_DOWNER;
> +	}
>  
>  	/*
>  	 * Swap the data forks of the inodes
> @@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
>  		(*src_log_flags) |= XFS_ILOG_DEXT;
>  		break;
>  	case XFS_DINODE_FMT_BTREE:
> -		ASSERT(ip->i_d.di_version < 3 ||
> +		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
>  		       (*src_log_flags & XFS_ILOG_DOWNER));
>  		(*src_log_flags) |= XFS_ILOG_DBROOT;
>  		break;
> @@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
>  		break;
>  	case XFS_DINODE_FMT_BTREE:
>  		(*target_log_flags) |= XFS_ILOG_DBROOT;
> -		ASSERT(tip->i_d.di_version < 3 ||
> +		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
>  		       (*target_log_flags & XFS_ILOG_DOWNER));
>  		break;
>  	}
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ebfd8efb0efa..600c0b59bdd7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -801,15 +801,6 @@ xfs_ialloc(
>  		return error;
>  	ASSERT(ip != NULL);
>  	inode = VFS_I(ip);
> -
> -	/*
> -	 * We always convert v1 inodes to v2 now - we only support filesystems
> -	 * with >= v2 inode capability, so there is no reason for ever leaving
> -	 * an inode in v1 format.
> -	 */
> -	if (ip->i_d.di_version == 1)
> -		ip->i_d.di_version = 2;
> -
>  	inode->i_mode = mode;
>  	set_nlink(inode, nlink);
>  	inode->i_uid = current_fsuid();
> @@ -847,14 +838,13 @@ xfs_ialloc(
>  	ip->i_d.di_dmstate = 0;
>  	ip->i_d.di_flags = 0;
>  
> -	if (ip->i_d.di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		inode_set_iversion(inode, 1);
>  		ip->i_d.di_flags2 = 0;
>  		ip->i_d.di_cowextsize = 0;
>  		ip->i_d.di_crtime = tv;
>  	}
>  
> -
>  	flags = XFS_ILOG_CORE;
>  	switch (mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -1115,7 +1105,6 @@ xfs_bumplink(
>  {
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  
> -	ASSERT(ip->i_d.di_version > 1);
>  	inc_nlink(VFS_I(ip));
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  }
> @@ -3798,7 +3787,6 @@ xfs_iflush_int(
>  	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
>  	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
>  	ASSERT(iip != NULL && iip->ili_fields != 0);
> -	ASSERT(ip->i_d.di_version > 1);
>  
>  	/* set *dip = inode's place in the buffer */
>  	dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
> @@ -3859,7 +3847,7 @@ xfs_iflush_int(
>  	 * backwards compatibility with old kernels that predate logging all
>  	 * inode changes.
>  	 */
> -	if (ip->i_d.di_version < 3)
> +	if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		ip->i_d.di_flushiter++;
>  
>  	/* Check the inline fork data before we write out. */
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 451f9b6b2806..5800bfda96b2 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
>  	struct inode		*inode = VFS_I(ip);
>  
>  	to->di_magic = XFS_DINODE_MAGIC;
> -
> -	to->di_version = from->di_version;
>  	to->di_format = from->di_format;
>  	to->di_uid = i_uid_read(inode);
>  	to->di_gid = i_gid_read(inode);
> @@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
>  	/* log a dummy value to ensure log structure is fully initialised */
>  	to->di_next_unlinked = NULLAGINO;
>  
> -	if (from->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		to->di_version = 3;
>  		to->di_changecount = inode_peek_iversion(inode);
>  		to->di_crtime.t_sec = from->di_crtime.tv_sec;
>  		to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
> @@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
>  		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
>  		to->di_flushiter = 0;
>  	} else {
> +		to->di_version = 2;
>  		to->di_flushiter = from->di_flushiter;
>  	}
>  }
> @@ -395,8 +395,6 @@ xfs_inode_item_format(
>  	struct xfs_log_iovec	*vecp = NULL;
>  	struct xfs_inode_log_format *ilf;
>  
> -	ASSERT(ip->i_d.di_version > 1);
> -
>  	ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
>  	ilf->ilf_type = XFS_LI_INODE;
>  	ilf->ilf_ino = ip->i_ino;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ad825ffa7e4c..a98909cc7ecb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
>  
>  	/* diflags2 only valid for v3 inodes. */
>  	di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> -	if (di_flags2 && ip->i_d.di_version < 3)
> +	if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		return -EINVAL;
>  
>  	ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> @@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
>  			olddquot = xfs_qm_vop_chown(tp, ip,
>  						&ip->i_pdquot, pdqp);
>  		}
> -		ASSERT(ip->i_d.di_version > 1);
>  		ip->i_d.di_projid = fa->fsx_projid;
>  	}
>  
> @@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
>  		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
>  	else
>  		ip->i_d.di_extsize = 0;
> -	if (ip->i_d.di_version == 3 &&
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
>  		ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
>  				mp->m_sb.sb_blocklog;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 87093a05aad7..df2cd57e984e 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -557,7 +557,7 @@ xfs_vn_getattr(
>  	stat->blocks =
>  		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>  
> -	if (ip->i_d.di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		if (request_mask & STATX_BTIME) {
>  			stat->result_mask |= STATX_BTIME;
>  			stat->btime = ip->i_d.di_crtime;
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index d10660469884..4d4dad557c2f 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
>  	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
>  	buf->bs_version = XFS_BULKSTAT_VERSION_V5;
>  
> -	if (dic->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
>  			buf->bs_cowextsize_blks = dic->di_cowextsize;
>  	}
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 79fc85a4ff08..9db6003d125b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
>  		return -ENOMEM;
>  
>  	/* instantiate the inode */
> +	ASSERT(dip->di_version >= 3);
>  	xfs_inode_from_disk(ip, dip);
> -	ASSERT(ip->i_d.di_version >= 3);
>  
>  	error = xfs_iformat_fork(ip, dip);
>  	if (error)
> -- 
> 2.24.1
> 

Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper

[Chandan Rajendra]

On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote: 
> Add a new wrapper to check if a file system supports the newer large
> dinode format.  Previously we uses xfs_sb_version_hascrc for that,
> which is technically correct but a little confusing to read.
>

I don't see any logical issues.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_format.h     |  5 +++++
>  fs/xfs/libxfs/xfs_ialloc.c     |  4 ++--
>  fs/xfs/libxfs/xfs_inode_buf.c  | 12 ++++++++----
>  fs/xfs/libxfs/xfs_trans_resv.c |  2 +-
>  fs/xfs/xfs_buf_item.c          |  2 +-
>  fs/xfs/xfs_log_recover.c       |  2 +-
>  6 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index cd814f99da28..a28bf6a978ad 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
>  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
>  }
>  
> +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)
> +{
> +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> +}
> +
>  static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
>  {
>  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index b4a404278935..6adffaa68fb8 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
>  	 * That means for v3 inode we log the entire buffer rather than just the
>  	 * inode cores.
>  	 */
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		version = 3;
>  		ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
>  
> @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
>  	 * cannot change the behavior.
>  	 */
>  	igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		int	new_size = igeo->inode_cluster_size_raw;
>  
>  		new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 17e88a8c8353..a5aa2f220c28 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -44,14 +44,18 @@ xfs_inobp_check(
>  }
>  #endif
>  
> +/*
> + * v4 and earlier file systems only support the small dinode, and must use the
> + * v1 or v2 inode formats.  v5 file systems support a larger dinode, and must
> + * use the v3 inode format.
> + */
>  bool
>  xfs_dinode_good_version(
>  	struct xfs_mount *mp,
>  	__u8		version)
>  {
> -	if (xfs_sb_version_hascrc(&mp->m_sb))
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		return version == 3;
> -
>  	return version == 1 || version == 2;
>  }
>  
> @@ -454,7 +458,7 @@ xfs_dinode_verify(
>  
>  	/* Verify v3 integrity information first */
>  	if (dip->di_version >= 3) {
> -		if (!xfs_sb_version_hascrc(&mp->m_sb))
> +		if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
>  			return __this_address;
>  		if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
>  				      XFS_DINODE_CRC_OFF))
> @@ -629,7 +633,7 @@ xfs_iread(
>  
>  	/* shortcut IO on inode allocation if possible */
>  	if ((iget_flags & XFS_IGET_CREATE) &&
> -	    xfs_sb_version_hascrc(&mp->m_sb) &&
> +	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
>  		VFS_I(ip)->i_generation = prandom_u32();
>  		ip->i_d.di_version = 3;
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 7a9c04920505..294e23d47912 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
>  			       XFS_FSB_TO_B(mp, 1));
>  	if (alloc) {
>  		/* icreate tx uses ordered buffers */
> -		if (xfs_sb_version_hascrc(&mp->m_sb))
> +		if (xfs_sb_version_has_large_dinode(&mp->m_sb))
>  			return res;
>  		size = XFS_FSB_TO_B(mp, 1);
>  	}
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 663810e6cd59..d004ae3455d7 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -345,7 +345,7 @@ xfs_buf_item_format(
>  	 * occurs during recovery.
>  	 */
>  	if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> -		if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> +		if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
>  		    !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
>  		      xfs_log_item_in_current_chkpt(lip)))
>  			bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 6abc0863c9c3..e5e976b5cc11 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
>  	 * superblock flag to determine whether we need to look at di_flushiter
>  	 * to skip replay when the on disk inode is newer than the log one
>  	 */
> -	if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> +	if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
>  		/*
>  		 * Deal with the wrap case, DI_MAX_FLUSH is less
> 


-- 
chandan

Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper

[Christoph Hellwig]

On Mon, Mar 16, 2020 at 09:16:49AM -0400, Brian Foster wrote:
> A comment would be useful here to indicate what this means (i.e., we can
> assume v3 inode format).

Sure.

> The function name is a little vague too I
> suppose (will the inode ever get larger than large? :P). I wonder if
> something like _has_v3_inode() is more clear, but we can always change
> it easily enough and either way is better than hascrc() IMO.

I'm not too fond of the v3 name as the check also guards any newer
version (but then again I hope we never need to rev the version..)

Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation

[Chandan Rajendra]

On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote: 
> The size of the dinode structure is only dependent on the file system
> version, so instead of checking the individual inode version just use
> the newly added xfs_sb_version_has_large_dinode helper, and simplify
> various calling conventions.
>

I don't see any logical issues.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c  |  5 ++---
>  fs/xfs/libxfs/xfs_bmap.c       | 10 ++++------
>  fs/xfs/libxfs/xfs_format.h     | 15 +++++++--------
>  fs/xfs/libxfs/xfs_ialloc.c     |  2 +-
>  fs/xfs/libxfs/xfs_inode_buf.c  |  2 +-
>  fs/xfs/libxfs/xfs_inode_fork.c |  2 +-
>  fs/xfs/libxfs/xfs_inode_fork.h |  9 ++-------
>  fs/xfs/libxfs/xfs_log_format.h | 10 ++++------
>  fs/xfs/xfs_inode_item.c        |  4 ++--
>  fs/xfs/xfs_log_recover.c       |  2 +-
>  fs/xfs/xfs_symlink.c           |  2 +-
>  11 files changed, 26 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 4be04aeee278..a4757e5e64ca 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -537,7 +537,7 @@ xfs_attr_shortform_bytesfit(
>  	int			offset;
>  
>  	/* rounded down */
> -	offset = (XFS_LITINO(mp, dp->i_d.di_version) - bytes) >> 3;
> +	offset = (XFS_LITINO(mp) - bytes) >> 3;
>  
>  	if (dp->i_d.di_format == XFS_DINODE_FMT_DEV) {
>  		minforkoff = roundup(sizeof(xfs_dev_t), 8) >> 3;
> @@ -604,8 +604,7 @@ xfs_attr_shortform_bytesfit(
>  	minforkoff = roundup(minforkoff, 8) >> 3;
>  
>  	/* attr fork btree root can have at least this many key/ptr pairs */
> -	maxforkoff = XFS_LITINO(mp, dp->i_d.di_version) -
> -			XFS_BMDR_SPACE_CALC(MINABTPTRS);
> +	maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
>  	maxforkoff = maxforkoff >> 3;	/* rounded down */
>  
>  	if (offset >= maxforkoff)
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 43ae2ab21084..a2fe8a585100 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -193,14 +193,12 @@ xfs_default_attroffset(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	uint			offset;
>  
> -	if (mp->m_sb.sb_inodesize == 256) {
> -		offset = XFS_LITINO(mp, ip->i_d.di_version) -
> -				XFS_BMDR_SPACE_CALC(MINABTPTRS);
> -	} else {
> +	if (mp->m_sb.sb_inodesize == 256)
> +		offset = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> +	else
>  		offset = XFS_BMDR_SPACE_CALC(6 * MINABTPTRS);
> -	}
>  
> -	ASSERT(offset < XFS_LITINO(mp, ip->i_d.di_version));
> +	ASSERT(offset < XFS_LITINO(mp));
>  	return offset;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index a28bf6a978ad..aeca184d63ee 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -942,8 +942,11 @@ enum xfs_dinode_fmt {
>  /*
>   * Inode size for given fs.
>   */
> -#define XFS_LITINO(mp, version) \
> -	((int)(((mp)->m_sb.sb_inodesize) - xfs_dinode_size(version)))
> +#define XFS_DINODE_SIZE(mp) \
> +       (xfs_sb_version_has_large_dinode(&(mp)->m_sb) ? \
> +        sizeof(struct xfs_dinode) : offsetof(struct xfs_dinode, di_crc))
> +#define XFS_LITINO(mp) \
> +	((mp)->m_sb.sb_inodesize - XFS_DINODE_SIZE(mp))
>  
>  /*
>   * Inode data & attribute fork sizes, per inode.
> @@ -952,13 +955,9 @@ enum xfs_dinode_fmt {
>  #define XFS_DFORK_BOFF(dip)		((int)((dip)->di_forkoff << 3))
>  
>  #define XFS_DFORK_DSIZE(dip,mp) \
> -	(XFS_DFORK_Q(dip) ? \
> -		XFS_DFORK_BOFF(dip) : \
> -		XFS_LITINO(mp, (dip)->di_version))
> +	(XFS_DFORK_Q(dip) ? XFS_DFORK_BOFF(dip) : XFS_LITINO(mp))
>  #define XFS_DFORK_ASIZE(dip,mp) \
> -	(XFS_DFORK_Q(dip) ? \
> -		XFS_LITINO(mp, (dip)->di_version) - XFS_DFORK_BOFF(dip) : \
> -		0)
> +	(XFS_DFORK_Q(dip) ? XFS_LITINO(mp) - XFS_DFORK_BOFF(dip) : 0)
>  #define XFS_DFORK_SIZE(dip,mp,w) \
>  	((w) == XFS_DATA_FORK ? \
>  		XFS_DFORK_DSIZE(dip, mp) : \
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 6adffaa68fb8..26de817351fa 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -339,7 +339,7 @@ xfs_ialloc_inode_init(
>  		xfs_buf_zero(fbuf, 0, BBTOB(fbuf->b_length));
>  		for (i = 0; i < M_IGEO(mp)->inodes_per_cluster; i++) {
>  			int	ioffset = i << mp->m_sb.sb_inodelog;
> -			uint	isize = xfs_dinode_size(version);
> +			uint	isize = XFS_DINODE_SIZE(mp);
>  
>  			free = xfs_make_iptr(mp, fbuf, i);
>  			free->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index a5aa2f220c28..34ccf162abe1 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -432,7 +432,7 @@ xfs_dinode_verify_forkoff(
>  	case XFS_DINODE_FMT_LOCAL:	/* fall through ... */
>  	case XFS_DINODE_FMT_EXTENTS:    /* fall through ... */
>  	case XFS_DINODE_FMT_BTREE:
> -		if (dip->di_forkoff >= (XFS_LITINO(mp, dip->di_version) >> 3))
> +		if (dip->di_forkoff >= (XFS_LITINO(mp) >> 3))
>  			return __this_address;
>  		break;
>  	default:
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index ad2b9c313fd2..518c6f0ec3a6 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,7 +183,7 @@ xfs_iformat_local(
>  	 */
>  	if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
>  		xfs_warn(ip->i_mount,
> -	"corrupt inode %Lu (bad size %d for local fork, size = %d).",
> +	"corrupt inode %Lu (bad size %d for local fork, size = %zd).",
>  			(unsigned long long) ip->i_ino, size,
>  			XFS_DFORK_SIZE(dip, ip->i_mount, whichfork));
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED,
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 500333d0101e..668ee942be22 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -46,14 +46,9 @@ struct xfs_ifork {
>  			(ip)->i_afp : \
>  			(ip)->i_cowfp))
>  #define XFS_IFORK_DSIZE(ip) \
> -	(XFS_IFORK_Q(ip) ? \
> -		XFS_IFORK_BOFF(ip) : \
> -		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version))
> +	(XFS_IFORK_Q(ip) ? XFS_IFORK_BOFF(ip) : XFS_LITINO((ip)->i_mount))
>  #define XFS_IFORK_ASIZE(ip) \
> -	(XFS_IFORK_Q(ip) ? \
> -		XFS_LITINO((ip)->i_mount, (ip)->i_d.di_version) - \
> -			XFS_IFORK_BOFF(ip) : \
> -		0)
> +	(XFS_IFORK_Q(ip) ? XFS_LITINO((ip)->i_mount) - XFS_IFORK_BOFF(ip) : 0)
>  #define XFS_IFORK_SIZE(ip,w) \
>  	((w) == XFS_DATA_FORK ? \
>  		XFS_IFORK_DSIZE(ip) : \
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 9bac0d2e56dc..76defbea8000 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -424,12 +424,10 @@ struct xfs_log_dinode {
>  	/* structure must be padded to 64 bit alignment */
>  };
>  
> -static inline uint xfs_log_dinode_size(int version)
> -{
> -	if (version == 3)
> -		return sizeof(struct xfs_log_dinode);
> -	return offsetof(struct xfs_log_dinode, di_next_unlinked);
> -}
> +#define xfs_log_dinode_size(mp)						\
> +	(xfs_sb_version_has_large_dinode(&(mp)->m_sb) ?				\
> +		sizeof(struct xfs_log_dinode) :				\
> +		offsetof(struct xfs_log_dinode, di_next_unlinked))
>  
>  /*
>   * Buffer Log Format definitions
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f021b55a0301..451f9b6b2806 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -125,7 +125,7 @@ xfs_inode_item_size(
>  
>  	*nvecs += 2;
>  	*nbytes += sizeof(struct xfs_inode_log_format) +
> -		   xfs_log_dinode_size(ip->i_d.di_version);
> +		   xfs_log_dinode_size(ip->i_mount);
>  
>  	xfs_inode_item_data_fork_size(iip, nvecs, nbytes);
>  	if (XFS_IFORK_Q(ip))
> @@ -370,7 +370,7 @@ xfs_inode_item_format_core(
>  
>  	dic = xlog_prepare_iovec(lv, vecp, XLOG_REG_TYPE_ICORE);
>  	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));
> +	xlog_finish_iovec(lv, *vecp, xfs_log_dinode_size(ip->i_mount));
>  }
>  
>  /*
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index e5e976b5cc11..79fc85a4ff08 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3068,7 +3068,7 @@ xlog_recover_inode_pass2(
>  		error = -EFSCORRUPTED;
>  		goto out_release;
>  	}
> -	isize = xfs_log_dinode_size(ldip->di_version);
> +	isize = xfs_log_dinode_size(mp);
>  	if (unlikely(item->ri_buf[1].i_len > isize)) {
>  		XFS_CORRUPTION_ERROR("xlog_recover_inode_pass2(7)",
>  				     XFS_ERRLEVEL_LOW, mp, ldip,
> diff --git a/fs/xfs/xfs_symlink.c b/fs/xfs/xfs_symlink.c
> index ea42e25ec1bf..fa0fa3c70f1a 100644
> --- a/fs/xfs/xfs_symlink.c
> +++ b/fs/xfs/xfs_symlink.c
> @@ -192,7 +192,7 @@ xfs_symlink(
>  	 * The symlink will fit into the inode data fork?
>  	 * There can't be any attributes so we get the whole variable part.
>  	 */
> -	if (pathlen <= XFS_LITINO(mp, dp->i_d.di_version))
> +	if (pathlen <= XFS_LITINO(mp))
>  		fs_blocks = 0;
>  	else
>  		fs_blocks = xfs_symlink_blocks(mp, pathlen);
> 


-- 
chandan

Re: [PATCH 3/5] xfs: simplify a check in xfs_ioctl_setattr_check_cowextsize

[Chandan Rajendra]

On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote: 
> Only v5 file systems can have the reflink feature, and those will
> always use the large dinode format.  Remove the extra check for the
> inode version.
>

I don't see any logical issues.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_ioctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a1d2b9cb05a..ad825ffa7e4c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1473,8 +1473,7 @@ xfs_ioctl_setattr_check_cowextsize(
>  	if (!(fa->fsx_xflags & FS_XFLAG_COWEXTSIZE))
>  		return 0;
>  
> -	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb) ||
> -	    ip->i_d.di_version != 3)
> +	if (!xfs_sb_version_hasreflink(&ip->i_mount->m_sb))
>  		return -EINVAL;
>  
>  	if (fa->fsx_cowextsize == 0)
> 


-- 
chandan

Re: [PATCH 2/5] xfs: only check the superblock version for dinode size calculation

[Christoph Hellwig]

On Mon, Mar 16, 2020 at 09:17:07AM -0400, Brian Foster wrote:
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> > index ad2b9c313fd2..518c6f0ec3a6 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.c
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> > @@ -183,7 +183,7 @@ xfs_iformat_local(
> >  	 */
> >  	if (unlikely(size > XFS_DFORK_SIZE(dip, ip->i_mount, whichfork))) {
> >  		xfs_warn(ip->i_mount,
> > -	"corrupt inode %Lu (bad size %d for local fork, size = %d).",
> > +	"corrupt inode %Lu (bad size %d for local fork, size = %zd).",
> 
> Is this here intentionally? Otherwise seems fine:

Yes.  XFS_DFORK_SIZE now returns a size_t, so the format strings needs
to match.

Re: [PATCH 4/5] xfs: simplify di_flags2 inheritance in xfs_ialloc

[Chandan Rajendra]

On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote: 
> di_flags2 is initialized to zero for v4 and earlier file systems.  This
> means di_flags2 can only be non-zero for a v5 file systems, in which
> case both the parent and child inodes can store the filed.  Remove the
> extra di_version check, and also remove the rather pointless local
> di_flags2 variable while at it.
>

I don't see any logical issues.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_inode.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index addc3ee0cb73..ebfd8efb0efa 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -907,20 +907,13 @@ xfs_ialloc(
>  
>  			ip->i_d.di_flags |= di_flags;
>  		}
> -		if (pip &&
> -		    (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY) &&
> -		    pip->i_d.di_version == 3 &&
> -		    ip->i_d.di_version == 3) {
> -			uint64_t	di_flags2 = 0;
> -
> +		if (pip && (pip->i_d.di_flags2 & XFS_DIFLAG2_ANY)) {
>  			if (pip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE) {
> -				di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> +				ip->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  				ip->i_d.di_cowextsize = pip->i_d.di_cowextsize;
>  			}
>  			if (pip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
> -				di_flags2 |= XFS_DIFLAG2_DAX;
> -
> -			ip->i_d.di_flags2 |= di_flags2;
> +				ip->i_d.di_flags2 |= XFS_DIFLAG2_DAX;
>  		}
>  		/* FALLTHROUGH */
>  	case S_IFLNK:
> 


-- 
chandan

Re: [PATCH 5/5] xfs: remove the di_version field from struct icdinode

[Chandan Rajendra]

On Thursday, March 12, 2020 7:52 PM Christoph Hellwig wrote: 
> We know the version is 3 if on a v5 file system.   For earlier file
> systems formats we always upgrade the remaining v1 inodes to v2 and
> thus only use v2 inodes.  Use the xfs_sb_version_has_large_dinode
> helper to check if we deal with small or large dinodes, and thus
> remove the need for the di_version field in struct icdinode.
>

I don't see any logical issues.

Reviewed-by: Chandan Rajendra <chandanrlinux@gmail.com>

> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_inode_buf.c | 16 ++++++----------
>  fs/xfs/libxfs/xfs_inode_buf.h |  1 -
>  fs/xfs/xfs_bmap_util.c        | 16 ++++++++--------
>  fs/xfs/xfs_inode.c            | 16 ++--------------
>  fs/xfs/xfs_inode_item.c       |  8 +++-----
>  fs/xfs/xfs_ioctl.c            |  5 ++---
>  fs/xfs/xfs_iops.c             |  2 +-
>  fs/xfs/xfs_itable.c           |  2 +-
>  fs/xfs/xfs_log_recover.c      |  2 +-
>  9 files changed, 24 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 34ccf162abe1..7384b9194922 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -209,16 +209,14 @@ xfs_inode_from_disk(
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
>  
> -
>  	/*
>  	 * Convert v1 inodes immediately to v2 inode format as this is the
>  	 * minimum inode version format we support in the rest of the code.
> +	 * They will also be unconditionally written back to disk as v2 inodes.
>  	 */
> -	to->di_version = from->di_version;
> -	if (to->di_version == 1) {
> +	if (unlikely(from->di_version == 1)) {
>  		set_nlink(inode, be16_to_cpu(from->di_onlink));
>  		to->di_projid = 0;
> -		to->di_version = 2;
>  	} else {
>  		set_nlink(inode, be32_to_cpu(from->di_nlink));
>  		to->di_projid = (prid_t)be16_to_cpu(from->di_projid_hi) << 16 |
> @@ -256,7 +254,7 @@ xfs_inode_from_disk(
>  	to->di_dmstate	= be16_to_cpu(from->di_dmstate);
>  	to->di_flags	= be16_to_cpu(from->di_flags);
>  
> -	if (to->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
>  		inode_set_iversion_queried(inode,
>  					   be64_to_cpu(from->di_changecount));
>  		to->di_crtime.tv_sec = be32_to_cpu(from->di_crtime.t_sec);
> @@ -278,7 +276,6 @@ xfs_inode_to_disk(
>  	to->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
>  	to->di_onlink = 0;
>  
> -	to->di_version = from->di_version;
>  	to->di_format = from->di_format;
>  	to->di_uid = cpu_to_be32(i_uid_read(inode));
>  	to->di_gid = cpu_to_be32(i_gid_read(inode));
> @@ -307,7 +304,8 @@ xfs_inode_to_disk(
>  	to->di_dmstate = cpu_to_be16(from->di_dmstate);
>  	to->di_flags = cpu_to_be16(from->di_flags);
>  
> -	if (from->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		to->di_version = 3;
>  		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
>  		to->di_crtime.t_sec = cpu_to_be32(from->di_crtime.tv_sec);
>  		to->di_crtime.t_nsec = cpu_to_be32(from->di_crtime.tv_nsec);
> @@ -319,6 +317,7 @@ xfs_inode_to_disk(
>  		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
>  		to->di_flushiter = 0;
>  	} else {
> +		to->di_version = 2;
>  		to->di_flushiter = cpu_to_be16(from->di_flushiter);
>  	}
>  }
> @@ -636,7 +635,6 @@ xfs_iread(
>  	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
>  		VFS_I(ip)->i_generation = prandom_u32();
> -		ip->i_d.di_version = 3;
>  		return 0;
>  	}
>  
> @@ -678,7 +676,6 @@ xfs_iread(
>  		 * 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_version = dip->di_version;
>  		VFS_I(ip)->i_generation = be32_to_cpu(dip->di_gen);
>  		ip->i_d.di_flushiter = be16_to_cpu(dip->di_flushiter);
>  
> @@ -692,7 +689,6 @@ xfs_iread(
>  		VFS_I(ip)->i_mode = 0;
>  	}
>  
> -	ASSERT(ip->i_d.di_version >= 2);
>  	ip->i_delayed_blks = 0;
>  
>  	/*
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
> index 2683e1e2c4a6..4e24fad3deb0 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.h
> +++ b/fs/xfs/libxfs/xfs_inode_buf.h
> @@ -16,7 +16,6 @@ struct xfs_dinode;
>   * format specific structures at the appropriate time.
>   */
>  struct xfs_icdinode {
> -	int8_t		di_version;	/* inode version */
>  	int8_t		di_format;	/* format of di_c data */
>  	uint16_t	di_flushiter;	/* incremented on flush */
>  	uint32_t	di_projid;	/* owner's project id */
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 3df4d0af9f22..6ca37944d71f 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1449,12 +1449,12 @@ xfs_swap_extent_forks(
>  	 * event of a crash. Set the owner change log flags now and leave the
>  	 * bmbt scan as the last step.
>  	 */
> -	if (ip->i_d.di_version == 3 &&
> -	    ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> -		(*target_log_flags) |= XFS_ILOG_DOWNER;
> -	if (tip->i_d.di_version == 3 &&
> -	    tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> -		(*src_log_flags) |= XFS_ILOG_DOWNER;
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> +			(*target_log_flags) |= XFS_ILOG_DOWNER;
> +		if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE)
> +			(*src_log_flags) |= XFS_ILOG_DOWNER;
> +	}
>  
>  	/*
>  	 * Swap the data forks of the inodes
> @@ -1489,7 +1489,7 @@ xfs_swap_extent_forks(
>  		(*src_log_flags) |= XFS_ILOG_DEXT;
>  		break;
>  	case XFS_DINODE_FMT_BTREE:
> -		ASSERT(ip->i_d.di_version < 3 ||
> +		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
>  		       (*src_log_flags & XFS_ILOG_DOWNER));
>  		(*src_log_flags) |= XFS_ILOG_DBROOT;
>  		break;
> @@ -1501,7 +1501,7 @@ xfs_swap_extent_forks(
>  		break;
>  	case XFS_DINODE_FMT_BTREE:
>  		(*target_log_flags) |= XFS_ILOG_DBROOT;
> -		ASSERT(tip->i_d.di_version < 3 ||
> +		ASSERT(!xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb) ||
>  		       (*target_log_flags & XFS_ILOG_DOWNER));
>  		break;
>  	}
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ebfd8efb0efa..600c0b59bdd7 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -801,15 +801,6 @@ xfs_ialloc(
>  		return error;
>  	ASSERT(ip != NULL);
>  	inode = VFS_I(ip);
> -
> -	/*
> -	 * We always convert v1 inodes to v2 now - we only support filesystems
> -	 * with >= v2 inode capability, so there is no reason for ever leaving
> -	 * an inode in v1 format.
> -	 */
> -	if (ip->i_d.di_version == 1)
> -		ip->i_d.di_version = 2;
> -
>  	inode->i_mode = mode;
>  	set_nlink(inode, nlink);
>  	inode->i_uid = current_fsuid();
> @@ -847,14 +838,13 @@ xfs_ialloc(
>  	ip->i_d.di_dmstate = 0;
>  	ip->i_d.di_flags = 0;
>  
> -	if (ip->i_d.di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		inode_set_iversion(inode, 1);
>  		ip->i_d.di_flags2 = 0;
>  		ip->i_d.di_cowextsize = 0;
>  		ip->i_d.di_crtime = tv;
>  	}
>  
> -
>  	flags = XFS_ILOG_CORE;
>  	switch (mode & S_IFMT) {
>  	case S_IFIFO:
> @@ -1115,7 +1105,6 @@ xfs_bumplink(
>  {
>  	xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_CHG);
>  
> -	ASSERT(ip->i_d.di_version > 1);
>  	inc_nlink(VFS_I(ip));
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  }
> @@ -3798,7 +3787,6 @@ xfs_iflush_int(
>  	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
>  	       ip->i_d.di_nextents > XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK));
>  	ASSERT(iip != NULL && iip->ili_fields != 0);
> -	ASSERT(ip->i_d.di_version > 1);
>  
>  	/* set *dip = inode's place in the buffer */
>  	dip = xfs_buf_offset(bp, ip->i_imap.im_boffset);
> @@ -3859,7 +3847,7 @@ xfs_iflush_int(
>  	 * backwards compatibility with old kernels that predate logging all
>  	 * inode changes.
>  	 */
> -	if (ip->i_d.di_version < 3)
> +	if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		ip->i_d.di_flushiter++;
>  
>  	/* Check the inline fork data before we write out. */
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index 451f9b6b2806..5800bfda96b2 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -305,8 +305,6 @@ xfs_inode_to_log_dinode(
>  	struct inode		*inode = VFS_I(ip);
>  
>  	to->di_magic = XFS_DINODE_MAGIC;
> -
> -	to->di_version = from->di_version;
>  	to->di_format = from->di_format;
>  	to->di_uid = i_uid_read(inode);
>  	to->di_gid = i_gid_read(inode);
> @@ -339,7 +337,8 @@ xfs_inode_to_log_dinode(
>  	/* log a dummy value to ensure log structure is fully initialised */
>  	to->di_next_unlinked = NULLAGINO;
>  
> -	if (from->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&ip->i_mount->m_sb)) {
> +		to->di_version = 3;
>  		to->di_changecount = inode_peek_iversion(inode);
>  		to->di_crtime.t_sec = from->di_crtime.tv_sec;
>  		to->di_crtime.t_nsec = from->di_crtime.tv_nsec;
> @@ -351,6 +350,7 @@ xfs_inode_to_log_dinode(
>  		uuid_copy(&to->di_uuid, &ip->i_mount->m_sb.sb_meta_uuid);
>  		to->di_flushiter = 0;
>  	} else {
> +		to->di_version = 2;
>  		to->di_flushiter = from->di_flushiter;
>  	}
>  }
> @@ -395,8 +395,6 @@ xfs_inode_item_format(
>  	struct xfs_log_iovec	*vecp = NULL;
>  	struct xfs_inode_log_format *ilf;
>  
> -	ASSERT(ip->i_d.di_version > 1);
> -
>  	ilf = xlog_prepare_iovec(lv, &vecp, XLOG_REG_TYPE_IFORMAT);
>  	ilf->ilf_type = XFS_LI_INODE;
>  	ilf->ilf_ino = ip->i_ino;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index ad825ffa7e4c..a98909cc7ecb 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1263,7 +1263,7 @@ xfs_ioctl_setattr_xflags(
>  
>  	/* diflags2 only valid for v3 inodes. */
>  	di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> -	if (di_flags2 && ip->i_d.di_version < 3)
> +	if (di_flags2 && !xfs_sb_version_has_large_dinode(&mp->m_sb))
>  		return -EINVAL;
>  
>  	ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> @@ -1601,7 +1601,6 @@ xfs_ioctl_setattr(
>  			olddquot = xfs_qm_vop_chown(tp, ip,
>  						&ip->i_pdquot, pdqp);
>  		}
> -		ASSERT(ip->i_d.di_version > 1);
>  		ip->i_d.di_projid = fa->fsx_projid;
>  	}
>  
> @@ -1614,7 +1613,7 @@ xfs_ioctl_setattr(
>  		ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
>  	else
>  		ip->i_d.di_extsize = 0;
> -	if (ip->i_d.di_version == 3 &&
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb) &&
>  	    (ip->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
>  		ip->i_d.di_cowextsize = fa->fsx_cowextsize >>
>  				mp->m_sb.sb_blocklog;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 87093a05aad7..df2cd57e984e 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -557,7 +557,7 @@ xfs_vn_getattr(
>  	stat->blocks =
>  		XFS_FSB_TO_BB(mp, ip->i_d.di_nblocks + ip->i_delayed_blks);
>  
> -	if (ip->i_d.di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		if (request_mask & STATX_BTIME) {
>  			stat->result_mask |= STATX_BTIME;
>  			stat->btime = ip->i_d.di_crtime;
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index d10660469884..4d4dad557c2f 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -110,7 +110,7 @@ xfs_bulkstat_one_int(
>  	buf->bs_forkoff = XFS_IFORK_BOFF(ip);
>  	buf->bs_version = XFS_BULKSTAT_VERSION_V5;
>  
> -	if (dic->di_version == 3) {
> +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
>  		if (dic->di_flags2 & XFS_DIFLAG2_COWEXTSIZE)
>  			buf->bs_cowextsize_blks = dic->di_cowextsize;
>  	}
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 79fc85a4ff08..9db6003d125b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -2869,8 +2869,8 @@ xfs_recover_inode_owner_change(
>  		return -ENOMEM;
>  
>  	/* instantiate the inode */
> +	ASSERT(dip->di_version >= 3);
>  	xfs_inode_from_disk(ip, dip);
> -	ASSERT(ip->i_d.di_version >= 3);
>  
>  	error = xfs_iformat_fork(ip, dip);
>  	if (error)
> 


-- 
chandan

Re: [PATCH 1/5] xfs: add a new xfs_sb_version_has_large_dinode helper

[Darrick J. Wong]

On Mon, Mar 16, 2020 at 09:16:49AM -0400, Brian Foster wrote:
> On Thu, Mar 12, 2020 at 03:22:31PM +0100, Christoph Hellwig wrote:
> > Add a new wrapper to check if a file system supports the newer large
> > dinode format.  Previously we uses xfs_sb_version_hascrc for that,
> > which is technically correct but a little confusing to read.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/libxfs/xfs_format.h     |  5 +++++
> >  fs/xfs/libxfs/xfs_ialloc.c     |  4 ++--
> >  fs/xfs/libxfs/xfs_inode_buf.c  | 12 ++++++++----
> >  fs/xfs/libxfs/xfs_trans_resv.c |  2 +-
> >  fs/xfs/xfs_buf_item.c          |  2 +-
> >  fs/xfs/xfs_log_recover.c       |  2 +-
> >  6 files changed, 18 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index cd814f99da28..a28bf6a978ad 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -497,6 +497,11 @@ static inline bool xfs_sb_version_hascrc(struct xfs_sb *sbp)
> >  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> >  }
> >  
> 
> A comment would be useful here to indicate what this means (i.e., we can
> assume v3 inode format). The function name is a little vague too I
> suppose (will the inode ever get larger than large? :P). I wonder if
> something like _has_v3_inode() is more clear, but we can always change
> it easily enough and either way is better than hascrc() IMO.
> 
> Brian
> 
> > +static inline bool xfs_sb_version_has_large_dinode(struct xfs_sb *sbp)

/me agrees that this function ought to have a comment saying that
"large_dinode" means v3 inodes.

Not sure about larger than large inodes though -- the last di_flags2
could be made to mean "...and now expand structure by X bytes" whenever
we run out of space.

(The rest of the series looks ok to me)

--D

> > +{
> > +	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > +}
> > +
> >  static inline bool xfs_sb_version_has_pquotino(struct xfs_sb *sbp)
> >  {
> >  	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5;
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index b4a404278935..6adffaa68fb8 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -304,7 +304,7 @@ xfs_ialloc_inode_init(
> >  	 * That means for v3 inode we log the entire buffer rather than just the
> >  	 * inode cores.
> >  	 */
> > -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> >  		version = 3;
> >  		ino = XFS_AGINO_TO_INO(mp, agno, XFS_AGB_TO_AGINO(mp, agbno));
> >  
> > @@ -2872,7 +2872,7 @@ xfs_ialloc_setup_geometry(
> >  	 * cannot change the behavior.
> >  	 */
> >  	igeo->inode_cluster_size_raw = XFS_INODE_BIG_CLUSTER_SIZE;
> > -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +	if (xfs_sb_version_has_large_dinode(&mp->m_sb)) {
> >  		int	new_size = igeo->inode_cluster_size_raw;
> >  
> >  		new_size *= mp->m_sb.sb_inodesize / XFS_DINODE_MIN_SIZE;
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 17e88a8c8353..a5aa2f220c28 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -44,14 +44,18 @@ xfs_inobp_check(
> >  }
> >  #endif
> >  
> > +/*
> > + * v4 and earlier file systems only support the small dinode, and must use the
> > + * v1 or v2 inode formats.  v5 file systems support a larger dinode, and must
> > + * use the v3 inode format.
> > + */
> >  bool
> >  xfs_dinode_good_version(
> >  	struct xfs_mount *mp,
> >  	__u8		version)
> >  {
> > -	if (xfs_sb_version_hascrc(&mp->m_sb))
> > +	if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> >  		return version == 3;
> > -
> >  	return version == 1 || version == 2;
> >  }
> >  
> > @@ -454,7 +458,7 @@ xfs_dinode_verify(
> >  
> >  	/* Verify v3 integrity information first */
> >  	if (dip->di_version >= 3) {
> > -		if (!xfs_sb_version_hascrc(&mp->m_sb))
> > +		if (!xfs_sb_version_has_large_dinode(&mp->m_sb))
> >  			return __this_address;
> >  		if (!xfs_verify_cksum((char *)dip, mp->m_sb.sb_inodesize,
> >  				      XFS_DINODE_CRC_OFF))
> > @@ -629,7 +633,7 @@ xfs_iread(
> >  
> >  	/* shortcut IO on inode allocation if possible */
> >  	if ((iget_flags & XFS_IGET_CREATE) &&
> > -	    xfs_sb_version_hascrc(&mp->m_sb) &&
> > +	    xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> >  	    !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> >  		VFS_I(ip)->i_generation = prandom_u32();
> >  		ip->i_d.di_version = 3;
> > diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> > index 7a9c04920505..294e23d47912 100644
> > --- a/fs/xfs/libxfs/xfs_trans_resv.c
> > +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> > @@ -187,7 +187,7 @@ xfs_calc_inode_chunk_res(
> >  			       XFS_FSB_TO_B(mp, 1));
> >  	if (alloc) {
> >  		/* icreate tx uses ordered buffers */
> > -		if (xfs_sb_version_hascrc(&mp->m_sb))
> > +		if (xfs_sb_version_has_large_dinode(&mp->m_sb))
> >  			return res;
> >  		size = XFS_FSB_TO_B(mp, 1);
> >  	}
> > diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> > index 663810e6cd59..d004ae3455d7 100644
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -345,7 +345,7 @@ xfs_buf_item_format(
> >  	 * occurs during recovery.
> >  	 */
> >  	if (bip->bli_flags & XFS_BLI_INODE_BUF) {
> > -		if (xfs_sb_version_hascrc(&lip->li_mountp->m_sb) ||
> > +		if (xfs_sb_version_has_large_dinode(&lip->li_mountp->m_sb) ||
> >  		    !((bip->bli_flags & XFS_BLI_INODE_ALLOC_BUF) &&
> >  		      xfs_log_item_in_current_chkpt(lip)))
> >  			bip->__bli_format.blf_flags |= XFS_BLF_INODE_BUF;
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index 6abc0863c9c3..e5e976b5cc11 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -2997,7 +2997,7 @@ xlog_recover_inode_pass2(
> >  	 * superblock flag to determine whether we need to look at di_flushiter
> >  	 * to skip replay when the on disk inode is newer than the log one
> >  	 */
> > -	if (!xfs_sb_version_hascrc(&mp->m_sb) &&
> > +	if (!xfs_sb_version_has_large_dinode(&mp->m_sb) &&
> >  	    ldip->di_flushiter < be16_to_cpu(dip->di_flushiter)) {
> >  		/*
> >  		 * Deal with the wrap case, DI_MAX_FLUSH is less
> > -- 
> > 2.24.1
> > 
>