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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
