[PATCH 0/6] xfs: fix the bulkstat mess

[PATCH 0/6] xfs: fix the bulkstat mess

[Dave Chinner]

Hi folks,

This series of patches applies on top of the xfs-fixes-for-3.18-rc3
branch in the upstream repository. The bulkstat refactoring in 3.17
broke bulkstat in a couple of subtle ways and exposed an API wart
that was undocumented and completely non-obvious and these lead to
xfsdump failing badly on some filesystems. The first 5 patches in
this series are necessary to fix the regressions, the 6th patch
cleans up a remaining loose end.

Essentially, the old bulkstat code was pretty much unchanged in
algorithm since 1995 when a commit was made to fix a deadlock. See
the commit in the XFS archive tree 314ccac26af ("Fix a locking bug
in bulkstat by rewriting it"). Unfortunately, this commit introduced
the unexpected behviour that the inode pointed to by the lastino
cookie is not included in the bulkstat output.

THe result of this is some tricky code in xfsdump - again
undocumented - where a specific bulkstat scan during the dump
process does not use the bulkstat lastino cookie, but uses it's own
internal inode map to determine where to start the next bulkstat
call. In this case, the bulkstat cookie has a magic "target - 1"
calculation. In all other bulkstat cases, the cookie from the
previous call is passed into the next call. This magic code in
cfsdump was introduced in 2006 as an optimisation to avoid making
too many bulkstat calls.

Essentially, the xfsdump code is working around the undocumented
lastino cookie behaviour. Seeing as userspace has already coded for
this bug, we can't change that behaviour now, and that's exactly
what the refactoring in 3.17 did - it rejected "lastino - 1" in
certain circumstances with EINVAL instead of doing the right thing
and returning the correct inodes.

But, this was the last problem I found, because the EINVAL was
dropped on the floor and ignored, because bulkstat only returned
errors from the formatting function of inodes it did find. Hence the
fact it didn't scan an inode it should have was silently lost.

I found this problem after I realised that errors from the
formatting function are only returned if no other inodes were
successfully formatted at all. Which means, in reality, errors
encountered during bulkstat are universally ignored and the result
is "successful" scans that are silently missing inodes. Which means
xfsdump succeeds, but xfsrestore tells us that it was a corrupted
dump.

Of course, in getting to this point, I first had to add the missing
checks in the btree walk termination detection, fix the user buffer
space tracking, the inode tracking, the slightly broken chunk
formattting loop conditions, remove a bunch of unused variables,
and so on. There was a *lot* that didn't work correctly and I'm
still slightly stunned at the fact it worked well enough to pass
all the xfsdump/restore QA in xfstests.

As such, the first 5 patches are -stable kernel material, and the
last patch should go with them into the 3.18 tree to complete the
cleanup of code. This needs eyeballs and testing - create some
metadumps of your production filesystems and use them to test
xfsdump on a kernel with these changes......

Comments, thoughts, testing all welcome as this needs to go upstream
sooner rather than later....

-Dave.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/6] xfs: bulkstat btree walk doesn't terminate

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The bulkstat code has several different ways of detecting the end of
an AG when doing a walk. They are not consistently detected, and the
code that checks for the end of AG conditions is not consistently
coded. Hence the are conditions where the walk code can get stuck in
an endless loop making no progress and not triggering any
termination conditions.

Convert all the "tmp/i" status return codes from btree operations
to a common name (stat) and apply end-of-ag detection to these
operations consistently.

cc: stable@vger.kernel.org
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 7765ff7..16737cb 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -356,7 +356,6 @@ xfs_bulkstat(
 	int			end_of_ag; /* set if we've seen the ag end */
 	int			error;	/* error code */
 	int                     fmterror;/* bulkstat formatter result */
-	int			i;	/* loop index */
 	int			icount;	/* count of inodes good in irbuf */
 	size_t			irbsize; /* size of irec buffer in bytes */
 	xfs_ino_t		ino;	/* inode number (filesystem) */
@@ -366,11 +365,11 @@ xfs_bulkstat(
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
 	int			rval;	/* return value error code */
-	int			tmp;	/* result value from btree calls */
 	int			ubcount; /* size of user's buffer */
 	int			ubleft;	/* bytes left in user's buffer */
 	char			__user *ubufp;	/* pointer into user's buffer */
 	int			ubelem;	/* spaces used in user's buffer */
+	int			stat;
 
 	/*
 	 * Get the last inode value, see if there's nothing to do.
@@ -436,13 +435,15 @@ xfs_bulkstat(
 				agino = r.ir_startino + XFS_INODES_PER_CHUNK;
 			}
 			/* Increment to the next record */
-			error = xfs_btree_increment(cur, 0, &tmp);
+			error = xfs_btree_increment(cur, 0, &stat);
 		} else {
 			/* Start of ag.  Lookup the first inode chunk */
-			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &tmp);
+			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
 		}
-		if (error)
+		if (error || stat == 0) {
+			end_of_ag = 1;
 			goto del_cursor;
+		}
 
 		/*
 		 * Loop through inode btree records in this ag,
@@ -451,8 +452,8 @@ xfs_bulkstat(
 		while (irbp < irbufend && icount < ubcount) {
 			struct xfs_inobt_rec_incore	r;
 
-			error = xfs_inobt_get_rec(cur, &r, &i);
-			if (error || i == 0) {
+			error = xfs_inobt_get_rec(cur, &r, &stat);
+			if (error || stat == 0) {
 				end_of_ag = 1;
 				goto del_cursor;
 			}
@@ -473,8 +474,8 @@ xfs_bulkstat(
 			 * Set agino to after this chunk and bump the cursor.
 			 */
 			agino = r.ir_startino + XFS_INODES_PER_CHUNK;
-			error = xfs_btree_increment(cur, 0, &tmp);
-			if (error) {
+			error = xfs_btree_increment(cur, 0, &stat);
+			if (error || stat == 0) {
 				end_of_ag = 1;
 				goto del_cursor;
 			}
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/6] xfs: bulkstat chunk formatting cursor is broken

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The xfs_bulkstat_agichunk formatting cursor takes buffer values from
the main loop and passes them via the structure to the chunk
formatter, and the writes the chnged values back into the main loop
local variables. UNfortunately, this complex dance is full of corner
cases that aren't handled correctly.

The biggest problem is that it is double handling the information in
both the main loop and the chunk formatting function, leading to
inconsistent updates and endless loops where progress is not made.

To fix this, push the struct xfs_bulkstat_agichunk outwards to be
the primary holder of user buffer information. this removes the
double handling in the main loop.

Also, pass the last inode processed by the chunk formatter as a
separate parameter as it purely an output variable and is not
related to the user buffer consumption cursor.

Finally, the chunk formatting code is not shared by anyone, so make
it local to xfs_itable.c.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 59 +++++++++++++++++++++++++----------------------------
 fs/xfs/xfs_itable.h | 16 ---------------
 2 files changed, 28 insertions(+), 47 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 16737cb..50a3e59 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -262,20 +262,26 @@ xfs_bulkstat_grab_ichunk(
 
 #define XFS_BULKSTAT_UBLEFT(ubleft)	((ubleft) >= statstruct_size)
 
+struct xfs_bulkstat_agichunk {
+	char		__user **ac_ubuffer;/* pointer into user's buffer */
+	int		ac_ubleft;	/* bytes left in user's buffer */
+	int		ac_ubelem;	/* spaces used in user's buffer */
+};
+
 /*
  * Process inodes in chunk with a pointer to a formatter function
  * that will iget the inode and fill in the appropriate structure.
  */
-int
+static int
 xfs_bulkstat_ag_ichunk(
 	struct xfs_mount		*mp,
 	xfs_agnumber_t			agno,
 	struct xfs_inobt_rec_incore	*irbp,
 	bulkstat_one_pf			formatter,
 	size_t				statstruct_size,
-	struct xfs_bulkstat_agichunk	*acp)
+	struct xfs_bulkstat_agichunk	*acp,
+	xfs_ino_t			*lastino)
 {
-	xfs_ino_t			lastino = acp->ac_lastino;
 	char				__user **ubufp = acp->ac_ubuffer;
 	int				ubleft = acp->ac_ubleft;
 	int				ubelem = acp->ac_ubelem;
@@ -295,7 +301,7 @@ xfs_bulkstat_ag_ichunk(
 
 		/* Skip if this inode is free */
 		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
-			lastino = ino;
+			*lastino = ino;
 			continue;
 		}
 
@@ -313,7 +319,7 @@ xfs_bulkstat_ag_ichunk(
 				ubleft = 0;
 				break;
 			}
-			lastino = ino;
+			*lastino = ino;
 			continue;
 		}
 		if (fmterror == BULKSTAT_RV_GIVEUP) {
@@ -325,10 +331,9 @@ xfs_bulkstat_ag_ichunk(
 			*ubufp += ubused;
 		ubleft -= ubused;
 		ubelem++;
-		lastino = ino;
+		*lastino = ino;
 	}
 
-	acp->ac_lastino = lastino;
 	acp->ac_ubleft = ubleft;
 	acp->ac_ubelem = ubelem;
 
@@ -355,7 +360,6 @@ xfs_bulkstat(
 	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
 	int			end_of_ag; /* set if we've seen the ag end */
 	int			error;	/* error code */
-	int                     fmterror;/* bulkstat formatter result */
 	int			icount;	/* count of inodes good in irbuf */
 	size_t			irbsize; /* size of irec buffer in bytes */
 	xfs_ino_t		ino;	/* inode number (filesystem) */
@@ -366,10 +370,8 @@ xfs_bulkstat(
 	int			nirbuf;	/* size of irbuf */
 	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
-	int			ubleft;	/* bytes left in user's buffer */
-	char			__user *ubufp;	/* pointer into user's buffer */
-	int			ubelem;	/* spaces used in user's buffer */
 	int			stat;
+	struct xfs_bulkstat_agichunk ac;
 
 	/*
 	 * Get the last inode value, see if there's nothing to do.
@@ -386,11 +388,13 @@ xfs_bulkstat(
 	}
 
 	ubcount = *ubcountp; /* statstruct's */
-	ubleft = ubcount * statstruct_size; /* bytes */
-	*ubcountp = ubelem = 0;
+	ac.ac_ubuffer = &ubuffer;
+	ac.ac_ubleft = ubcount * statstruct_size; /* bytes */;
+	ac.ac_ubelem = 0;
+
+	*ubcountp = 0;
 	*done = 0;
-	fmterror = 0;
-	ubufp = ubuffer;
+
 	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
 	if (!irbuf)
 		return -ENOMEM;
@@ -402,7 +406,7 @@ xfs_bulkstat(
 	 * inode returned; 0 means start of the allocation group.
 	 */
 	rval = 0;
-	while (XFS_BULKSTAT_UBLEFT(ubleft) && agno < mp->m_sb.sb_agcount) {
+	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
 		cond_resched();
 		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
 		if (error)
@@ -497,28 +501,21 @@ del_cursor:
 		 */
 		irbufend = irbp;
 		for (irbp = irbuf;
-		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ubleft); irbp++) {
-			struct xfs_bulkstat_agichunk ac;
-
-			ac.ac_lastino = lastino;
-			ac.ac_ubuffer = &ubuffer;
-			ac.ac_ubleft = ubleft;
-			ac.ac_ubelem = ubelem;
+		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
+		     irbp++) {
 			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
-					formatter, statstruct_size, &ac);
+					formatter, statstruct_size, &ac,
+					&lastino);
 			if (error)
 				rval = error;
 
-			lastino = ac.ac_lastino;
-			ubleft = ac.ac_ubleft;
-			ubelem = ac.ac_ubelem;
-
 			cond_resched();
 		}
+
 		/*
 		 * Set up for the next loop iteration.
 		 */
-		if (XFS_BULKSTAT_UBLEFT(ubleft)) {
+		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
 			if (end_of_ag) {
 				agno++;
 				agino = 0;
@@ -531,11 +528,11 @@ del_cursor:
 	 * Done, we're either out of filesystem or space to put the data.
 	 */
 	kmem_free(irbuf);
-	*ubcountp = ubelem;
+	*ubcountp = ac.ac_ubelem;
 	/*
 	 * Found some inodes, return them now and return the error next time.
 	 */
-	if (ubelem)
+	if (ac.ac_ubelem)
 		rval = 0;
 	if (agno >= mp->m_sb.sb_agcount) {
 		/*
diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
index aaed080..6ea8b39 100644
--- a/fs/xfs/xfs_itable.h
+++ b/fs/xfs/xfs_itable.h
@@ -30,22 +30,6 @@ typedef int (*bulkstat_one_pf)(struct xfs_mount	*mp,
 			       int		*ubused,
 			       int		*stat);
 
-struct xfs_bulkstat_agichunk {
-	xfs_ino_t	ac_lastino;	/* last inode returned */
-	char		__user **ac_ubuffer;/* pointer into user's buffer */
-	int		ac_ubleft;	/* bytes left in user's buffer */
-	int		ac_ubelem;	/* spaces used in user's buffer */
-};
-
-int
-xfs_bulkstat_ag_ichunk(
-	struct xfs_mount		*mp,
-	xfs_agnumber_t			agno,
-	struct xfs_inobt_rec_incore	*irbp,
-	bulkstat_one_pf			formatter,
-	size_t				statstruct_size,
-	struct xfs_bulkstat_agichunk	*acp);
-
 /*
  * Values for stat return value.
  */
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/6] xfs: bulkstat chunk-formatter has issues

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The loop construct has issues:
	- clustidx is completely unused, so remove it.
	- the loop tries to be smart by terminating when the
	  "freecount" tells it that all inodes are free. Just drop
	  it as in most cases we have to scan all inodes in the
	  chunk anyway.
	- move the "user buffer left" condition check to the only
	  point where we consume space int eh user buffer.
	- move the initialisation of agino out of the loop, leaving
	  just a simple loop control logic using the clusteridx.

Also, double handling of the user buffer variables leads to problems
tracking the current state - use the cursor variables directly
rather than keeping local copies and then having to update the
cursor before returning.

cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++---------------------------------
 1 file changed, 21 insertions(+), 34 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 50a3e59..ff31965 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -283,59 +283,46 @@ xfs_bulkstat_ag_ichunk(
 	xfs_ino_t			*lastino)
 {
 	char				__user **ubufp = acp->ac_ubuffer;
-	int				ubleft = acp->ac_ubleft;
-	int				ubelem = acp->ac_ubelem;
-	int				chunkidx, clustidx;
+	int				chunkidx;
 	int				error = 0;
 	xfs_agino_t			agino;
 
-	for (agino = irbp->ir_startino, chunkidx = clustidx = 0;
-	     XFS_BULKSTAT_UBLEFT(ubleft) &&
-	     irbp->ir_freecount < XFS_INODES_PER_CHUNK;
-	     chunkidx++, clustidx++, agino++) {
-		int		fmterror;	/* bulkstat formatter result */
+	agino = irbp->ir_startino;
+	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
+	     chunkidx++, agino++) {
+		int		fmterror;
 		int		ubused;
 		xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, agno, agino);
 
-		ASSERT(chunkidx < XFS_INODES_PER_CHUNK);
-
 		/* Skip if this inode is free */
 		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
 			*lastino = ino;
 			continue;
 		}
 
-		/*
-		 * Count used inodes as free so we can tell when the
-		 * chunk is used up.
-		 */
-		irbp->ir_freecount++;
-
 		/* Get the inode and fill in a single buffer */
 		ubused = statstruct_size;
-		error = formatter(mp, ino, *ubufp, ubleft, &ubused, &fmterror);
-		if (fmterror == BULKSTAT_RV_NOTHING) {
-			if (error && error != -ENOENT && error != -EINVAL) {
-				ubleft = 0;
-				break;
-			}
-			*lastino = ino;
-			continue;
-		}
-		if (fmterror == BULKSTAT_RV_GIVEUP) {
-			ubleft = 0;
+		error = formatter(mp, ino, *ubufp, acp->ac_ubleft,
+				  &ubused, &fmterror);
+		if (fmterror == BULKSTAT_RV_GIVEUP ||
+		    (error && error != -ENOENT && error != -EINVAL)) {
+			acp->ac_ubleft = 0;
 			ASSERT(error);
 			break;
 		}
-		if (*ubufp)
-			*ubufp += ubused;
-		ubleft -= ubused;
-		ubelem++;
+		if (fmterror == BULKSTAT_RV_NOTHING || error) {
+			*lastino = ino;
+			continue;
+		}
+
+		*ubufp += ubused;
+		acp->ac_ubleft -= ubused;
+		acp->ac_ubelem++;
 		*lastino = ino;
-	}
 
-	acp->ac_ubleft = ubleft;
-	acp->ac_ubelem = ubelem;
+		if (acp->ac_ubleft < statstruct_size)
+			break;
+	}
 
 	return error;
 }
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/6] xfs: bulkstat main loop logic is a mess

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

There are a bunch of variables tha tare more wildy scoped than they
need to be, obfuscated user buffer checks and tortured "next inode"
tracking. This all needs cleaning up to expose the real issues that
need fixing.

cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index ff31965..fc4cf5d 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -345,30 +345,23 @@ xfs_bulkstat(
 	xfs_agino_t		agino;	/* inode # in allocation group */
 	xfs_agnumber_t		agno;	/* allocation group number */
 	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
-	int			end_of_ag; /* set if we've seen the ag end */
-	int			error;	/* error code */
-	int			icount;	/* count of inodes good in irbuf */
 	size_t			irbsize; /* size of irec buffer in bytes */
-	xfs_ino_t		ino;	/* inode number (filesystem) */
-	xfs_inobt_rec_incore_t	*irbp;	/* current irec buffer pointer */
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
-	xfs_inobt_rec_incore_t	*irbufend; /* end of good irec buffer entries */
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
 	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
-	int			stat;
 	struct xfs_bulkstat_agichunk ac;
+	int			error = 0;
 
 	/*
 	 * Get the last inode value, see if there's nothing to do.
 	 */
-	ino = (xfs_ino_t)*lastinop;
-	lastino = ino;
-	agno = XFS_INO_TO_AGNO(mp, ino);
-	agino = XFS_INO_TO_AGINO(mp, ino);
+	lastino = *lastinop;
+	agno = XFS_INO_TO_AGNO(mp, lastino);
+	agino = XFS_INO_TO_AGINO(mp, lastino);
 	if (agno >= mp->m_sb.sb_agcount ||
-	    ino != XFS_AGINO_TO_INO(mp, agno, agino)) {
+	    lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
 		*done = 1;
 		*ubcountp = 0;
 		return 0;
@@ -393,8 +386,13 @@ xfs_bulkstat(
 	 * inode returned; 0 means start of the allocation group.
 	 */
 	rval = 0;
-	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
-		cond_resched();
+	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
+		struct xfs_inobt_rec_incore	*irbp = irbuf;
+		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
+		bool				end_of_ag = false;
+		int				icount = 0;
+		int				stat;
+
 		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
 		if (error)
 			break;
@@ -404,10 +402,6 @@ xfs_bulkstat(
 		 */
 		cur = xfs_inobt_init_cursor(mp, NULL, agbp, agno,
 					    XFS_BTNUM_INO);
-		irbp = irbuf;
-		irbufend = irbuf + nirbuf;
-		end_of_ag = 0;
-		icount = 0;
 		if (agino > 0) {
 			/*
 			 * In the middle of an allocation group, we need to get
@@ -432,7 +426,7 @@ xfs_bulkstat(
 			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
 		}
 		if (error || stat == 0) {
-			end_of_ag = 1;
+			end_of_ag = true;
 			goto del_cursor;
 		}
 
@@ -445,7 +439,7 @@ xfs_bulkstat(
 
 			error = xfs_inobt_get_rec(cur, &r, &stat);
 			if (error || stat == 0) {
-				end_of_ag = 1;
+				end_of_ag = true;
 				goto del_cursor;
 			}
 
@@ -467,7 +461,7 @@ xfs_bulkstat(
 			agino = r.ir_startino + XFS_INODES_PER_CHUNK;
 			error = xfs_btree_increment(cur, 0, &stat);
 			if (error || stat == 0) {
-				end_of_ag = 1;
+				end_of_ag = true;
 				goto del_cursor;
 			}
 			cond_resched();
@@ -488,7 +482,7 @@ del_cursor:
 		 */
 		irbufend = irbp;
 		for (irbp = irbuf;
-		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
+		     irbp < irbufend && ac.ac_ubleft >= statstruct_size;
 		     irbp++) {
 			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
 					formatter, statstruct_size, &ac,
@@ -499,17 +493,14 @@ del_cursor:
 			cond_resched();
 		}
 
-		/*
-		 * Set up for the next loop iteration.
-		 */
-		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
-			if (end_of_ag) {
-				agno++;
-				agino = 0;
-			} else
-				agino = XFS_INO_TO_AGINO(mp, lastino);
-		} else
+		if (error)
 			break;
+
+		if (end_of_ag) {
+			agno++;
+			agino = 0;
+		} else
+			agino = XFS_INO_TO_AGINO(mp, lastino);
 	}
 	/*
 	 * Done, we're either out of filesystem or space to put the data.
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/6] xfs: bulkstat error handling is broken

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The error propagation is a horror - xfs_bulkstat() returns
a rval variable which is only set if there are formatter errors. Any
sort of btree walk error or corruption will cause the bulkstat walk
to terminate but will not pass an error back to userspace. Worse
is the fact that formatter errors will also be ignored if any inodes
were correctly formatted into the user buffer.

Hence bulkstat can fail badly yet still report success to userspace.
This causes significant issues with xfsdump not dumping everything
in the filesystem yet reporting success. It's not until a restore
fails that there is any indication that the dump was bad and tha
bulkstat failed. This patch now triggers xfsdump to fail with
bulkstat errors rather than silently missing files in the dump.

This now causes bulkstat to fail when the lastino cookie does not
fall inside an existing inode chunk. The pre-3.17 code tolerated
that error by allowing the code to move to the next inode chunk
as the agino target is guaranteed to fall into the next btree
record.

With the fixes up to this point in the series, xfsdump now passes on
the troublesome filesystem image that exposes all these bugs.

cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index fc4cf5d..6a4ef8e 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -236,8 +236,10 @@ xfs_bulkstat_grab_ichunk(
 	XFS_WANT_CORRUPTED_RETURN(stat == 1);
 
 	/* Check if the record contains the inode in request */
-	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
-		return -EINVAL;
+	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
+		*icount = 0;
+		return 0;
+	}
 
 	idx = agino - irec->ir_startino + 1;
 	if (idx < XFS_INODES_PER_CHUNK &&
@@ -349,7 +351,6 @@ xfs_bulkstat(
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
-	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
 	struct xfs_bulkstat_agichunk ac;
 	int			error = 0;
@@ -385,7 +386,6 @@ xfs_bulkstat(
 	 * Loop over the allocation groups, starting from the last
 	 * inode returned; 0 means start of the allocation group.
 	 */
-	rval = 0;
 	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
 		struct xfs_inobt_rec_incore	*irbp = irbuf;
 		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
@@ -488,7 +488,7 @@ del_cursor:
 					formatter, statstruct_size, &ac,
 					&lastino);
 			if (error)
-				rval = error;
+				break;
 
 			cond_resched();
 		}
@@ -507,11 +507,17 @@ del_cursor:
 	 */
 	kmem_free(irbuf);
 	*ubcountp = ac.ac_ubelem;
+
 	/*
-	 * Found some inodes, return them now and return the error next time.
+	 * We found some inodes, so clear the error status and return them.
+	 * The lastino pointer will point directly at the inode that triggered
+	 * any error that occurred, so on the next call the error will be
+	 * triggered again and propagated to userspace as there will be no
+	 * formatted inodes in the buffer.
 	 */
 	if (ac.ac_ubelem)
-		rval = 0;
+		error = 0;
+
 	if (agno >= mp->m_sb.sb_agcount) {
 		/*
 		 * If we ran out of filesystem, mark lastino as off
@@ -523,7 +529,7 @@ del_cursor:
 	} else
 		*lastinop = (xfs_ino_t)lastino;
 
-	return rval;
+	return error;
 }
 
 int
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/6] xfs: track bulkstat progress by agino

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The bulkstat main loop progress is tracked by the "lastino"
variable, which is a full 64 bit inode. However, the loop actually
works on agno/agino pairs, and so there's a significant disconnect
between the rest of the loop and the main cursor. Convert this to
use the agino, and pass the agino into the chunk formatting function
and convert it too.

This gets rid of the inconsistency in the loop processing, and
finally makes it simple for us to skip inodes at any point in the
loop simply by incrementing the agino cursor.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_itable.c | 50 +++++++++++++++++++-------------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index 6a4ef8e..2a6f2e8 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -282,45 +282,39 @@ xfs_bulkstat_ag_ichunk(
 	bulkstat_one_pf			formatter,
 	size_t				statstruct_size,
 	struct xfs_bulkstat_agichunk	*acp,
-	xfs_ino_t			*lastino)
+	xfs_agino_t			*last_agino)
 {
 	char				__user **ubufp = acp->ac_ubuffer;
 	int				chunkidx;
 	int				error = 0;
-	xfs_agino_t			agino;
 
-	agino = irbp->ir_startino;
+	*last_agino = irbp->ir_startino;
 	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
-	     chunkidx++, agino++) {
+	     chunkidx++, (*last_agino)++) {
 		int		fmterror;
 		int		ubused;
-		xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, agno, agino);
 
 		/* Skip if this inode is free */
-		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
-			*lastino = ino;
+		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free)
 			continue;
-		}
 
 		/* Get the inode and fill in a single buffer */
 		ubused = statstruct_size;
-		error = formatter(mp, ino, *ubufp, acp->ac_ubleft,
-				  &ubused, &fmterror);
+		error = formatter(mp, XFS_AGINO_TO_INO(mp, agno, *last_agino),
+				  *ubufp, acp->ac_ubleft, &ubused, &fmterror);
+
 		if (fmterror == BULKSTAT_RV_GIVEUP ||
 		    (error && error != -ENOENT && error != -EINVAL)) {
 			acp->ac_ubleft = 0;
 			ASSERT(error);
 			break;
 		}
-		if (fmterror == BULKSTAT_RV_NOTHING || error) {
-			*lastino = ino;
+		if (fmterror == BULKSTAT_RV_NOTHING || error)
 			continue;
-		}
 
 		*ubufp += ubused;
 		acp->ac_ubleft -= ubused;
 		acp->ac_ubelem++;
-		*lastino = ino;
 
 		if (acp->ac_ubleft < statstruct_size)
 			break;
@@ -349,7 +343,6 @@ xfs_bulkstat(
 	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
 	size_t			irbsize; /* size of irec buffer in bytes */
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
-	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
 	int			ubcount; /* size of user's buffer */
 	struct xfs_bulkstat_agichunk ac;
@@ -358,11 +351,10 @@ xfs_bulkstat(
 	/*
 	 * Get the last inode value, see if there's nothing to do.
 	 */
-	lastino = *lastinop;
-	agno = XFS_INO_TO_AGNO(mp, lastino);
-	agino = XFS_INO_TO_AGINO(mp, lastino);
+	agno = XFS_INO_TO_AGNO(mp, *lastinop);
+	agino = XFS_INO_TO_AGINO(mp, *lastinop);
 	if (agno >= mp->m_sb.sb_agcount ||
-	    lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
+	    *lastinop != XFS_AGINO_TO_INO(mp, agno, agino)) {
 		*done = 1;
 		*ubcountp = 0;
 		return 0;
@@ -486,7 +478,7 @@ del_cursor:
 		     irbp++) {
 			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
 					formatter, statstruct_size, &ac,
-					&lastino);
+					&agino);
 			if (error)
 				break;
 
@@ -499,8 +491,7 @@ del_cursor:
 		if (end_of_ag) {
 			agno++;
 			agino = 0;
-		} else
-			agino = XFS_INO_TO_AGINO(mp, lastino);
+		}
 	}
 	/*
 	 * Done, we're either out of filesystem or space to put the data.
@@ -518,16 +509,13 @@ del_cursor:
 	if (ac.ac_ubelem)
 		error = 0;
 
-	if (agno >= mp->m_sb.sb_agcount) {
-		/*
-		 * If we ran out of filesystem, mark lastino as off
-		 * the end of the filesystem, so the next call
-		 * will return immediately.
-		 */
-		*lastinop = (xfs_ino_t)XFS_AGINO_TO_INO(mp, agno, 0);
+	/*
+	 * If we ran out of filesystem, lastino will point off the end of
+	 * the filesystem so the next call will return immediately.
+	 */
+	*lastinop = XFS_AGINO_TO_INO(mp, agno, agino);
+	if (agno >= mp->m_sb.sb_agcount)
 		*done = 1;
-	} else
-		*lastinop = (xfs_ino_t)lastino;
 
 	return error;
 }
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/6] xfs: bulkstat btree walk doesn't terminate

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:16PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The bulkstat code has several different ways of detecting the end of
> an AG when doing a walk. They are not consistently detected, and the
> code that checks for the end of AG conditions is not consistently
> coded. Hence the are conditions where the walk code can get stuck in
> an endless loop making no progress and not triggering any
> termination conditions.
> 
> Convert all the "tmp/i" status return codes from btree operations
> to a common name (stat) and apply end-of-ag detection to these
> operations consistently.
> 
> cc: stable@vger.kernel.org
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Looks Ok...

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

>  fs/xfs/xfs_itable.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 7765ff7..16737cb 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -356,7 +356,6 @@ xfs_bulkstat(
>  	int			end_of_ag; /* set if we've seen the ag end */
>  	int			error;	/* error code */
>  	int                     fmterror;/* bulkstat formatter result */
> -	int			i;	/* loop index */
>  	int			icount;	/* count of inodes good in irbuf */
>  	size_t			irbsize; /* size of irec buffer in bytes */
>  	xfs_ino_t		ino;	/* inode number (filesystem) */
> @@ -366,11 +365,11 @@ xfs_bulkstat(
>  	xfs_ino_t		lastino; /* last inode number returned */
>  	int			nirbuf;	/* size of irbuf */
>  	int			rval;	/* return value error code */
> -	int			tmp;	/* result value from btree calls */
>  	int			ubcount; /* size of user's buffer */
>  	int			ubleft;	/* bytes left in user's buffer */
>  	char			__user *ubufp;	/* pointer into user's buffer */
>  	int			ubelem;	/* spaces used in user's buffer */
> +	int			stat;
>  
>  	/*
>  	 * Get the last inode value, see if there's nothing to do.
> @@ -436,13 +435,15 @@ xfs_bulkstat(
>  				agino = r.ir_startino + XFS_INODES_PER_CHUNK;
>  			}
>  			/* Increment to the next record */
> -			error = xfs_btree_increment(cur, 0, &tmp);
> +			error = xfs_btree_increment(cur, 0, &stat);
>  		} else {
>  			/* Start of ag.  Lookup the first inode chunk */
> -			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &tmp);
> +			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
>  		}
> -		if (error)
> +		if (error || stat == 0) {
> +			end_of_ag = 1;
>  			goto del_cursor;
> +		}
>  
>  		/*
>  		 * Loop through inode btree records in this ag,
> @@ -451,8 +452,8 @@ xfs_bulkstat(
>  		while (irbp < irbufend && icount < ubcount) {
>  			struct xfs_inobt_rec_incore	r;
>  
> -			error = xfs_inobt_get_rec(cur, &r, &i);
> -			if (error || i == 0) {
> +			error = xfs_inobt_get_rec(cur, &r, &stat);
> +			if (error || stat == 0) {
>  				end_of_ag = 1;
>  				goto del_cursor;
>  			}
> @@ -473,8 +474,8 @@ xfs_bulkstat(
>  			 * Set agino to after this chunk and bump the cursor.
>  			 */
>  			agino = r.ir_startino + XFS_INODES_PER_CHUNK;
> -			error = xfs_btree_increment(cur, 0, &tmp);
> -			if (error) {
> +			error = xfs_btree_increment(cur, 0, &stat);
> +			if (error || stat == 0) {
>  				end_of_ag = 1;
>  				goto del_cursor;
>  			}
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/6] xfs: bulkstat chunk formatting cursor is broken

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:17PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The xfs_bulkstat_agichunk formatting cursor takes buffer values from
> the main loop and passes them via the structure to the chunk
> formatter, and the writes the chnged values back into the main loop
> local variables. UNfortunately, this complex dance is full of corner
> cases that aren't handled correctly.
> 
> The biggest problem is that it is double handling the information in
> both the main loop and the chunk formatting function, leading to
> inconsistent updates and endless loops where progress is not made.
> 
> To fix this, push the struct xfs_bulkstat_agichunk outwards to be
> the primary holder of user buffer information. this removes the
> double handling in the main loop.
> 
> Also, pass the last inode processed by the chunk formatter as a
> separate parameter as it purely an output variable and is not
> related to the user buffer consumption cursor.
> 
> Finally, the chunk formatting code is not shared by anyone, so make
> it local to xfs_itable.c.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

The commit log suggests this patch is fixing some problem(s), but I see
mostly a refactor to pull up the ac structure and eliminate the variable
swapping between xfs_bulkstat() and xfs_bulkstat_ag_ichunk(). The
lastino separation doesn't appear to change semantics that I can tell
either. The refactor looks good to me:

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

... but is there something subtle in here I'm missing about the problem
being fixed? If not, could we update the commit title to suggest this is
a clean up?

Brian

>  fs/xfs/xfs_itable.c | 59 +++++++++++++++++++++++++----------------------------
>  fs/xfs/xfs_itable.h | 16 ---------------
>  2 files changed, 28 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 16737cb..50a3e59 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -262,20 +262,26 @@ xfs_bulkstat_grab_ichunk(
>  
>  #define XFS_BULKSTAT_UBLEFT(ubleft)	((ubleft) >= statstruct_size)
>  
> +struct xfs_bulkstat_agichunk {
> +	char		__user **ac_ubuffer;/* pointer into user's buffer */
> +	int		ac_ubleft;	/* bytes left in user's buffer */
> +	int		ac_ubelem;	/* spaces used in user's buffer */
> +};
> +
>  /*
>   * Process inodes in chunk with a pointer to a formatter function
>   * that will iget the inode and fill in the appropriate structure.
>   */
> -int
> +static int
>  xfs_bulkstat_ag_ichunk(
>  	struct xfs_mount		*mp,
>  	xfs_agnumber_t			agno,
>  	struct xfs_inobt_rec_incore	*irbp,
>  	bulkstat_one_pf			formatter,
>  	size_t				statstruct_size,
> -	struct xfs_bulkstat_agichunk	*acp)
> +	struct xfs_bulkstat_agichunk	*acp,
> +	xfs_ino_t			*lastino)
>  {
> -	xfs_ino_t			lastino = acp->ac_lastino;
>  	char				__user **ubufp = acp->ac_ubuffer;
>  	int				ubleft = acp->ac_ubleft;
>  	int				ubelem = acp->ac_ubelem;
> @@ -295,7 +301,7 @@ xfs_bulkstat_ag_ichunk(
>  
>  		/* Skip if this inode is free */
>  		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
> -			lastino = ino;
> +			*lastino = ino;
>  			continue;
>  		}
>  
> @@ -313,7 +319,7 @@ xfs_bulkstat_ag_ichunk(
>  				ubleft = 0;
>  				break;
>  			}
> -			lastino = ino;
> +			*lastino = ino;
>  			continue;
>  		}
>  		if (fmterror == BULKSTAT_RV_GIVEUP) {
> @@ -325,10 +331,9 @@ xfs_bulkstat_ag_ichunk(
>  			*ubufp += ubused;
>  		ubleft -= ubused;
>  		ubelem++;
> -		lastino = ino;
> +		*lastino = ino;
>  	}
>  
> -	acp->ac_lastino = lastino;
>  	acp->ac_ubleft = ubleft;
>  	acp->ac_ubelem = ubelem;
>  
> @@ -355,7 +360,6 @@ xfs_bulkstat(
>  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
>  	int			end_of_ag; /* set if we've seen the ag end */
>  	int			error;	/* error code */
> -	int                     fmterror;/* bulkstat formatter result */
>  	int			icount;	/* count of inodes good in irbuf */
>  	size_t			irbsize; /* size of irec buffer in bytes */
>  	xfs_ino_t		ino;	/* inode number (filesystem) */
> @@ -366,10 +370,8 @@ xfs_bulkstat(
>  	int			nirbuf;	/* size of irbuf */
>  	int			rval;	/* return value error code */
>  	int			ubcount; /* size of user's buffer */
> -	int			ubleft;	/* bytes left in user's buffer */
> -	char			__user *ubufp;	/* pointer into user's buffer */
> -	int			ubelem;	/* spaces used in user's buffer */
>  	int			stat;
> +	struct xfs_bulkstat_agichunk ac;
>  
>  	/*
>  	 * Get the last inode value, see if there's nothing to do.
> @@ -386,11 +388,13 @@ xfs_bulkstat(
>  	}
>  
>  	ubcount = *ubcountp; /* statstruct's */
> -	ubleft = ubcount * statstruct_size; /* bytes */
> -	*ubcountp = ubelem = 0;
> +	ac.ac_ubuffer = &ubuffer;
> +	ac.ac_ubleft = ubcount * statstruct_size; /* bytes */;
> +	ac.ac_ubelem = 0;
> +
> +	*ubcountp = 0;
>  	*done = 0;
> -	fmterror = 0;
> -	ubufp = ubuffer;
> +
>  	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
>  	if (!irbuf)
>  		return -ENOMEM;
> @@ -402,7 +406,7 @@ xfs_bulkstat(
>  	 * inode returned; 0 means start of the allocation group.
>  	 */
>  	rval = 0;
> -	while (XFS_BULKSTAT_UBLEFT(ubleft) && agno < mp->m_sb.sb_agcount) {
> +	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
>  		cond_resched();
>  		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
>  		if (error)
> @@ -497,28 +501,21 @@ del_cursor:
>  		 */
>  		irbufend = irbp;
>  		for (irbp = irbuf;
> -		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ubleft); irbp++) {
> -			struct xfs_bulkstat_agichunk ac;
> -
> -			ac.ac_lastino = lastino;
> -			ac.ac_ubuffer = &ubuffer;
> -			ac.ac_ubleft = ubleft;
> -			ac.ac_ubelem = ubelem;
> +		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> +		     irbp++) {
>  			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
> -					formatter, statstruct_size, &ac);
> +					formatter, statstruct_size, &ac,
> +					&lastino);
>  			if (error)
>  				rval = error;
>  
> -			lastino = ac.ac_lastino;
> -			ubleft = ac.ac_ubleft;
> -			ubelem = ac.ac_ubelem;
> -
>  			cond_resched();
>  		}
> +
>  		/*
>  		 * Set up for the next loop iteration.
>  		 */
> -		if (XFS_BULKSTAT_UBLEFT(ubleft)) {
> +		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
>  			if (end_of_ag) {
>  				agno++;
>  				agino = 0;
> @@ -531,11 +528,11 @@ del_cursor:
>  	 * Done, we're either out of filesystem or space to put the data.
>  	 */
>  	kmem_free(irbuf);
> -	*ubcountp = ubelem;
> +	*ubcountp = ac.ac_ubelem;
>  	/*
>  	 * Found some inodes, return them now and return the error next time.
>  	 */
> -	if (ubelem)
> +	if (ac.ac_ubelem)
>  		rval = 0;
>  	if (agno >= mp->m_sb.sb_agcount) {
>  		/*
> diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> index aaed080..6ea8b39 100644
> --- a/fs/xfs/xfs_itable.h
> +++ b/fs/xfs/xfs_itable.h
> @@ -30,22 +30,6 @@ typedef int (*bulkstat_one_pf)(struct xfs_mount	*mp,
>  			       int		*ubused,
>  			       int		*stat);
>  
> -struct xfs_bulkstat_agichunk {
> -	xfs_ino_t	ac_lastino;	/* last inode returned */
> -	char		__user **ac_ubuffer;/* pointer into user's buffer */
> -	int		ac_ubleft;	/* bytes left in user's buffer */
> -	int		ac_ubelem;	/* spaces used in user's buffer */
> -};
> -
> -int
> -xfs_bulkstat_ag_ichunk(
> -	struct xfs_mount		*mp,
> -	xfs_agnumber_t			agno,
> -	struct xfs_inobt_rec_incore	*irbp,
> -	bulkstat_one_pf			formatter,
> -	size_t				statstruct_size,
> -	struct xfs_bulkstat_agichunk	*acp);
> -
>  /*
>   * Values for stat return value.
>   */
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/6] xfs: bulkstat chunk-formatter has issues

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:18PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The loop construct has issues:
> 	- clustidx is completely unused, so remove it.
> 	- the loop tries to be smart by terminating when the
> 	  "freecount" tells it that all inodes are free. Just drop
> 	  it as in most cases we have to scan all inodes in the
> 	  chunk anyway.
> 	- move the "user buffer left" condition check to the only
> 	  point where we consume space int eh user buffer.
> 	- move the initialisation of agino out of the loop, leaving
> 	  just a simple loop control logic using the clusteridx.
> 
> Also, double handling of the user buffer variables leads to problems
> tracking the current state - use the cursor variables directly
> rather than keeping local copies and then having to update the
> cursor before returning.
> 
> cc: <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++---------------------------------
>  1 file changed, 21 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 50a3e59..ff31965 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -283,59 +283,46 @@ xfs_bulkstat_ag_ichunk(
>  	xfs_ino_t			*lastino)
>  {
>  	char				__user **ubufp = acp->ac_ubuffer;
> -	int				ubleft = acp->ac_ubleft;
> -	int				ubelem = acp->ac_ubelem;
> -	int				chunkidx, clustidx;
> +	int				chunkidx;
>  	int				error = 0;
>  	xfs_agino_t			agino;
>  
> -	for (agino = irbp->ir_startino, chunkidx = clustidx = 0;
> -	     XFS_BULKSTAT_UBLEFT(ubleft) &&
> -	     irbp->ir_freecount < XFS_INODES_PER_CHUNK;
> -	     chunkidx++, clustidx++, agino++) {
> -		int		fmterror;	/* bulkstat formatter result */
> +	agino = irbp->ir_startino;
> +	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
> +	     chunkidx++, agino++) {
> +		int		fmterror;
>  		int		ubused;
>  		xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, agno, agino);
>  
> -		ASSERT(chunkidx < XFS_INODES_PER_CHUNK);
> -
>  		/* Skip if this inode is free */
>  		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
>  			*lastino = ino;
>  			continue;
>  		}
>  
> -		/*
> -		 * Count used inodes as free so we can tell when the
> -		 * chunk is used up.
> -		 */
> -		irbp->ir_freecount++;
> -
>  		/* Get the inode and fill in a single buffer */
>  		ubused = statstruct_size;
> -		error = formatter(mp, ino, *ubufp, ubleft, &ubused, &fmterror);
> -		if (fmterror == BULKSTAT_RV_NOTHING) {
> -			if (error && error != -ENOENT && error != -EINVAL) {
> -				ubleft = 0;
> -				break;
> -			}
> -			*lastino = ino;
> -			continue;
> -		}
> -		if (fmterror == BULKSTAT_RV_GIVEUP) {
> -			ubleft = 0;
> +		error = formatter(mp, ino, *ubufp, acp->ac_ubleft,
> +				  &ubused, &fmterror);
> +		if (fmterror == BULKSTAT_RV_GIVEUP ||
> +		    (error && error != -ENOENT && error != -EINVAL)) {
> +			acp->ac_ubleft = 0;
>  			ASSERT(error);
>  			break;
>  		}
> -		if (*ubufp)
> -			*ubufp += ubused;
> -		ubleft -= ubused;
> -		ubelem++;
> +		if (fmterror == BULKSTAT_RV_NOTHING || error) {
> +			*lastino = ino;
> +			continue;
> +		}

Not introduced by this patch, but this looks like inconsistent error
handling. The structure of the code suggests we intend to carry on in
the event of ENOENT or EINVAL, but said errors can leak out of this
function if we never get back to the formatter call (e.g., we're at the
last inode or all subsequent inodes are free).

Brian

> +
> +		*ubufp += ubused;
> +		acp->ac_ubleft -= ubused;
> +		acp->ac_ubelem++;
>  		*lastino = ino;
> -	}
>  
> -	acp->ac_ubleft = ubleft;
> -	acp->ac_ubelem = ubelem;
> +		if (acp->ac_ubleft < statstruct_size)
> +			break;
> +	}
>  
>  	return error;
>  }
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: bulkstat main loop logic is a mess

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:19PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> There are a bunch of variables tha tare more wildy scoped than they
> need to be, obfuscated user buffer checks and tortured "next inode"
> tracking. This all needs cleaning up to expose the real issues that
> need fixing.
> 
> cc: <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index ff31965..fc4cf5d 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -345,30 +345,23 @@ xfs_bulkstat(
>  	xfs_agino_t		agino;	/* inode # in allocation group */
>  	xfs_agnumber_t		agno;	/* allocation group number */
>  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
> -	int			end_of_ag; /* set if we've seen the ag end */
> -	int			error;	/* error code */
> -	int			icount;	/* count of inodes good in irbuf */
>  	size_t			irbsize; /* size of irec buffer in bytes */
> -	xfs_ino_t		ino;	/* inode number (filesystem) */
> -	xfs_inobt_rec_incore_t	*irbp;	/* current irec buffer pointer */
>  	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
> -	xfs_inobt_rec_incore_t	*irbufend; /* end of good irec buffer entries */
>  	xfs_ino_t		lastino; /* last inode number returned */
>  	int			nirbuf;	/* size of irbuf */
>  	int			rval;	/* return value error code */
>  	int			ubcount; /* size of user's buffer */
> -	int			stat;
>  	struct xfs_bulkstat_agichunk ac;
> +	int			error = 0;
>  
>  	/*
>  	 * Get the last inode value, see if there's nothing to do.
>  	 */
> -	ino = (xfs_ino_t)*lastinop;
> -	lastino = ino;
> -	agno = XFS_INO_TO_AGNO(mp, ino);
> -	agino = XFS_INO_TO_AGINO(mp, ino);
> +	lastino = *lastinop;
> +	agno = XFS_INO_TO_AGNO(mp, lastino);
> +	agino = XFS_INO_TO_AGINO(mp, lastino);
>  	if (agno >= mp->m_sb.sb_agcount ||
> -	    ino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> +	    lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
>  		*done = 1;
>  		*ubcountp = 0;
>  		return 0;
> @@ -393,8 +386,13 @@ xfs_bulkstat(
>  	 * inode returned; 0 means start of the allocation group.
>  	 */
>  	rval = 0;
> -	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
> -		cond_resched();
> +	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
> +		struct xfs_inobt_rec_incore	*irbp = irbuf;
> +		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
> +		bool				end_of_ag = false;
> +		int				icount = 0;
> +		int				stat;
> +
>  		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
>  		if (error)
>  			break;
> @@ -404,10 +402,6 @@ xfs_bulkstat(
>  		 */
>  		cur = xfs_inobt_init_cursor(mp, NULL, agbp, agno,
>  					    XFS_BTNUM_INO);
> -		irbp = irbuf;
> -		irbufend = irbuf + nirbuf;
> -		end_of_ag = 0;
> -		icount = 0;
>  		if (agino > 0) {
>  			/*
>  			 * In the middle of an allocation group, we need to get
> @@ -432,7 +426,7 @@ xfs_bulkstat(
>  			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
>  		}
>  		if (error || stat == 0) {
> -			end_of_ag = 1;
> +			end_of_ag = true;
>  			goto del_cursor;
>  		}
>  
> @@ -445,7 +439,7 @@ xfs_bulkstat(
>  
>  			error = xfs_inobt_get_rec(cur, &r, &stat);
>  			if (error || stat == 0) {
> -				end_of_ag = 1;
> +				end_of_ag = true;
>  				goto del_cursor;
>  			}
>  
> @@ -467,7 +461,7 @@ xfs_bulkstat(
>  			agino = r.ir_startino + XFS_INODES_PER_CHUNK;
>  			error = xfs_btree_increment(cur, 0, &stat);
>  			if (error || stat == 0) {
> -				end_of_ag = 1;
> +				end_of_ag = true;
>  				goto del_cursor;
>  			}
>  			cond_resched();
> @@ -488,7 +482,7 @@ del_cursor:
>  		 */
>  		irbufend = irbp;
>  		for (irbp = irbuf;
> -		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> +		     irbp < irbufend && ac.ac_ubleft >= statstruct_size;
>  		     irbp++) {
>  			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
>  					formatter, statstruct_size, &ac,
> @@ -499,17 +493,14 @@ del_cursor:
>  			cond_resched();
>  		}
>  
> -		/*
> -		 * Set up for the next loop iteration.
> -		 */
> -		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
> -			if (end_of_ag) {
> -				agno++;
> -				agino = 0;
> -			} else
> -				agino = XFS_INO_TO_AGINO(mp, lastino);
> -		} else
> +		if (error)
>  			break;

This stood out to me as potentially broken as it introduces a
non-deterministic error path. The preceding loop can overwrite error
since it iterates until either all records are handled or the buffer is
consumed.

That said, this is all fixed up by the next patch and the rest looks
fine:

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

... though I think the error check should technically be introduced by
the next patch.

Brian

> +
> +		if (end_of_ag) {
> +			agno++;
> +			agino = 0;
> +		} else
> +			agino = XFS_INO_TO_AGINO(mp, lastino);
>  	}
>  	/*
>  	 * Done, we're either out of filesystem or space to put the data.
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfs: bulkstat error handling is broken

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The error propagation is a horror - xfs_bulkstat() returns
> a rval variable which is only set if there are formatter errors. Any
> sort of btree walk error or corruption will cause the bulkstat walk
> to terminate but will not pass an error back to userspace. Worse
> is the fact that formatter errors will also be ignored if any inodes
> were correctly formatted into the user buffer.
> 
> Hence bulkstat can fail badly yet still report success to userspace.
> This causes significant issues with xfsdump not dumping everything
> in the filesystem yet reporting success. It's not until a restore
> fails that there is any indication that the dump was bad and tha
> bulkstat failed. This patch now triggers xfsdump to fail with
> bulkstat errors rather than silently missing files in the dump.
> 
> This now causes bulkstat to fail when the lastino cookie does not
> fall inside an existing inode chunk. The pre-3.17 code tolerated
> that error by allowing the code to move to the next inode chunk
> as the agino target is guaranteed to fall into the next btree
> record.
> 
> With the fixes up to this point in the series, xfsdump now passes on
> the troublesome filesystem image that exposes all these bugs.
> 
> cc: <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

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

>  fs/xfs/xfs_itable.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index fc4cf5d..6a4ef8e 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -236,8 +236,10 @@ xfs_bulkstat_grab_ichunk(
>  	XFS_WANT_CORRUPTED_RETURN(stat == 1);
>  
>  	/* Check if the record contains the inode in request */
> -	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
> -		return -EINVAL;
> +	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
> +		*icount = 0;
> +		return 0;
> +	}
>  
>  	idx = agino - irec->ir_startino + 1;
>  	if (idx < XFS_INODES_PER_CHUNK &&
> @@ -349,7 +351,6 @@ xfs_bulkstat(
>  	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
>  	xfs_ino_t		lastino; /* last inode number returned */
>  	int			nirbuf;	/* size of irbuf */
> -	int			rval;	/* return value error code */
>  	int			ubcount; /* size of user's buffer */
>  	struct xfs_bulkstat_agichunk ac;
>  	int			error = 0;
> @@ -385,7 +386,6 @@ xfs_bulkstat(
>  	 * Loop over the allocation groups, starting from the last
>  	 * inode returned; 0 means start of the allocation group.
>  	 */
> -	rval = 0;
>  	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
>  		struct xfs_inobt_rec_incore	*irbp = irbuf;
>  		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
> @@ -488,7 +488,7 @@ del_cursor:
>  					formatter, statstruct_size, &ac,
>  					&lastino);
>  			if (error)
> -				rval = error;
> +				break;
>  
>  			cond_resched();
>  		}
> @@ -507,11 +507,17 @@ del_cursor:
>  	 */
>  	kmem_free(irbuf);
>  	*ubcountp = ac.ac_ubelem;
> +
>  	/*
> -	 * Found some inodes, return them now and return the error next time.
> +	 * We found some inodes, so clear the error status and return them.
> +	 * The lastino pointer will point directly at the inode that triggered
> +	 * any error that occurred, so on the next call the error will be
> +	 * triggered again and propagated to userspace as there will be no
> +	 * formatted inodes in the buffer.
>  	 */
>  	if (ac.ac_ubelem)
> -		rval = 0;
> +		error = 0;
> +
>  	if (agno >= mp->m_sb.sb_agcount) {
>  		/*
>  		 * If we ran out of filesystem, mark lastino as off
> @@ -523,7 +529,7 @@ del_cursor:
>  	} else
>  		*lastinop = (xfs_ino_t)lastino;
>  
> -	return rval;
> +	return error;
>  }
>  
>  int
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 6/6] xfs: track bulkstat progress by agino

[Brian Foster]

On Tue, Nov 04, 2014 at 11:53:21PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The bulkstat main loop progress is tracked by the "lastino"
> variable, which is a full 64 bit inode. However, the loop actually
> works on agno/agino pairs, and so there's a significant disconnect
> between the rest of the loop and the main cursor. Convert this to
> use the agino, and pass the agino into the chunk formatting function
> and convert it too.
> 
> This gets rid of the inconsistency in the loop processing, and
> finally makes it simple for us to skip inodes at any point in the
> loop simply by incrementing the agino cursor.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_itable.c | 50 +++++++++++++++++++-------------------------------
>  1 file changed, 19 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 6a4ef8e..2a6f2e8 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -282,45 +282,39 @@ xfs_bulkstat_ag_ichunk(
>  	bulkstat_one_pf			formatter,
>  	size_t				statstruct_size,
>  	struct xfs_bulkstat_agichunk	*acp,
> -	xfs_ino_t			*lastino)
> +	xfs_agino_t			*last_agino)
>  {
>  	char				__user **ubufp = acp->ac_ubuffer;
>  	int				chunkidx;
>  	int				error = 0;
> -	xfs_agino_t			agino;
>  
> -	agino = irbp->ir_startino;
> +	*last_agino = irbp->ir_startino;

Shouldn't last_agino refer to the last inode written out to the
userspace buffer? I'm not familiar with the semantics that xfsdump
expects or works around here as you've discussed in the commit logs and
whatnot. What happens if we set last_agino here and error out in the
first iteration of the loop?

>  	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
> -	     chunkidx++, agino++) {
> +	     chunkidx++, (*last_agino)++) {

Similar question here when we handle a full record and increment
*last_agino beyond the last agino in the record. Unless I'm missing
something, it looks to me that if the next record is adjacent,
xfs_bulkstat_grab_ichunk() would skip the first inode in that record.

>  		int		fmterror;
>  		int		ubused;
> -		xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, agno, agino);
>  
>  		/* Skip if this inode is free */
> -		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
> -			*lastino = ino;
> +		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free)
>  			continue;
> -		}
>  
>  		/* Get the inode and fill in a single buffer */
>  		ubused = statstruct_size;
> -		error = formatter(mp, ino, *ubufp, acp->ac_ubleft,
> -				  &ubused, &fmterror);
> +		error = formatter(mp, XFS_AGINO_TO_INO(mp, agno, *last_agino),
> +				  *ubufp, acp->ac_ubleft, &ubused, &fmterror);
> +
>  		if (fmterror == BULKSTAT_RV_GIVEUP ||
>  		    (error && error != -ENOENT && error != -EINVAL)) {
>  			acp->ac_ubleft = 0;
>  			ASSERT(error);
>  			break;
>  		}
> -		if (fmterror == BULKSTAT_RV_NOTHING || error) {
> -			*lastino = ino;
> +		if (fmterror == BULKSTAT_RV_NOTHING || error)
>  			continue;
> -		}
>  
>  		*ubufp += ubused;
>  		acp->ac_ubleft -= ubused;
>  		acp->ac_ubelem++;
> -		*lastino = ino;
>  
>  		if (acp->ac_ubleft < statstruct_size)
>  			break;
> @@ -349,7 +343,6 @@ xfs_bulkstat(
>  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
>  	size_t			irbsize; /* size of irec buffer in bytes */
>  	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
> -	xfs_ino_t		lastino; /* last inode number returned */
>  	int			nirbuf;	/* size of irbuf */
>  	int			ubcount; /* size of user's buffer */
>  	struct xfs_bulkstat_agichunk ac;
> @@ -358,11 +351,10 @@ xfs_bulkstat(
>  	/*
>  	 * Get the last inode value, see if there's nothing to do.
>  	 */
> -	lastino = *lastinop;
> -	agno = XFS_INO_TO_AGNO(mp, lastino);
> -	agino = XFS_INO_TO_AGINO(mp, lastino);
> +	agno = XFS_INO_TO_AGNO(mp, *lastinop);
> +	agino = XFS_INO_TO_AGINO(mp, *lastinop);
>  	if (agno >= mp->m_sb.sb_agcount ||
> -	    lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> +	    *lastinop != XFS_AGINO_TO_INO(mp, agno, agino)) {
>  		*done = 1;
>  		*ubcountp = 0;
>  		return 0;
> @@ -486,7 +478,7 @@ del_cursor:
>  		     irbp++) {
>  			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
>  					formatter, statstruct_size, &ac,
> -					&lastino);
> +					&agino);
>  			if (error)
>  				break;
>  
> @@ -499,8 +491,7 @@ del_cursor:
>  		if (end_of_ag) {
>  			agno++;
>  			agino = 0;
> -		} else
> -			agino = XFS_INO_TO_AGINO(mp, lastino);
> +		}
>  	}
>  	/*
>  	 * Done, we're either out of filesystem or space to put the data.
> @@ -518,16 +509,13 @@ del_cursor:
>  	if (ac.ac_ubelem)
>  		error = 0;
>  
> -	if (agno >= mp->m_sb.sb_agcount) {
> -		/*
> -		 * If we ran out of filesystem, mark lastino as off
> -		 * the end of the filesystem, so the next call
> -		 * will return immediately.
> -		 */
> -		*lastinop = (xfs_ino_t)XFS_AGINO_TO_INO(mp, agno, 0);
> +	/*
> +	 * If we ran out of filesystem, lastino will point off the end of
> +	 * the filesystem so the next call will return immediately.
> +	 */
> +	*lastinop = XFS_AGINO_TO_INO(mp, agno, agino);

This means that the main loop should never move agino forward unless the
inodes up through agino are guaranteed to be copied out to the user
buffer. There are two places in the main loop where we do this:

	agino = r.ir_startino + XFS_INODES_PER_CHUNK;

... and both of those lines execute before a potential error path that
breaks out of the main loop before that current record is formatted out.
Those lines appear to be spurious at this point, however, so I suspect
we can just kill them.

Brian

> +	if (agno >= mp->m_sb.sb_agcount)
>  		*done = 1;
> -	} else
> -		*lastinop = (xfs_ino_t)lastino;
>  
>  	return error;
>  }
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/6] xfs: bulkstat chunk formatting cursor is broken

[Dave Chinner]

On Tue, Nov 04, 2014 at 01:58:32PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:17PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The xfs_bulkstat_agichunk formatting cursor takes buffer values from
> > the main loop and passes them via the structure to the chunk
> > formatter, and the writes the chnged values back into the main loop
> > local variables. UNfortunately, this complex dance is full of corner
> > cases that aren't handled correctly.
> > 
> > The biggest problem is that it is double handling the information in
> > both the main loop and the chunk formatting function, leading to
> > inconsistent updates and endless loops where progress is not made.
> > 
> > To fix this, push the struct xfs_bulkstat_agichunk outwards to be
> > the primary holder of user buffer information. this removes the
> > double handling in the main loop.
> > 
> > Also, pass the last inode processed by the chunk formatter as a
> > separate parameter as it purely an output variable and is not
> > related to the user buffer consumption cursor.
> > 
> > Finally, the chunk formatting code is not shared by anyone, so make
> > it local to xfs_itable.c.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> 
> The commit log suggests this patch is fixing some problem(s), but I see
> mostly a refactor to pull up the ac structure and eliminate the variable
> swapping between xfs_bulkstat() and xfs_bulkstat_ag_ichunk(). The
> lastino separation doesn't appear to change semantics that I can tell
> either. The refactor looks good to me:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> ... but is there something subtle in here I'm missing about the problem
> being fixed? If not, could we update the commit title to suggest this is
> a clean up?

There is something subtle in this, but I didn't dig into it a great
deal.

Essentially, I had one big patch that changed everythign related to
the user buffer handling, but I felt it was too large to easily
review so I split it into two - one for the mainloop and one for the
chunk formatter. If I drop either of the two patches then bulkstat
hangs up in an endless loop. Neither of the two patches look to fix
obvious problems, but they do change behaviour.

So yes, there are subtle interactions with the rest of the main loop
state here, but yesterday afternoon I really didn't care much about
exactly what bugs the split patches were fixing, just that after
them it all worked correctly.

Cheers,

Dave.


> 
> Brian
> 
> >  fs/xfs/xfs_itable.c | 59 +++++++++++++++++++++++++----------------------------
> >  fs/xfs/xfs_itable.h | 16 ---------------
> >  2 files changed, 28 insertions(+), 47 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 16737cb..50a3e59 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -262,20 +262,26 @@ xfs_bulkstat_grab_ichunk(
> >  
> >  #define XFS_BULKSTAT_UBLEFT(ubleft)	((ubleft) >= statstruct_size)
> >  
> > +struct xfs_bulkstat_agichunk {
> > +	char		__user **ac_ubuffer;/* pointer into user's buffer */
> > +	int		ac_ubleft;	/* bytes left in user's buffer */
> > +	int		ac_ubelem;	/* spaces used in user's buffer */
> > +};
> > +
> >  /*
> >   * Process inodes in chunk with a pointer to a formatter function
> >   * that will iget the inode and fill in the appropriate structure.
> >   */
> > -int
> > +static int
> >  xfs_bulkstat_ag_ichunk(
> >  	struct xfs_mount		*mp,
> >  	xfs_agnumber_t			agno,
> >  	struct xfs_inobt_rec_incore	*irbp,
> >  	bulkstat_one_pf			formatter,
> >  	size_t				statstruct_size,
> > -	struct xfs_bulkstat_agichunk	*acp)
> > +	struct xfs_bulkstat_agichunk	*acp,
> > +	xfs_ino_t			*lastino)
> >  {
> > -	xfs_ino_t			lastino = acp->ac_lastino;
> >  	char				__user **ubufp = acp->ac_ubuffer;
> >  	int				ubleft = acp->ac_ubleft;
> >  	int				ubelem = acp->ac_ubelem;
> > @@ -295,7 +301,7 @@ xfs_bulkstat_ag_ichunk(
> >  
> >  		/* Skip if this inode is free */
> >  		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
> > -			lastino = ino;
> > +			*lastino = ino;
> >  			continue;
> >  		}
> >  
> > @@ -313,7 +319,7 @@ xfs_bulkstat_ag_ichunk(
> >  				ubleft = 0;
> >  				break;
> >  			}
> > -			lastino = ino;
> > +			*lastino = ino;
> >  			continue;
> >  		}
> >  		if (fmterror == BULKSTAT_RV_GIVEUP) {
> > @@ -325,10 +331,9 @@ xfs_bulkstat_ag_ichunk(
> >  			*ubufp += ubused;
> >  		ubleft -= ubused;
> >  		ubelem++;
> > -		lastino = ino;
> > +		*lastino = ino;
> >  	}
> >  
> > -	acp->ac_lastino = lastino;
> >  	acp->ac_ubleft = ubleft;
> >  	acp->ac_ubelem = ubelem;
> >  
> > @@ -355,7 +360,6 @@ xfs_bulkstat(
> >  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
> >  	int			end_of_ag; /* set if we've seen the ag end */
> >  	int			error;	/* error code */
> > -	int                     fmterror;/* bulkstat formatter result */
> >  	int			icount;	/* count of inodes good in irbuf */
> >  	size_t			irbsize; /* size of irec buffer in bytes */
> >  	xfs_ino_t		ino;	/* inode number (filesystem) */
> > @@ -366,10 +370,8 @@ xfs_bulkstat(
> >  	int			nirbuf;	/* size of irbuf */
> >  	int			rval;	/* return value error code */
> >  	int			ubcount; /* size of user's buffer */
> > -	int			ubleft;	/* bytes left in user's buffer */
> > -	char			__user *ubufp;	/* pointer into user's buffer */
> > -	int			ubelem;	/* spaces used in user's buffer */
> >  	int			stat;
> > +	struct xfs_bulkstat_agichunk ac;
> >  
> >  	/*
> >  	 * Get the last inode value, see if there's nothing to do.
> > @@ -386,11 +388,13 @@ xfs_bulkstat(
> >  	}
> >  
> >  	ubcount = *ubcountp; /* statstruct's */
> > -	ubleft = ubcount * statstruct_size; /* bytes */
> > -	*ubcountp = ubelem = 0;
> > +	ac.ac_ubuffer = &ubuffer;
> > +	ac.ac_ubleft = ubcount * statstruct_size; /* bytes */;
> > +	ac.ac_ubelem = 0;
> > +
> > +	*ubcountp = 0;
> >  	*done = 0;
> > -	fmterror = 0;
> > -	ubufp = ubuffer;
> > +
> >  	irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
> >  	if (!irbuf)
> >  		return -ENOMEM;
> > @@ -402,7 +406,7 @@ xfs_bulkstat(
> >  	 * inode returned; 0 means start of the allocation group.
> >  	 */
> >  	rval = 0;
> > -	while (XFS_BULKSTAT_UBLEFT(ubleft) && agno < mp->m_sb.sb_agcount) {
> > +	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
> >  		cond_resched();
> >  		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
> >  		if (error)
> > @@ -497,28 +501,21 @@ del_cursor:
> >  		 */
> >  		irbufend = irbp;
> >  		for (irbp = irbuf;
> > -		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ubleft); irbp++) {
> > -			struct xfs_bulkstat_agichunk ac;
> > -
> > -			ac.ac_lastino = lastino;
> > -			ac.ac_ubuffer = &ubuffer;
> > -			ac.ac_ubleft = ubleft;
> > -			ac.ac_ubelem = ubelem;
> > +		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> > +		     irbp++) {
> >  			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
> > -					formatter, statstruct_size, &ac);
> > +					formatter, statstruct_size, &ac,
> > +					&lastino);
> >  			if (error)
> >  				rval = error;
> >  
> > -			lastino = ac.ac_lastino;
> > -			ubleft = ac.ac_ubleft;
> > -			ubelem = ac.ac_ubelem;
> > -
> >  			cond_resched();
> >  		}
> > +
> >  		/*
> >  		 * Set up for the next loop iteration.
> >  		 */
> > -		if (XFS_BULKSTAT_UBLEFT(ubleft)) {
> > +		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
> >  			if (end_of_ag) {
> >  				agno++;
> >  				agino = 0;
> > @@ -531,11 +528,11 @@ del_cursor:
> >  	 * Done, we're either out of filesystem or space to put the data.
> >  	 */
> >  	kmem_free(irbuf);
> > -	*ubcountp = ubelem;
> > +	*ubcountp = ac.ac_ubelem;
> >  	/*
> >  	 * Found some inodes, return them now and return the error next time.
> >  	 */
> > -	if (ubelem)
> > +	if (ac.ac_ubelem)
> >  		rval = 0;
> >  	if (agno >= mp->m_sb.sb_agcount) {
> >  		/*
> > diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> > index aaed080..6ea8b39 100644
> > --- a/fs/xfs/xfs_itable.h
> > +++ b/fs/xfs/xfs_itable.h
> > @@ -30,22 +30,6 @@ typedef int (*bulkstat_one_pf)(struct xfs_mount	*mp,
> >  			       int		*ubused,
> >  			       int		*stat);
> >  
> > -struct xfs_bulkstat_agichunk {
> > -	xfs_ino_t	ac_lastino;	/* last inode returned */
> > -	char		__user **ac_ubuffer;/* pointer into user's buffer */
> > -	int		ac_ubleft;	/* bytes left in user's buffer */
> > -	int		ac_ubelem;	/* spaces used in user's buffer */
> > -};
> > -
> > -int
> > -xfs_bulkstat_ag_ichunk(
> > -	struct xfs_mount		*mp,
> > -	xfs_agnumber_t			agno,
> > -	struct xfs_inobt_rec_incore	*irbp,
> > -	bulkstat_one_pf			formatter,
> > -	size_t				statstruct_size,
> > -	struct xfs_bulkstat_agichunk	*acp);
> > -
> >  /*
> >   * Values for stat return value.
> >   */
> > -- 
> > 2.0.0
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/6] xfs: bulkstat chunk-formatter has issues

[Dave Chinner]

On Tue, Nov 04, 2014 at 01:58:49PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:18PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The loop construct has issues:
> > 	- clustidx is completely unused, so remove it.
> > 	- the loop tries to be smart by terminating when the
> > 	  "freecount" tells it that all inodes are free. Just drop
> > 	  it as in most cases we have to scan all inodes in the
> > 	  chunk anyway.
> > 	- move the "user buffer left" condition check to the only
> > 	  point where we consume space int eh user buffer.
> > 	- move the initialisation of agino out of the loop, leaving
> > 	  just a simple loop control logic using the clusteridx.
> > 
> > Also, double handling of the user buffer variables leads to problems
> > tracking the current state - use the cursor variables directly
> > rather than keeping local copies and then having to update the
> > cursor before returning.
> > 
> > cc: <stable@vger.kernel.org>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++---------------------------------
> >  1 file changed, 21 insertions(+), 34 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 50a3e59..ff31965 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -283,59 +283,46 @@ xfs_bulkstat_ag_ichunk(
> >  	xfs_ino_t			*lastino)
> >  {
> >  	char				__user **ubufp = acp->ac_ubuffer;
> > -	int				ubleft = acp->ac_ubleft;
> > -	int				ubelem = acp->ac_ubelem;
> > -	int				chunkidx, clustidx;
> > +	int				chunkidx;
> >  	int				error = 0;
> >  	xfs_agino_t			agino;
> >  
> > -	for (agino = irbp->ir_startino, chunkidx = clustidx = 0;
> > -	     XFS_BULKSTAT_UBLEFT(ubleft) &&
> > -	     irbp->ir_freecount < XFS_INODES_PER_CHUNK;
> > -	     chunkidx++, clustidx++, agino++) {
> > -		int		fmterror;	/* bulkstat formatter result */
> > +	agino = irbp->ir_startino;
> > +	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
> > +	     chunkidx++, agino++) {
> > +		int		fmterror;
> >  		int		ubused;
> >  		xfs_ino_t	ino = XFS_AGINO_TO_INO(mp, agno, agino);
> >  
> > -		ASSERT(chunkidx < XFS_INODES_PER_CHUNK);
> > -
> >  		/* Skip if this inode is free */
> >  		if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
> >  			*lastino = ino;
> >  			continue;
> >  		}
> >  
> > -		/*
> > -		 * Count used inodes as free so we can tell when the
> > -		 * chunk is used up.
> > -		 */
> > -		irbp->ir_freecount++;
> > -
> >  		/* Get the inode and fill in a single buffer */
> >  		ubused = statstruct_size;
> > -		error = formatter(mp, ino, *ubufp, ubleft, &ubused, &fmterror);
> > -		if (fmterror == BULKSTAT_RV_NOTHING) {
> > -			if (error && error != -ENOENT && error != -EINVAL) {
> > -				ubleft = 0;
> > -				break;
> > -			}
> > -			*lastino = ino;
> > -			continue;
> > -		}
> > -		if (fmterror == BULKSTAT_RV_GIVEUP) {
> > -			ubleft = 0;
> > +		error = formatter(mp, ino, *ubufp, acp->ac_ubleft,
> > +				  &ubused, &fmterror);
> > +		if (fmterror == BULKSTAT_RV_GIVEUP ||
> > +		    (error && error != -ENOENT && error != -EINVAL)) {
> > +			acp->ac_ubleft = 0;
> >  			ASSERT(error);
> >  			break;
> >  		}
> > -		if (*ubufp)
> > -			*ubufp += ubused;
> > -		ubleft -= ubused;
> > -		ubelem++;
> > +		if (fmterror == BULKSTAT_RV_NOTHING || error) {
> > +			*lastino = ino;
> > +			continue;
> > +		}
> 
> Not introduced by this patch, but this looks like inconsistent error
> handling. The structure of the code suggests we intend to carry on in
> the event of ENOENT or EINVAL, but said errors can leak out of this
> function if we never get back to the formatter call (e.g., we're at the
> last inode or all subsequent inodes are free).

Good catch - I'll fix it. I'll just zero the error in that branch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfs: bulkstat main loop logic is a mess

[Dave Chinner]

On Tue, Nov 04, 2014 at 01:59:08PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:19PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > There are a bunch of variables tha tare more wildy scoped than they
> > need to be, obfuscated user buffer checks and tortured "next inode"
> > tracking. This all needs cleaning up to expose the real issues that
> > need fixing.
> > 
> > cc: <stable@vger.kernel.org>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++++-------------------------------
> >  1 file changed, 23 insertions(+), 32 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index ff31965..fc4cf5d 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -345,30 +345,23 @@ xfs_bulkstat(
> >  	xfs_agino_t		agino;	/* inode # in allocation group */
> >  	xfs_agnumber_t		agno;	/* allocation group number */
> >  	xfs_btree_cur_t		*cur;	/* btree cursor for ialloc btree */
> > -	int			end_of_ag; /* set if we've seen the ag end */
> > -	int			error;	/* error code */
> > -	int			icount;	/* count of inodes good in irbuf */
> >  	size_t			irbsize; /* size of irec buffer in bytes */
> > -	xfs_ino_t		ino;	/* inode number (filesystem) */
> > -	xfs_inobt_rec_incore_t	*irbp;	/* current irec buffer pointer */
> >  	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
> > -	xfs_inobt_rec_incore_t	*irbufend; /* end of good irec buffer entries */
> >  	xfs_ino_t		lastino; /* last inode number returned */
> >  	int			nirbuf;	/* size of irbuf */
> >  	int			rval;	/* return value error code */
> >  	int			ubcount; /* size of user's buffer */
> > -	int			stat;
> >  	struct xfs_bulkstat_agichunk ac;
> > +	int			error = 0;
> >  
> >  	/*
> >  	 * Get the last inode value, see if there's nothing to do.
> >  	 */
> > -	ino = (xfs_ino_t)*lastinop;
> > -	lastino = ino;
> > -	agno = XFS_INO_TO_AGNO(mp, ino);
> > -	agino = XFS_INO_TO_AGINO(mp, ino);
> > +	lastino = *lastinop;
> > +	agno = XFS_INO_TO_AGNO(mp, lastino);
> > +	agino = XFS_INO_TO_AGINO(mp, lastino);
> >  	if (agno >= mp->m_sb.sb_agcount ||
> > -	    ino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> > +	    lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> >  		*done = 1;
> >  		*ubcountp = 0;
> >  		return 0;
> > @@ -393,8 +386,13 @@ xfs_bulkstat(
> >  	 * inode returned; 0 means start of the allocation group.
> >  	 */
> >  	rval = 0;
> > -	while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) {
> > -		cond_resched();
> > +	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
> > +		struct xfs_inobt_rec_incore	*irbp = irbuf;
> > +		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
> > +		bool				end_of_ag = false;
> > +		int				icount = 0;
> > +		int				stat;
> > +
> >  		error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
> >  		if (error)
> >  			break;
> > @@ -404,10 +402,6 @@ xfs_bulkstat(
> >  		 */
> >  		cur = xfs_inobt_init_cursor(mp, NULL, agbp, agno,
> >  					    XFS_BTNUM_INO);
> > -		irbp = irbuf;
> > -		irbufend = irbuf + nirbuf;
> > -		end_of_ag = 0;
> > -		icount = 0;
> >  		if (agino > 0) {
> >  			/*
> >  			 * In the middle of an allocation group, we need to get
> > @@ -432,7 +426,7 @@ xfs_bulkstat(
> >  			error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
> >  		}
> >  		if (error || stat == 0) {
> > -			end_of_ag = 1;
> > +			end_of_ag = true;
> >  			goto del_cursor;
> >  		}
> >  
> > @@ -445,7 +439,7 @@ xfs_bulkstat(
> >  
> >  			error = xfs_inobt_get_rec(cur, &r, &stat);
> >  			if (error || stat == 0) {
> > -				end_of_ag = 1;
> > +				end_of_ag = true;
> >  				goto del_cursor;
> >  			}
> >  
> > @@ -467,7 +461,7 @@ xfs_bulkstat(
> >  			agino = r.ir_startino + XFS_INODES_PER_CHUNK;
> >  			error = xfs_btree_increment(cur, 0, &stat);
> >  			if (error || stat == 0) {
> > -				end_of_ag = 1;
> > +				end_of_ag = true;
> >  				goto del_cursor;
> >  			}
> >  			cond_resched();
> > @@ -488,7 +482,7 @@ del_cursor:
> >  		 */
> >  		irbufend = irbp;
> >  		for (irbp = irbuf;
> > -		     irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> > +		     irbp < irbufend && ac.ac_ubleft >= statstruct_size;
> >  		     irbp++) {
> >  			error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
> >  					formatter, statstruct_size, &ac,
> > @@ -499,17 +493,14 @@ del_cursor:
> >  			cond_resched();
> >  		}
> >  
> > -		/*
> > -		 * Set up for the next loop iteration.
> > -		 */
> > -		if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
> > -			if (end_of_ag) {
> > -				agno++;
> > -				agino = 0;
> > -			} else
> > -				agino = XFS_INO_TO_AGINO(mp, lastino);
> > -		} else
> > +		if (error)
> >  			break;
> 
> This stood out to me as potentially broken as it introduces a
> non-deterministic error path. The preceding loop can overwrite error
> since it iterates until either all records are handled or the buffer is
> consumed.

That's true. Again an issue from splitting the larger patch up and
putting a chunk in the wrong patch. I'll move it to the next patch.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 6/6] xfs: track bulkstat progress by agino

[Dave Chinner]

On Tue, Nov 04, 2014 at 02:00:06PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:21PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The bulkstat main loop progress is tracked by the "lastino"
> > variable, which is a full 64 bit inode. However, the loop actually
> > works on agno/agino pairs, and so there's a significant disconnect
> > between the rest of the loop and the main cursor. Convert this to
> > use the agino, and pass the agino into the chunk formatting function
> > and convert it too.
> > 
> > This gets rid of the inconsistency in the loop processing, and
> > finally makes it simple for us to skip inodes at any point in the
> > loop simply by incrementing the agino cursor.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/xfs_itable.c | 50 +++++++++++++++++++-------------------------------
> >  1 file changed, 19 insertions(+), 31 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 6a4ef8e..2a6f2e8 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -282,45 +282,39 @@ xfs_bulkstat_ag_ichunk(
> >  	bulkstat_one_pf			formatter,
> >  	size_t				statstruct_size,
> >  	struct xfs_bulkstat_agichunk	*acp,
> > -	xfs_ino_t			*lastino)
> > +	xfs_agino_t			*last_agino)
> >  {
> >  	char				__user **ubufp = acp->ac_ubuffer;
> >  	int				chunkidx;
> >  	int				error = 0;
> > -	xfs_agino_t			agino;
> >  
> > -	agino = irbp->ir_startino;
> > +	*last_agino = irbp->ir_startino;
> 
> Shouldn't last_agino refer to the last inode written out to the
> userspace buffer? I'm not familiar with the semantics that xfsdump
> expects or works around here as you've discussed in the commit logs and
> whatnot. What happens if we set last_agino here and error out in the
> first iteration of the loop?

lastino is supposed to represent the last inode scanned, not
necessarily the last inode formatted into the buffer. This can be
seen that we update lastino even if the inode if free - it hasn't
been written into the user buffer, but we have processed it.

> 
> >  	for (chunkidx = 0; chunkidx < XFS_INODES_PER_CHUNK;
> > -	     chunkidx++, agino++) {
> > +	     chunkidx++, (*last_agino)++) {
> 
> Similar question here when we handle a full record and increment
> *last_agino beyond the last agino in the record. Unless I'm missing
> something, it looks to me that if the next record is adjacent,
> xfs_bulkstat_grab_ichunk() would skip the first inode in that record.

Hmmm - yes, that could occur. I have another patch that I didn't
post that changes the xfs_bulkstat_grab_ichunk() to not skip the
incoming lastino value, but I reasoned that it was a change of
behaviour and hence could cause xfsdump to break.

I'll rework *last_agino to do post-update rather than pre-update.

> >  	 * Done, we're either out of filesystem or space to put the data.
> > @@ -518,16 +509,13 @@ del_cursor:
> >  	if (ac.ac_ubelem)
> >  		error = 0;
> >  
> > -	if (agno >= mp->m_sb.sb_agcount) {
> > -		/*
> > -		 * If we ran out of filesystem, mark lastino as off
> > -		 * the end of the filesystem, so the next call
> > -		 * will return immediately.
> > -		 */
> > -		*lastinop = (xfs_ino_t)XFS_AGINO_TO_INO(mp, agno, 0);
> > +	/*
> > +	 * If we ran out of filesystem, lastino will point off the end of
> > +	 * the filesystem so the next call will return immediately.
> > +	 */
> > +	*lastinop = XFS_AGINO_TO_INO(mp, agno, agino);
> 
> This means that the main loop should never move agino forward unless the
> inodes up through agino are guaranteed to be copied out to the user
> buffer. There are two places in the main loop where we do this:
> 
> 	agino = r.ir_startino + XFS_INODES_PER_CHUNK;
> 
> ... and both of those lines execute before a potential error path that
> breaks out of the main loop before that current record is formatted out.
> Those lines appear to be spurious at this point, however, so I suspect
> we can just kill them.

*nod*

I was in two minds about that, but then figured agino would be
overwritten by the formatting callout if there was no error. I
didn't consider the error breakout case, so yes, those updates need
to die...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/6] xfs: bulkstat error handling is broken

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The error propagation is a horror - xfs_bulkstat() returns
a rval variable which is only set if there are formatter errors. Any
sort of btree walk error or corruption will cause the bulkstat walk
to terminate but will not pass an error back to userspace. Worse
is the fact that formatter errors will also be ignored if any inodes
were correctly formatted into the user buffer.

Hence bulkstat can fail badly yet still report success to userspace.
This causes significant issues with xfsdump not dumping everything
in the filesystem yet reporting success. It's not until a restore
fails that there is any indication that the dump was bad and tha
bulkstat failed. This patch now triggers xfsdump to fail with
bulkstat errors rather than silently missing files in the dump.

This now causes bulkstat to fail when the lastino cookie does not
fall inside an existing inode chunk. The pre-3.17 code tolerated
that error by allowing the code to move to the next inode chunk
as the agino target is guaranteed to fall into the next btree
record.

With the fixes up to this point in the series, xfsdump now passes on
the troublesome filesystem image that exposes all these bugs.

cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_itable.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index a9c28d1..be96754 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -236,8 +236,10 @@ xfs_bulkstat_grab_ichunk(
 	XFS_WANT_CORRUPTED_RETURN(stat == 1);
 
 	/* Check if the record contains the inode in request */
-	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
-		return -EINVAL;
+	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
+		*icount = 0;
+		return 0;
+	}
 
 	idx = agino - irec->ir_startino + 1;
 	if (idx < XFS_INODES_PER_CHUNK &&
@@ -352,7 +354,6 @@ xfs_bulkstat(
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
-	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
 	struct xfs_bulkstat_agichunk ac;
 	int			error = 0;
@@ -388,7 +389,6 @@ xfs_bulkstat(
 	 * Loop over the allocation groups, starting from the last
 	 * inode returned; 0 means start of the allocation group.
 	 */
-	rval = 0;
 	while (ac.ac_ubleft >= statstruct_size && agno < mp->m_sb.sb_agcount) {
 		struct xfs_inobt_rec_incore	*irbp = irbuf;
 		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
@@ -491,11 +491,14 @@ del_cursor:
 					formatter, statstruct_size, &ac,
 					&lastino);
 			if (error)
-				rval = error;
+				break;
 
 			cond_resched();
 		}
 
+		if (error)
+			break;
+
 		if (end_of_ag) {
 			agno++;
 			agino = 0;
@@ -507,11 +510,17 @@ del_cursor:
 	 */
 	kmem_free(irbuf);
 	*ubcountp = ac.ac_ubelem;
+
 	/*
-	 * Found some inodes, return them now and return the error next time.
+	 * We found some inodes, so clear the error status and return them.
+	 * The lastino pointer will point directly at the inode that triggered
+	 * any error that occurred, so on the next call the error will be
+	 * triggered again and propagated to userspace as there will be no
+	 * formatted inodes in the buffer.
 	 */
 	if (ac.ac_ubelem)
-		rval = 0;
+		error = 0;
+
 	if (agno >= mp->m_sb.sb_agcount) {
 		/*
 		 * If we ran out of filesystem, mark lastino as off
@@ -523,7 +532,7 @@ del_cursor:
 	} else
 		*lastinop = (xfs_ino_t)lastino;
 
-	return rval;
+	return error;
 }
 
 int
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/6] xfs: bulkstat error handling is broken

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The error propagation is a horror - xfs_bulkstat() returns
a rval variable which is only set if there are formatter errors. Any
sort of btree walk error or corruption will cause the bulkstat walk
to terminate but will not pass an error back to userspace. Worse
is the fact that formatter errors will also be ignored if any inodes
were correctly formatted into the user buffer.

Hence bulkstat can fail badly yet still report success to userspace.
This causes significant issues with xfsdump not dumping everything
in the filesystem yet reporting success. It's not until a restore
fails that there is any indication that the dump was bad and tha
bulkstat failed. This patch now triggers xfsdump to fail with
bulkstat errors rather than silently missing files in the dump.

This now causes bulkstat to fail when the lastino cookie does not
fall inside an existing inode chunk. The pre-3.17 code tolerated
that error by allowing the code to move to the next inode chunk
as the agino target is guaranteed to fall into the next btree
record.

With the fixes up to this point in the series, xfsdump now passes on
the troublesome filesystem image that exposes all these bugs.

cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/xfs_itable.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
index acae335..ff3f431 100644
--- a/fs/xfs/xfs_itable.c
+++ b/fs/xfs/xfs_itable.c
@@ -236,8 +236,10 @@ xfs_bulkstat_grab_ichunk(
 	XFS_WANT_CORRUPTED_RETURN(stat == 1);
 
 	/* Check if the record contains the inode in request */
-	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino)
-		return -EINVAL;
+	if (irec->ir_startino + XFS_INODES_PER_CHUNK <= agino) {
+		*icount = 0;
+		return 0;
+	}
 
 	idx = agino - irec->ir_startino + 1;
 	if (idx < XFS_INODES_PER_CHUNK &&
@@ -352,7 +354,6 @@ xfs_bulkstat(
 	xfs_inobt_rec_incore_t	*irbuf;	/* start of irec buffer */
 	xfs_ino_t		lastino; /* last inode number returned */
 	int			nirbuf;	/* size of irbuf */
-	int			rval;	/* return value error code */
 	int			ubcount; /* size of user's buffer */
 	struct xfs_bulkstat_agichunk ac;
 	int			error = 0;
@@ -388,7 +389,6 @@ xfs_bulkstat(
 	 * Loop over the allocation groups, starting from the last
 	 * inode returned; 0 means start of the allocation group.
 	 */
-	rval = 0;
 	while (agno < mp->m_sb.sb_agcount) {
 		struct xfs_inobt_rec_incore	*irbp = irbuf;
 		struct xfs_inobt_rec_incore	*irbufend = irbuf + nirbuf;
@@ -491,13 +491,16 @@ del_cursor:
 					formatter, statstruct_size, &ac,
 					&lastino);
 			if (error)
-				rval = error;
+				break;
 
 			cond_resched();
 		}
 
-		/* If we've run out of space, we are done */
-		if (ac.ac_ubleft < statstruct_size)
+		/*
+		 * If we've run out of space or had a formatting error, we
+		 * are now done
+		 */
+		if (ac.ac_ubleft < statstruct_size || error)
 			break;
 
 		if (end_of_ag) {
@@ -511,11 +514,17 @@ del_cursor:
 	 */
 	kmem_free(irbuf);
 	*ubcountp = ac.ac_ubelem;
+
 	/*
-	 * Found some inodes, return them now and return the error next time.
+	 * We found some inodes, so clear the error status and return them.
+	 * The lastino pointer will point directly at the inode that triggered
+	 * any error that occurred, so on the next call the error will be
+	 * triggered again and propagated to userspace as there will be no
+	 * formatted inodes in the buffer.
 	 */
 	if (ac.ac_ubelem)
-		rval = 0;
+		error = 0;
+
 	if (agno >= mp->m_sb.sb_agcount) {
 		/*
 		 * If we ran out of filesystem, mark lastino as off
@@ -527,7 +536,7 @@ del_cursor:
 	} else
 		*lastinop = (xfs_ino_t)lastino;
 
-	return rval;
+	return error;
 }
 
 int
-- 
2.0.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs