xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 3/6] xfs: bulkstat chunk-formatter has issues
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 6 Nov 2014 11:31:54 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1415279699-8739-4-git-send-email-david@xxxxxxxxxxxxx>
References: <1415279699-8739-1-git-send-email-david@xxxxxxxxxxxxx> <1415279699-8739-4-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Nov 07, 2014 at 12:14:56AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

FYI, this looks exactly like the previous version. That seems fine
because we still handle lastino separately from agino at this point.

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

>  fs/xfs/xfs_itable.c | 58 
> ++++++++++++++++++++++-------------------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 50a3e59..7ea2b11 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -283,59 +283,49 @@ 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++;
> +
> +             /* be careful not to leak error if at end of chunk */
> +             if (fmterror == BULKSTAT_RV_NOTHING || error) {
> +                     *lastino = ino;
> +                     error = 0;
> +                     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@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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