On Tue, Nov 04, 2014 at 01:58:49PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:18PM +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>
> > ---
> > fs/xfs/xfs_itable.c | 55 ++++++++++++++++++++---------------------------------
> > 1 file changed, 21 insertions(+), 34 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index 50a3e59..ff31965 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -283,59 +283,46 @@ 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++;
> > + if (fmterror == BULKSTAT_RV_NOTHING || error) {
> > + *lastino = ino;
> > + continue;
> > + }
>
> Not introduced by this patch, but this looks like inconsistent error
> handling. The structure of the code suggests we intend to carry on in
> the event of ENOENT or EINVAL, but said errors can leak out of this
> function if we never get back to the formatter call (e.g., we're at the
> last inode or all subsequent inodes are free).
Good catch - I'll fix it. I'll just zero the error in that branch.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
