This is a note to let you know that I've just added the patch titled
xfs: bulkstat error handling is broken
to the 3.17-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
xfs-bulkstat-error-handling-is-broken.patch
and it can be found in the queue-3.17 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@xxxxxxxxxxxxxxx> know about it.
>From febe3cbe38b0bc0a925906dc90e8d59048851f87 Mon Sep 17 00:00:00 2001
From: Dave Chinner <dchinner@xxxxxxxxxx>
Date: Fri, 7 Nov 2014 08:31:15 +1100
Subject: xfs: bulkstat error handling is broken
From: Dave Chinner <dchinner@xxxxxxxxxx>
commit febe3cbe38b0bc0a925906dc90e8d59048851f87 upstream.
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.
Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
fs/xfs/xfs_itable.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
--- 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
Patches currently in stable-queue which might be from dchinner@xxxxxxxxxx are
queue-3.17/xfs-bulkstat-main-loop-logic-is-a-mess.patch
queue-3.17/xfs-bulkstat-doesn-t-release-agi-buffer-on-error.patch
queue-3.17/xfs-bulkstat-chunk-formatter-has-issues.patch
queue-3.17/xfs-track-bulkstat-progress-by-agino.patch
queue-3.17/mm-remove-false-warn_on-from-pagecache_isize_extended.patch
queue-3.17/xfs-bulkstat-chunk-formatting-cursor-is-broken.patch
queue-3.17/xfs-bulkstat-error-handling-is-broken.patch
queue-3.17/xfs-bulkstat-btree-walk-doesn-t-terminate.patch
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html