xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 5/6] xfs: bulkstat error handling is broken
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Tue, 4 Nov 2014 13:59:21 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1415105601-6455-6-git-send-email-david@xxxxxxxxxxxxx>
References: <1415105601-6455-1-git-send-email-david@xxxxxxxxxxxxx> <1415105601-6455-6-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
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

<Prev in Thread] Current Thread [Next in Thread>