[PATCH 0/3] xfs: preparation for syncing with 5.8

[PATCH 0/3] xfs: preparation for syncing with 5.8

[Darrick J. Wong]

Hi all,

Here are a few patches to prepare for API changes in 5.8.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=libxfs-5.8-prep
---
 db/attrset.c             |    6 +--
 db/check.c               |  100 ++++++++++++++++++++++++++--------------------
 include/xfs_inode.h      |    6 +--
 libxfs/libxfs_api_defs.h |    1 
 libxfs/rdwr.c            |    5 +-
 libxfs/util.c            |    3 -
 repair/dir2.c            |   13 +++++-
 repair/phase6.c          |   80 +++++--------------------------------
 repair/phase7.c          |    2 -
 repair/quotacheck.c      |    4 +-
 repair/xfs_repair.c      |    3 -
 11 files changed, 88 insertions(+), 135 deletions(-)

[PATCH 1/3] xfs_db: stop misusing an onstack inode

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The onstack inode in xfs_check's process_inode is a potential landmine
since it's not a /real/ incore inode.  The upcoming 5.8 merge will make
this messier wrt inode forks, so just remove the onstack inode and
reference the ondisk fields directly.  This also reduces the amount of
thinking that I have to do w.r.t. future libxfs porting efforts.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/check.c |  100 ++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 56 insertions(+), 44 deletions(-)


diff --git a/db/check.c b/db/check.c
index 12c03b6d..96abea21 100644
--- a/db/check.c
+++ b/db/check.c
@@ -2707,7 +2707,6 @@ process_inode(
 {
 	blkmap_t		*blkmap;
 	xfs_fsblock_t		bno = 0;
-	struct xfs_inode	xino;
 	inodata_t		*id = NULL;
 	xfs_ino_t		ino;
 	xfs_extnum_t		nextents = 0;
@@ -2724,6 +2723,12 @@ process_inode(
 	xfs_qcnt_t		rc = 0;
 	int			v = 0;
 	mode_t			mode;
+	uint16_t		diflags;
+	uint64_t		diflags2 = 0;
+	xfs_nlink_t		nlink;
+	xfs_dqid_t		uid;
+	xfs_dqid_t		gid;
+	xfs_dqid_t		prid;
 	static char		okfmts[] = {
 		0,				/* type 0 unused */
 		1 << XFS_DINODE_FMT_DEV,	/* FIFO */
@@ -2750,10 +2755,6 @@ process_inode(
 		"dev", "local", "extents", "btree", "uuid"
 	};
 
-	/* xfs_inode_from_disk expects to have an mp to work with */
-	xino.i_mount = mp;
-	libxfs_inode_from_disk(&xino, dip);
-
 	ino = XFS_AGINO_TO_INO(mp, be32_to_cpu(agf->agf_seqno), agino);
 	if (!isfree) {
 		id = find_inode(ino, 1);
@@ -2775,12 +2776,25 @@ process_inode(
 		error++;
 		return;
 	}
+	if (dip->di_version == 1) {
+		nlink = be16_to_cpu(dip->di_onlink);
+		prid = 0;
+	} else {
+		nlink = be32_to_cpu(dip->di_nlink);
+		prid = (xfs_dqid_t)be16_to_cpu(dip->di_projid_hi) << 16 |
+				   be16_to_cpu(dip->di_projid_lo);
+	}
+	uid = be32_to_cpu(dip->di_uid);
+	gid = be32_to_cpu(dip->di_gid);
+	diflags = be16_to_cpu(dip->di_flags);
+	if (xfs_sb_version_has_v3inode(&mp->m_sb))
+		diflags2 = be64_to_cpu(dip->di_flags2);
 	if (isfree) {
-		if (xino.i_d.di_nblocks != 0) {
+		if (be64_to_cpu(dip->di_nblocks) != 0) {
 			if (v)
 				dbprintf(_("bad nblocks %lld for free inode "
 					 "%lld\n"),
-					xino.i_d.di_nblocks, ino);
+					be64_to_cpu(dip->di_nblocks), ino);
 			error++;
 		}
 		if (dip->di_nlink != 0) {
@@ -2809,24 +2823,24 @@ process_inode(
 	 */
 	mode = be16_to_cpu(dip->di_mode);
 	if ((((mode & S_IFMT) >> 12) > 15) ||
-	    (!(okfmts[(mode & S_IFMT) >> 12] & (1 << xino.i_d.di_format)))) {
+	    (!(okfmts[(mode & S_IFMT) >> 12] & (1 << dip->di_format)))) {
 		if (v)
 			dbprintf(_("bad format %d for inode %lld type %#o\n"),
-				xino.i_d.di_format, id->ino, mode & S_IFMT);
+				dip->di_format, id->ino, mode & S_IFMT);
 		error++;
 		return;
 	}
 	if ((unsigned int)XFS_DFORK_ASIZE(dip, mp) >= XFS_LITINO(mp)) {
 		if (v)
 			dbprintf(_("bad fork offset %d for inode %lld\n"),
-				xino.i_d.di_forkoff, id->ino);
+				dip->di_forkoff, id->ino);
 		error++;
 		return;
 	}
-	if ((unsigned int)xino.i_d.di_aformat > XFS_DINODE_FMT_BTREE)  {
+	if ((unsigned int)dip->di_aformat > XFS_DINODE_FMT_BTREE)  {
 		if (v)
 			dbprintf(_("bad attribute format %d for inode %lld\n"),
-				xino.i_d.di_aformat, id->ino);
+				dip->di_aformat, id->ino);
 		error++;
 		return;
 	}
@@ -2834,43 +2848,43 @@ process_inode(
 		dbprintf(_("inode %lld mode %#o fmt %s "
 			 "afmt %s "
 			 "nex %d anex %d nblk %lld sz %lld%s%s%s%s%s%s%s\n"),
-			id->ino, mode, fmtnames[(int)xino.i_d.di_format],
-			fmtnames[(int)xino.i_d.di_aformat],
-			xino.i_d.di_nextents,
-			xino.i_d.di_anextents,
-			xino.i_d.di_nblocks, xino.i_d.di_size,
-			xino.i_d.di_flags & XFS_DIFLAG_REALTIME ? " rt" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_PREALLOC ? " pre" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_IMMUTABLE? " imm" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_APPEND   ? " app" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_SYNC     ? " syn" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_NOATIME  ? " noa" : "",
-			xino.i_d.di_flags & XFS_DIFLAG_NODUMP   ? " nod" : "");
+			id->ino, mode, fmtnames[(int)dip->di_format],
+			fmtnames[(int)dip->di_aformat],
+			be32_to_cpu(dip->di_nextents),
+			be16_to_cpu(dip->di_anextents),
+			be64_to_cpu(dip->di_nblocks), be64_to_cpu(dip->di_size),
+			diflags & XFS_DIFLAG_REALTIME ? " rt" : "",
+			diflags & XFS_DIFLAG_PREALLOC ? " pre" : "",
+			diflags & XFS_DIFLAG_IMMUTABLE? " imm" : "",
+			diflags & XFS_DIFLAG_APPEND   ? " app" : "",
+			diflags & XFS_DIFLAG_SYNC     ? " syn" : "",
+			diflags & XFS_DIFLAG_NOATIME  ? " noa" : "",
+			diflags & XFS_DIFLAG_NODUMP   ? " nod" : "");
 	security = 0;
 	switch (mode & S_IFMT) {
 	case S_IFDIR:
 		type = DBM_DIR;
-		if (xino.i_d.di_format == XFS_DINODE_FMT_LOCAL)
+		if (dip->di_format == XFS_DINODE_FMT_LOCAL)
 			break;
-		blkmap = blkmap_alloc(xino.i_d.di_nextents);
+		blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
 		break;
 	case S_IFREG:
-		if (xino.i_d.di_flags & XFS_DIFLAG_REALTIME)
+		if (diflags & XFS_DIFLAG_REALTIME)
 			type = DBM_RTDATA;
 		else if (id->ino == mp->m_sb.sb_rbmino) {
 			type = DBM_RTBITMAP;
-			blkmap = blkmap_alloc(xino.i_d.di_nextents);
+			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
 			addlink_inode(id);
 		} else if (id->ino == mp->m_sb.sb_rsumino) {
 			type = DBM_RTSUM;
-			blkmap = blkmap_alloc(xino.i_d.di_nextents);
+			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
 			addlink_inode(id);
 		}
 		else if (id->ino == mp->m_sb.sb_uquotino ||
 			 id->ino == mp->m_sb.sb_gquotino ||
 			 id->ino == mp->m_sb.sb_pquotino) {
 			type = DBM_QUOTA;
-			blkmap = blkmap_alloc(xino.i_d.di_nextents);
+			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
 			addlink_inode(id);
 		}
 		else
@@ -2887,10 +2901,10 @@ process_inode(
 		break;
 	}
 
-	id->isreflink = !!(xino.i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
-	setlink_inode(id, VFS_I(&xino)->i_nlink, type == DBM_DIR, security);
+	id->isreflink = !!(diflags2 & XFS_DIFLAG2_REFLINK);
+	setlink_inode(id, nlink, type == DBM_DIR, security);
 
-	switch (xino.i_d.di_format) {
+	switch (dip->di_format) {
 	case XFS_DINODE_FMT_LOCAL:
 		process_lclinode(id, dip, type, &totdblocks, &totiblocks,
 			&nextents, &blkmap, XFS_DATA_FORK);
@@ -2906,7 +2920,7 @@ process_inode(
 	}
 	if (XFS_DFORK_Q(dip)) {
 		sbversion |= XFS_SB_VERSION_ATTRBIT;
-		switch (xino.i_d.di_aformat) {
+		switch (dip->di_aformat) {
 		case XFS_DINODE_FMT_LOCAL:
 			process_lclinode(id, dip, DBM_ATTR, &atotdblocks,
 				&atotiblocks, &anextents, NULL, XFS_ATTR_FORK);
@@ -2941,30 +2955,28 @@ process_inode(
 		default:
 			break;
 		}
-		if (ic) {
-			quota_add(&xino.i_d.di_projid, &VFS_I(&xino)->i_gid,
-				  &VFS_I(&xino)->i_uid, 0, bc, ic, rc);
-		}
+		if (ic)
+			quota_add(&prid, &gid, &uid, 0, bc, ic, rc);
 	}
 	totblocks = totdblocks + totiblocks + atotdblocks + atotiblocks;
-	if (totblocks != xino.i_d.di_nblocks) {
+	if (totblocks != be64_to_cpu(dip->di_nblocks)) {
 		if (v)
 			dbprintf(_("bad nblocks %lld for inode %lld, counted "
 				 "%lld\n"),
-				xino.i_d.di_nblocks, id->ino, totblocks);
+				be64_to_cpu(dip->di_nblocks), id->ino, totblocks);
 		error++;
 	}
-	if (nextents != xino.i_d.di_nextents) {
+	if (nextents != be32_to_cpu(dip->di_nextents)) {
 		if (v)
 			dbprintf(_("bad nextents %d for inode %lld, counted %d\n"),
-				xino.i_d.di_nextents, id->ino, nextents);
+				be32_to_cpu(dip->di_nextents), id->ino, nextents);
 		error++;
 	}
-	if (anextents != xino.i_d.di_anextents) {
+	if (anextents != be16_to_cpu(dip->di_anextents)) {
 		if (v)
 			dbprintf(_("bad anextents %d for inode %lld, counted "
 				 "%d\n"),
-				xino.i_d.di_anextents, id->ino, anextents);
+				be16_to_cpu(dip->di_anextents), id->ino, anextents);
 		error++;
 	}
 	if (type == DBM_DIR)

Re: [PATCH 1/3] xfs_db: stop misusing an onstack inode

[Christoph Hellwig]

On Tue, Jul 14, 2020 at 02:46:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The onstack inode in xfs_check's process_inode is a potential landmine
> since it's not a /real/ incore inode.  The upcoming 5.8 merge will make
> this messier wrt inode forks, so just remove the onstack inode and
> reference the ondisk fields directly.  This also reduces the amount of
> thinking that I have to do w.r.t. future libxfs porting efforts.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Comparing this to my version here:

http://git.infradead.org/users/hch/xfsprogs.git/commitdiff/791d7d324290dbb83be7bf35fe15f9898f5df1c1

>  	mode_t			mode;
> +	uint16_t		diflags;
> +	uint64_t		diflags2 = 0;
> +	xfs_nlink_t		nlink;
> +	xfs_dqid_t		uid;
> +	xfs_dqid_t		gid;
> +	xfs_dqid_t		prid;

Not sure we really need the local variables, as they are mostly just
used once except for error messages..

> +	if (dip->di_version == 1) {
> +		nlink = be16_to_cpu(dip->di_onlink);
> +		prid = 0;
> +	} else {
> +		nlink = be32_to_cpu(dip->di_nlink);
> +		prid = (xfs_dqid_t)be16_to_cpu(dip->di_projid_hi) << 16 |
> +				   be16_to_cpu(dip->di_projid_lo);
> +	}

I mad the assumption that we don't support v1 inodes anymore, but
it appears we actually do.  So we might need to keep these two.

>  	if (isfree) {
> -		if (xino.i_d.di_nblocks != 0) {
> +		if (be64_to_cpu(dip->di_nblocks) != 0) {

No need to byte swap for a comparism with 0.

> -	if ((unsigned int)xino.i_d.di_aformat > XFS_DINODE_FMT_BTREE)  {
> +	if ((unsigned int)dip->di_aformat > XFS_DINODE_FMT_BTREE)  {

No need for the (pre-existing) cast here.

> +			fmtnames[(int)dip->di_aformat],

Same here.

Re: [PATCH 1/3] xfs_db: stop misusing an onstack inode

[Darrick J. Wong]

On Wed, Jul 15, 2020 at 07:38:49PM +0100, Christoph Hellwig wrote:
> On Tue, Jul 14, 2020 at 02:46:43PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The onstack inode in xfs_check's process_inode is a potential landmine
> > since it's not a /real/ incore inode.  The upcoming 5.8 merge will make
> > this messier wrt inode forks, so just remove the onstack inode and
> > reference the ondisk fields directly.  This also reduces the amount of
> > thinking that I have to do w.r.t. future libxfs porting efforts.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Comparing this to my version here:
> 
> http://git.infradead.org/users/hch/xfsprogs.git/commitdiff/791d7d324290dbb83be7bf35fe15f9898f5df1c1
> 
> >  	mode_t			mode;
> > +	uint16_t		diflags;
> > +	uint64_t		diflags2 = 0;

<shrug> I'd rather just leave these over cluttering up the function with
repeated flags/flags2 endian conversions.

> > +	xfs_nlink_t		nlink;

Actually, we do need this one for proper error reporting because we
still (sort of) have to have v1 disk format support.  For that matter,
the nlink checking further in that function looks totally wrong; it
should be using nlink and not accessing the raw field directly.

> > +	xfs_dqid_t		uid;
> > +	xfs_dqid_t		gid;
> > +	xfs_dqid_t		prid;

These actually are needed, because the quota_add function that uses them
requires pointers to xfs_dqid_t to do some hazy magic to skip quota
accounting if the pointer is null, which implies local variables since
the ondisk inode has be32 values...

That whole thing is messy (and gets worse with the project id) but I'd
rather not go rearchitecting more of db/check.c seeing as we can
probably just kill it after xfsprogs 5.8 releases.

> Not sure we really need the local variables, as they are mostly just
> used once except for error messages..
> 
> > +	if (dip->di_version == 1) {
> > +		nlink = be16_to_cpu(dip->di_onlink);
> > +		prid = 0;
> > +	} else {
> > +		nlink = be32_to_cpu(dip->di_nlink);
> > +		prid = (xfs_dqid_t)be16_to_cpu(dip->di_projid_hi) << 16 |
> > +				   be16_to_cpu(dip->di_projid_lo);
> > +	}
> 
> I mad the assumption that we don't support v1 inodes anymore, but
> it appears we actually do.  So we might need to keep these two.

<nod>

> >  	if (isfree) {
> > -		if (xino.i_d.di_nblocks != 0) {
> > +		if (be64_to_cpu(dip->di_nblocks) != 0) {
> 
> No need to byte swap for a comparism with 0.
> 
> > -	if ((unsigned int)xino.i_d.di_aformat > XFS_DINODE_FMT_BTREE)  {
> > +	if ((unsigned int)dip->di_aformat > XFS_DINODE_FMT_BTREE)  {
> 
> No need for the (pre-existing) cast here.
>
> > +			fmtnames[(int)dip->di_aformat],

Fixed all of these.

--D

> 
> Same here.

Re: [PATCH 1/3] xfs_db: stop misusing an onstack inode

[Eric Sandeen]

On 7/14/20 2:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The onstack inode in xfs_check's process_inode is a potential landmine
> since it's not a /real/ incore inode.  The upcoming 5.8 merge will make
> this messier wrt inode forks, so just remove the onstack inode and
> reference the ondisk fields directly.  This also reduces the amount of
> thinking that I have to do w.r.t. future libxfs porting efforts.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I ended up merging & testing the original patch last week, and Darrick's
recent response indicates that likely only the cosmetic changes seemed
worth making; given that I've already run regression on this patch as is,
I'm going to go ahead & give it my rvb, and keep it merged.  We can clean
up the other stuff in another patch if needed.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  db/check.c |  100 ++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 56 insertions(+), 44 deletions(-)
> 
> 
> diff --git a/db/check.c b/db/check.c
> index 12c03b6d..96abea21 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -2707,7 +2707,6 @@ process_inode(
>  {
>  	blkmap_t		*blkmap;
>  	xfs_fsblock_t		bno = 0;
> -	struct xfs_inode	xino;
>  	inodata_t		*id = NULL;
>  	xfs_ino_t		ino;
>  	xfs_extnum_t		nextents = 0;
> @@ -2724,6 +2723,12 @@ process_inode(
>  	xfs_qcnt_t		rc = 0;
>  	int			v = 0;
>  	mode_t			mode;
> +	uint16_t		diflags;
> +	uint64_t		diflags2 = 0;
> +	xfs_nlink_t		nlink;
> +	xfs_dqid_t		uid;
> +	xfs_dqid_t		gid;
> +	xfs_dqid_t		prid;
>  	static char		okfmts[] = {
>  		0,				/* type 0 unused */
>  		1 << XFS_DINODE_FMT_DEV,	/* FIFO */
> @@ -2750,10 +2755,6 @@ process_inode(
>  		"dev", "local", "extents", "btree", "uuid"
>  	};
>  
> -	/* xfs_inode_from_disk expects to have an mp to work with */
> -	xino.i_mount = mp;
> -	libxfs_inode_from_disk(&xino, dip);
> -
>  	ino = XFS_AGINO_TO_INO(mp, be32_to_cpu(agf->agf_seqno), agino);
>  	if (!isfree) {
>  		id = find_inode(ino, 1);
> @@ -2775,12 +2776,25 @@ process_inode(
>  		error++;
>  		return;
>  	}
> +	if (dip->di_version == 1) {
> +		nlink = be16_to_cpu(dip->di_onlink);
> +		prid = 0;
> +	} else {
> +		nlink = be32_to_cpu(dip->di_nlink);
> +		prid = (xfs_dqid_t)be16_to_cpu(dip->di_projid_hi) << 16 |
> +				   be16_to_cpu(dip->di_projid_lo);
> +	}
> +	uid = be32_to_cpu(dip->di_uid);
> +	gid = be32_to_cpu(dip->di_gid);
> +	diflags = be16_to_cpu(dip->di_flags);
> +	if (xfs_sb_version_has_v3inode(&mp->m_sb))
> +		diflags2 = be64_to_cpu(dip->di_flags2);
>  	if (isfree) {
> -		if (xino.i_d.di_nblocks != 0) {
> +		if (be64_to_cpu(dip->di_nblocks) != 0) {
>  			if (v)
>  				dbprintf(_("bad nblocks %lld for free inode "
>  					 "%lld\n"),
> -					xino.i_d.di_nblocks, ino);
> +					be64_to_cpu(dip->di_nblocks), ino);
>  			error++;
>  		}
>  		if (dip->di_nlink != 0) {
> @@ -2809,24 +2823,24 @@ process_inode(
>  	 */
>  	mode = be16_to_cpu(dip->di_mode);
>  	if ((((mode & S_IFMT) >> 12) > 15) ||
> -	    (!(okfmts[(mode & S_IFMT) >> 12] & (1 << xino.i_d.di_format)))) {
> +	    (!(okfmts[(mode & S_IFMT) >> 12] & (1 << dip->di_format)))) {
>  		if (v)
>  			dbprintf(_("bad format %d for inode %lld type %#o\n"),
> -				xino.i_d.di_format, id->ino, mode & S_IFMT);
> +				dip->di_format, id->ino, mode & S_IFMT);
>  		error++;
>  		return;
>  	}
>  	if ((unsigned int)XFS_DFORK_ASIZE(dip, mp) >= XFS_LITINO(mp)) {
>  		if (v)
>  			dbprintf(_("bad fork offset %d for inode %lld\n"),
> -				xino.i_d.di_forkoff, id->ino);
> +				dip->di_forkoff, id->ino);
>  		error++;
>  		return;
>  	}
> -	if ((unsigned int)xino.i_d.di_aformat > XFS_DINODE_FMT_BTREE)  {
> +	if ((unsigned int)dip->di_aformat > XFS_DINODE_FMT_BTREE)  {
>  		if (v)
>  			dbprintf(_("bad attribute format %d for inode %lld\n"),
> -				xino.i_d.di_aformat, id->ino);
> +				dip->di_aformat, id->ino);
>  		error++;
>  		return;
>  	}
> @@ -2834,43 +2848,43 @@ process_inode(
>  		dbprintf(_("inode %lld mode %#o fmt %s "
>  			 "afmt %s "
>  			 "nex %d anex %d nblk %lld sz %lld%s%s%s%s%s%s%s\n"),
> -			id->ino, mode, fmtnames[(int)xino.i_d.di_format],
> -			fmtnames[(int)xino.i_d.di_aformat],
> -			xino.i_d.di_nextents,
> -			xino.i_d.di_anextents,
> -			xino.i_d.di_nblocks, xino.i_d.di_size,
> -			xino.i_d.di_flags & XFS_DIFLAG_REALTIME ? " rt" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_PREALLOC ? " pre" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_IMMUTABLE? " imm" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_APPEND   ? " app" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_SYNC     ? " syn" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_NOATIME  ? " noa" : "",
> -			xino.i_d.di_flags & XFS_DIFLAG_NODUMP   ? " nod" : "");
> +			id->ino, mode, fmtnames[(int)dip->di_format],
> +			fmtnames[(int)dip->di_aformat],
> +			be32_to_cpu(dip->di_nextents),
> +			be16_to_cpu(dip->di_anextents),
> +			be64_to_cpu(dip->di_nblocks), be64_to_cpu(dip->di_size),
> +			diflags & XFS_DIFLAG_REALTIME ? " rt" : "",
> +			diflags & XFS_DIFLAG_PREALLOC ? " pre" : "",
> +			diflags & XFS_DIFLAG_IMMUTABLE? " imm" : "",
> +			diflags & XFS_DIFLAG_APPEND   ? " app" : "",
> +			diflags & XFS_DIFLAG_SYNC     ? " syn" : "",
> +			diflags & XFS_DIFLAG_NOATIME  ? " noa" : "",
> +			diflags & XFS_DIFLAG_NODUMP   ? " nod" : "");
>  	security = 0;
>  	switch (mode & S_IFMT) {
>  	case S_IFDIR:
>  		type = DBM_DIR;
> -		if (xino.i_d.di_format == XFS_DINODE_FMT_LOCAL)
> +		if (dip->di_format == XFS_DINODE_FMT_LOCAL)
>  			break;
> -		blkmap = blkmap_alloc(xino.i_d.di_nextents);
> +		blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
>  		break;
>  	case S_IFREG:
> -		if (xino.i_d.di_flags & XFS_DIFLAG_REALTIME)
> +		if (diflags & XFS_DIFLAG_REALTIME)
>  			type = DBM_RTDATA;
>  		else if (id->ino == mp->m_sb.sb_rbmino) {
>  			type = DBM_RTBITMAP;
> -			blkmap = blkmap_alloc(xino.i_d.di_nextents);
> +			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
>  			addlink_inode(id);
>  		} else if (id->ino == mp->m_sb.sb_rsumino) {
>  			type = DBM_RTSUM;
> -			blkmap = blkmap_alloc(xino.i_d.di_nextents);
> +			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
>  			addlink_inode(id);
>  		}
>  		else if (id->ino == mp->m_sb.sb_uquotino ||
>  			 id->ino == mp->m_sb.sb_gquotino ||
>  			 id->ino == mp->m_sb.sb_pquotino) {
>  			type = DBM_QUOTA;
> -			blkmap = blkmap_alloc(xino.i_d.di_nextents);
> +			blkmap = blkmap_alloc(be32_to_cpu(dip->di_nextents));
>  			addlink_inode(id);
>  		}
>  		else
> @@ -2887,10 +2901,10 @@ process_inode(
>  		break;
>  	}
>  
> -	id->isreflink = !!(xino.i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
> -	setlink_inode(id, VFS_I(&xino)->i_nlink, type == DBM_DIR, security);
> +	id->isreflink = !!(diflags2 & XFS_DIFLAG2_REFLINK);
> +	setlink_inode(id, nlink, type == DBM_DIR, security);
>  
> -	switch (xino.i_d.di_format) {
> +	switch (dip->di_format) {
>  	case XFS_DINODE_FMT_LOCAL:
>  		process_lclinode(id, dip, type, &totdblocks, &totiblocks,
>  			&nextents, &blkmap, XFS_DATA_FORK);
> @@ -2906,7 +2920,7 @@ process_inode(
>  	}
>  	if (XFS_DFORK_Q(dip)) {
>  		sbversion |= XFS_SB_VERSION_ATTRBIT;
> -		switch (xino.i_d.di_aformat) {
> +		switch (dip->di_aformat) {
>  		case XFS_DINODE_FMT_LOCAL:
>  			process_lclinode(id, dip, DBM_ATTR, &atotdblocks,
>  				&atotiblocks, &anextents, NULL, XFS_ATTR_FORK);
> @@ -2941,30 +2955,28 @@ process_inode(
>  		default:
>  			break;
>  		}
> -		if (ic) {
> -			quota_add(&xino.i_d.di_projid, &VFS_I(&xino)->i_gid,
> -				  &VFS_I(&xino)->i_uid, 0, bc, ic, rc);
> -		}
> +		if (ic)
> +			quota_add(&prid, &gid, &uid, 0, bc, ic, rc);
>  	}
>  	totblocks = totdblocks + totiblocks + atotdblocks + atotiblocks;
> -	if (totblocks != xino.i_d.di_nblocks) {
> +	if (totblocks != be64_to_cpu(dip->di_nblocks)) {
>  		if (v)
>  			dbprintf(_("bad nblocks %lld for inode %lld, counted "
>  				 "%lld\n"),
> -				xino.i_d.di_nblocks, id->ino, totblocks);
> +				be64_to_cpu(dip->di_nblocks), id->ino, totblocks);
>  		error++;
>  	}
> -	if (nextents != xino.i_d.di_nextents) {
> +	if (nextents != be32_to_cpu(dip->di_nextents)) {
>  		if (v)
>  			dbprintf(_("bad nextents %d for inode %lld, counted %d\n"),
> -				xino.i_d.di_nextents, id->ino, nextents);
> +				be32_to_cpu(dip->di_nextents), id->ino, nextents);
>  		error++;
>  	}
> -	if (anextents != xino.i_d.di_anextents) {
> +	if (anextents != be16_to_cpu(dip->di_anextents)) {
>  		if (v)
>  			dbprintf(_("bad anextents %d for inode %lld, counted "
>  				 "%d\n"),
> -				xino.i_d.di_anextents, id->ino, anextents);
> +				be16_to_cpu(dip->di_anextents), id->ino, anextents);
>  		error++;
>  	}
>  	if (type == DBM_DIR)
> 

[PATCH 2/3] xfs_repair: never zero a shortform '..' entry

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Repair has this strange behavior during phase 4 where it will zero the
parent pointer entry of a shortform directory if the pointer is
obviously invalid.  Unfortunately, this causes the inode fork verifiers
to fail, so change it to reset bad pointers (ondisk) to the root
directory.  If repair crashes, a subsequent run will notice the
incorrect parent pointer and either fix the dir or move it to
lost+found.

Note that we maintain the practice of setting the *incore* parent to
NULLFSINO so that phase 7 knows that it has to fix the directory.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dir2.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)


diff --git a/repair/dir2.c b/repair/dir2.c
index b374bc7b..61e1aaaf 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -165,7 +165,6 @@ process_sf_dir2(
 	int			tmp_elen;
 	int			tmp_len;
 	xfs_dir2_sf_entry_t	*tmp_sfep;
-	xfs_ino_t		zero = 0;
 
 	sfp = (struct xfs_dir2_sf_hdr *)XFS_DFORK_DPTR(dip);
 	max_size = XFS_DFORK_DSIZE(dip, mp);
@@ -494,7 +493,11 @@ _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			/*
+			 * Set the ondisk parent to the root inode so that we
+			 * never write garbage parent pointers to disk.
+			 */
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {
@@ -529,7 +532,11 @@ _("bad .. entry in directory inode %" PRIu64 ", points to self, "),
 		if (!no_modify)  {
 			do_warn(_("clearing inode number\n"));
 
-			libxfs_dir2_sf_put_parent_ino(sfp, zero);
+			/*
+			 * Set the ondisk parent to the root inode so that we
+			 * never write garbage parent pointers to disk.
+			 */
+			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
 			*dino_dirty = 1;
 			*repair = 1;
 		} else  {

Re: [PATCH 2/3] xfs_repair: never zero a shortform '..' entry

[Christoph Hellwig]

On Tue, Jul 14, 2020 at 02:46:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Repair has this strange behavior during phase 4 where it will zero the
> parent pointer entry of a shortform directory if the pointer is
> obviously invalid.  Unfortunately, this causes the inode fork verifiers
> to fail, so change it to reset bad pointers (ondisk) to the root
> directory.  If repair crashes, a subsequent run will notice the
> incorrect parent pointer and either fix the dir or move it to
> lost+found.
> 
> Note that we maintain the practice of setting the *incore* parent to
> NULLFSINO so that phase 7 knows that it has to fix the directory.

I think we probably want to take Brians series instead, which does
a few more things in the area.

[PATCH 3/3] libxfs: remove ifork_ops from all client programs

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

We're removing ifork_ops from libxfs in 5.8, so start by collapsing the
usages back into a single place.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/attrset.c             |    6 +--
 include/xfs_inode.h      |    6 ++-
 libxfs/libxfs_api_defs.h |    1 -
 libxfs/rdwr.c            |    5 +--
 libxfs/util.c            |    3 +-
 repair/phase6.c          |   80 ++++++----------------------------------------
 repair/phase7.c          |    2 +
 repair/quotacheck.c      |    4 +-
 repair/xfs_repair.c      |    3 +-
 9 files changed, 22 insertions(+), 88 deletions(-)


diff --git a/db/attrset.c b/db/attrset.c
index b86ecec7..98a08a49 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -153,8 +153,7 @@ attr_set_f(
 		memset(args.value, 'v', args.valuelen);
 	}
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
-			&xfs_default_ifork_ops)) {
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
@@ -238,8 +237,7 @@ attr_remove_f(
 		return 0;
 	}
 
-	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp,
-			&xfs_default_ifork_ops)) {
+	if (libxfs_iget(mp, NULL, iocur_top->ino, 0, &args.dp)) {
 		dbprintf(_("failed to iget inode %llu\n"),
 			(unsigned long long)iocur_top->ino);
 		goto out;
diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index 3caeeb39..ce8f6599 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -160,9 +160,9 @@ extern struct timespec64 current_time(struct inode *inode);
 
 /* Inode Cache Interfaces */
 extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
-extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
-				uint, struct xfs_inode **,
-				struct xfs_ifork_ops *);
+extern int	libxfs_iget(struct xfs_mount *mp, struct xfs_trans *tp,
+				xfs_ino_t ino, uint lock_flags,
+				struct xfs_inode **ipp);
 extern void	libxfs_irele(struct xfs_inode *ip);
 
 #endif /* __XFS_INODE_H__ */
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 1a7cdbf9..e7e42e93 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -60,7 +60,6 @@
 #define xfs_da_hashname			libxfs_da_hashname
 #define xfs_da_read_buf			libxfs_da_read_buf
 #define xfs_da_shrink_inode		libxfs_da_shrink_inode
-#define xfs_default_ifork_ops		libxfs_default_ifork_ops
 #define xfs_defer_cancel		libxfs_defer_cancel
 #define xfs_defer_finish		libxfs_defer_finish
 #define xfs_dinode_calc_crc		libxfs_dinode_calc_crc
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 13a414d7..2eaf099e 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1262,8 +1262,7 @@ libxfs_iget(
 	struct xfs_trans	*tp,
 	xfs_ino_t		ino,
 	uint			lock_flags,
-	struct xfs_inode	**ipp,
-	struct xfs_ifork_ops	*ifork_ops)
+	struct xfs_inode	**ipp)
 {
 	struct xfs_inode	*ip;
 	int			error = 0;
@@ -1281,7 +1280,7 @@ libxfs_iget(
 		return error;
 	}
 
-	ip->i_fork_ops = ifork_ops;
+	ip->i_fork_ops = &xfs_default_ifork_ops;
 	if (!libxfs_inode_verify_forks(ip)) {
 		libxfs_irele(ip);
 		return -EFSCORRUPTED;
diff --git a/libxfs/util.c b/libxfs/util.c
index 914e4ca5..4bf06082 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -247,8 +247,7 @@ libxfs_ialloc(
 	}
 	ASSERT(*ialloc_context == NULL);
 
-	error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = libxfs_iget(tp->t_mountp, tp, ino, 0, &ip);
 	if (error != 0)
 		return error;
 	ASSERT(ip != NULL);
diff --git a/repair/phase6.c b/repair/phase6.c
index b6391326..f69582d4 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -26,58 +26,6 @@ static struct xfs_name		xfs_name_dot = {(unsigned char *)".",
 						1,
 						XFS_DIR3_FT_DIR};
 
-/*
- * When we're checking directory inodes, we're allowed to set a directory's
- * dotdot entry to zero to signal that the parent needs to be reconnected
- * during phase 6.  If we're handling a shortform directory the ifork
- * verifiers will fail, so temporarily patch out this canary so that we can
- * verify the rest of the fork and move on to fixing the dir.
- */
-static xfs_failaddr_t
-phase6_verify_dir(
-	struct xfs_inode		*ip)
-{
-	struct xfs_mount		*mp = ip->i_mount;
-	struct xfs_ifork		*ifp;
-	struct xfs_dir2_sf_hdr		*sfp;
-	xfs_failaddr_t			fa;
-	xfs_ino_t			old_parent;
-	bool				parent_bypass = false;
-	int				size;
-
-	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
-	sfp = (struct xfs_dir2_sf_hdr *)ifp->if_u1.if_data;
-	size = ifp->if_bytes;
-
-	/*
-	 * If this is a shortform directory, phase4 may have set the parent
-	 * inode to zero to indicate that it must be fixed.  Temporarily
-	 * set a valid parent so that the directory verifier will pass.
-	 */
-	if (size > offsetof(struct xfs_dir2_sf_hdr, parent) &&
-	    size >= xfs_dir2_sf_hdr_size(sfp->i8count)) {
-		old_parent = libxfs_dir2_sf_get_parent_ino(sfp);
-		if (old_parent == 0) {
-			libxfs_dir2_sf_put_parent_ino(sfp, mp->m_sb.sb_rootino);
-			parent_bypass = true;
-		}
-	}
-
-	fa = libxfs_default_ifork_ops.verify_dir(ip);
-
-	/* Put it back. */
-	if (parent_bypass)
-		libxfs_dir2_sf_put_parent_ino(sfp, old_parent);
-
-	return fa;
-}
-
-static struct xfs_ifork_ops phase6_ifork_ops = {
-	.verify_attr	= xfs_attr_shortform_verify,
-	.verify_dir	= phase6_verify_dir,
-	.verify_symlink	= xfs_symlink_shortform_verify,
-};
-
 /*
  * Data structures used to keep track of directories where the ".."
  * entries are updated. These must be rebuilt after the initial pass
@@ -529,8 +477,7 @@ mk_rbmino(xfs_mount_t *mp)
 	if (i)
 		res_failed(i);
 
-	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rbmino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rbmino, 0, &ip);
 	if (error) {
 		do_error(
 		_("couldn't iget realtime bitmap inode -- error - %d\n"),
@@ -628,8 +575,7 @@ fill_rbmino(xfs_mount_t *mp)
 	if (error)
 		res_failed(error);
 
-	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rbmino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rbmino, 0, &ip);
 	if (error) {
 		do_error(
 		_("couldn't iget realtime bitmap inode -- error - %d\n"),
@@ -699,8 +645,7 @@ fill_rsumino(xfs_mount_t *mp)
 	if (error)
 		res_failed(error);
 
-	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rsumino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rsumino, 0, &ip);
 	if (error) {
 		do_error(
 		_("couldn't iget realtime summary inode -- error - %d\n"),
@@ -772,8 +717,7 @@ mk_rsumino(xfs_mount_t *mp)
 	if (i)
 		res_failed(i);
 
-	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rsumino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rsumino, 0, &ip);
 	if (error) {
 		do_error(
 		_("couldn't iget realtime summary inode -- error - %d\n"),
@@ -871,8 +815,7 @@ mk_root_dir(xfs_mount_t *mp)
 	if (i)
 		res_failed(i);
 
-	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rootino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, mp->m_sb.sb_rootino, 0, &ip);
 	if (error) {
 		do_error(_("could not iget root inode -- error - %d\n"), error);
 	}
@@ -946,8 +889,7 @@ mk_orphanage(xfs_mount_t *mp)
 	 * would have been cleared in phase3 and phase4.
 	 */
 
-	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
-			&xfs_default_ifork_ops);
+	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip);
 	if (i)
 		do_error(_("%d - couldn't iget root inode to obtain %s\n"),
 			i, ORPHANAGE);
@@ -971,8 +913,7 @@ mk_orphanage(xfs_mount_t *mp)
 	 * use iget/ijoin instead of trans_iget because the ialloc
 	 * wrapper can commit the transaction and start a new one
 	 */
-/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip,
-			&xfs_default_ifork_ops);
+/*	i = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &pip);
 	if (i)
 		do_error(_("%d - couldn't iget root inode to make %s\n"),
 			i, ORPHANAGE);*/
@@ -1080,8 +1021,7 @@ mv_orphanage(
 	xname.len = snprintf((char *)fname, sizeof(fname), "%llu",
 				(unsigned long long)ino);
 
-	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip,
-			&xfs_default_ifork_ops);
+	err = -libxfs_iget(mp, NULL, orphanage_ino, 0, &orphanage_ip);
 	if (err)
 		do_error(_("%d - couldn't iget orphanage inode\n"), err);
 	/*
@@ -1094,7 +1034,7 @@ mv_orphanage(
 					(unsigned long long)ino, ++incr);
 
 	/* Orphans may not have a proper parent, so use custom ops here */
-	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p, &phase6_ifork_ops);
+	err = -libxfs_iget(mp, NULL, ino, 0, &ino_p);
 	if (err)
 		do_error(_("%d - couldn't iget disconnected inode\n"), err);
 
@@ -2868,7 +2808,7 @@ process_dir_inode(
 
 	ASSERT(!is_inode_refchecked(irec, ino_offset) || dotdot_update);
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &phase6_ifork_ops);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
 	if (error) {
 		if (!no_modify)
 			do_error(
diff --git a/repair/phase7.c b/repair/phase7.c
index 47e76b56..30cb46f9 100644
--- a/repair/phase7.c
+++ b/repair/phase7.c
@@ -33,7 +33,7 @@ update_inode_nlinks(
 	error = -libxfs_trans_alloc(mp, &M_RES(mp)->tr_remove, nres, 0, 0, &tp);
 	ASSERT(error == 0);
 
-	error = -libxfs_iget(mp, tp, ino, 0, &ip, &xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, tp, ino, 0, &ip);
 	if (error)  {
 		if (!no_modify)
 			do_error(
diff --git a/repair/quotacheck.c b/repair/quotacheck.c
index 0df1f2be..87f81c3d 100644
--- a/repair/quotacheck.c
+++ b/repair/quotacheck.c
@@ -210,7 +210,7 @@ quotacheck_adjust(
 	    ino == mp->m_sb.sb_pquotino)
 		return;
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
 	if (error) {
 		do_warn(
 	_("could not open file %"PRIu64" for quotacheck, err=%d\n"),
@@ -367,7 +367,7 @@ quotacheck_verify(
 	if (!dquots || !chkd_flags)
 		return;
 
-	error = -libxfs_iget(mp, NULL, ino, 0, &ip, &xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, NULL, ino, 0, &ip);
 	if (error) {
 		do_warn(
 	_("could not open %s inode %"PRIu64" for quotacheck, err=%d\n"),
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index d687edea..5efc5586 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -459,8 +459,7 @@ has_plausible_rootdir(
 	int			error;
 	bool			ret = false;
 
-	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip,
-			&xfs_default_ifork_ops);
+	error = -libxfs_iget(mp, NULL, mp->m_sb.sb_rootino, 0, &ip);
 	if (error)
 		goto out;
 	if (!S_ISDIR(VFS_I(ip)->i_mode))