On Tue, Nov 04, 2014 at 11:53:20PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> 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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
