On Mon, Oct 16, 2017 at 02:16:47PM +1100, Dave Chinner wrote:

On Thu, Oct 12, 2017 at 03:32:50PM -0700, Darrick J. Wong wrote:

quoted

On Wed, Oct 11, 2017 at 06:43:00PM -0700, Darrick J. Wong wrote:

quoted

From: Darrick J. Wong <redacted>

Scrub the fields within an inode.

.....

quoted

quoted

+
+/*
+ * Given an inode and the scrub control structure, grab either the
+ * inode referenced in the control structure or the inode passed in.
+ * The inode is not locked.
+ */
+int
+xfs_scrub_get_inode(
+	struct xfs_scrub_context	*sc,
+	struct xfs_inode		*ip_in)
+{
+	struct xfs_mount		*mp = sc->mp;
+	struct xfs_inode		*ip = NULL;
+	int				error;
+
+	/*
+	 * If userspace passed us an AG number or a generation number
+	 * without an inode number, they haven't got a clue so bail out
+	 * immediately.
+	 */
+	if (sc->sm->sm_agno || (sc->sm->sm_gen && !sc->sm->sm_ino))
+		return -EINVAL;
+
+	/* We want to scan the inode we already had opened. */
+	if (sc->sm->sm_ino == 0 || sc->sm->sm_ino == ip_in->i_ino) {
+		sc->ip = ip_in;
+		return 0;
+	}
+
+	/* Look up the inode, see if the generation number matches. */
+	if (xfs_internal_inum(mp, sc->sm->sm_ino))
+		return -ENOENT;
+	error = xfs_iget(mp, NULL, sc->sm->sm_ino,
+			XFS_IGET_UNTRUSTED | XFS_IGET_DONTCACHE, 0, &ip);
+	if (error == -ENOENT || error == -EINVAL) {
+		/* inode doesn't exist... */
+		return -ENOENT;
+	} else if (error) {
+		trace_xfs_scrub_op_error(sc,
+				XFS_INO_TO_AGNO(mp, sc->sm->sm_ino),
+				XFS_INO_TO_AGBNO(mp, sc->sm->sm_ino),
+				error, __return_address);
+		return error;
+	}
+	if (VFS_I(ip)->i_generation != sc->sm->sm_gen) {
+		iput(VFS_I(ip));
+		return -ENOENT;
+	}
+
+	sc->ip = ip;
+	return 0;
+}

Much nicer with the way everything is clearly spelled out :P

quoted

quoted

+/* Inode core */
+
+/*
+ * di_extsize hint validation is somewhat cumbersome. Rules are:
+ *
+ * 1. extent size hint is only valid for directories and regular files
+ * 2. DIFLAG_EXTSIZE is only valid for regular files
+ * 3. DIFLAG_EXTSZINHERIT is only valid for directories.
+ * 4. extsize hint of 0 turns off hints, clears inode flags.
+ * 5. either flag must be set if extsize != 0
+ * 6. Extent size must be a multiple of the appropriate block size.
+ * 7. extent size hint cannot be longer than maximum extent length
+ * 8. for non-realtime files, the extent size hint must be limited
+ *    to half the AG size to avoid alignment extending the extent
+ *    beyond the limits of the AG.
+ */
+STATIC void
+xfs_scrub_inode_extsize(
+	struct xfs_scrub_context	*sc,
+	struct xfs_buf			*bp,
+	struct xfs_dinode		*dip,
+	xfs_ino_t			ino,
+	uint16_t			mode,
+	uint16_t			flags)
+{
+	struct xfs_mount		*mp = sc->mp;
+	bool				rt_flag;
+	bool				hint_flag;
+	bool				inherit_flag;
+	uint32_t			extsize;
+	uint32_t			extsize_bytes;
+	uint32_t			blocksize_bytes;
+
+	rt_flag = (flags & XFS_DIFLAG_REALTIME);
+	hint_flag = (flags & XFS_DIFLAG_EXTSIZE);
+	inherit_flag = (flags & XFS_DIFLAG_EXTSZINHERIT);
+	extsize = be32_to_cpu(dip->di_extsize);
+	extsize_bytes = XFS_FSB_TO_B(sc->mp, extsize);
+
+	if (rt_flag)
+		blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
+	else
+		blocksize_bytes = mp->m_sb.sb_blocksize;
+
+	if ((hint_flag || inherit_flag) && (!S_ISDIR(mode) && !S_ISREG(mode)))

Logic is a correct but reads funny:

	if ((hint_flag || inherit_flag) &&
	    !(S_ISREG(mode) || S_ISDIR(mode)))

Ok.  Fixed this and the cowextsize.

quoted

quoted

+/*
+ * di_cowextsize hint validation is somewhat cumbersome. Rules are:
+ *
+ * 1. flag requires reflink feature
+ * 2. cow extent size hint is only valid for directories and regular files
+ * 3. cow extsize hint of 0 turns off hints, clears inode flags.
+ * 4. either flag must be set if cow extsize != 0
+ * 5. flag cannot be set for rt files
+ * 6. Extent size must be a multiple of the appropriate block size.
+ * 7. extent size hint cannot be longer than maximum extent length
+ * 8. the extent size hint must be limited
+ *    to half the AG size to avoid alignment extending the extent
+ *    beyond the limits of the AG.
+ */

Perhaps this comment doesn't need duplicating for a 3rd time. Maybe
for both di_extsize and di_cowextsize just say:

/*
 * Extent size hints have explicit rules. They are documented at
 * xfs_ioctl_setattr_check_extsize() - these functions need to be
 * kept in sync with each other.
 */

Ok.  I've also amended the comment at xfs_ioctl_setattr_check_extsize to
remind people to keep the scrub version in sync.

quoted

quoted

+STATIC void
+xfs_scrub_inode_cowextsize(
+	struct xfs_scrub_context	*sc,
+	struct xfs_buf			*bp,
+	struct xfs_dinode		*dip,
+	xfs_ino_t			ino,
+	uint16_t			mode,
+	uint16_t			flags,
+	uint64_t			flags2)
+{
+	struct xfs_mount		*mp = sc->mp;
+	bool				rt_flag;
+	bool				hint_flag;
+	uint32_t			extsize;
+	uint32_t			extsize_bytes;
+
+	rt_flag = (flags & XFS_DIFLAG_REALTIME);
+	hint_flag = (flags2 & XFS_DIFLAG2_COWEXTSIZE);
+	extsize = be32_to_cpu(dip->di_extsize);

Doh, this ought to be extsize = be32_to_cpu(dip->di_cowextsize); will fix.

Yup, with that fix in place all the spurious inode warnings I was
getting went away.

quoted

quoted

+/* Map and read a raw inode. */
+STATIC int
+xfs_scrub_inode_map_raw(
+	struct xfs_scrub_context	*sc,
+	xfs_ino_t			ino,
+	struct xfs_buf			**bpp,
+	struct xfs_dinode		**dipp)
+{
+	struct xfs_imap			imap;
+	struct xfs_mount		*mp = sc->mp;
+	struct xfs_buf			*bp;
+	struct xfs_dinode		*dip;
+	int				error;
+
+	error = xfs_imap(mp, sc->tp, ino, &imap, XFS_IGET_UNTRUSTED);
+	if (error == -EINVAL) {
+		/*
+		 * Inode could have gotten deleted out from under us;
+		 * just forget about it.
+		 */
+		error = -ENOENT;
+		goto out;
+	}
+	if (!xfs_scrub_process_error(sc, XFS_INO_TO_AGNO(mp, ino),
+			XFS_INO_TO_AGBNO(mp, ino), &error))
+		goto out;
+
+	error = xfs_trans_read_buf(mp, sc->tp, mp->m_ddev_targp,
+			imap.im_blkno, imap.im_len, XBF_UNMAPPED, &bp,
+			NULL);
+	if (!xfs_scrub_process_error(sc, XFS_INO_TO_AGNO(mp, ino),
+			XFS_INO_TO_AGBNO(mp, ino), &error))
+		goto out;
+
+	/* Is this really an inode? */
+	bp->b_ops = &xfs_inode_buf_ops;

A comment here on why we skip the read verifier when pulling in the
inode buffer would be nice.

/*
 * Is this really an inode?  We disabled verifiers in the above
 * xfs_trans_read_buf call because the inode buffer verifier
 * fails on /any/ inode record in the inode cluster with a bad
 * magic or version number, not just the one that we're
 * checking.  Therefore, grab the buffer unconditionally, attach
 * the inode verifiers by hand, and run the inode verifier only
 * on the one inode we want.
 */

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html