xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 3/6] xfs: bulkstat chunk-formatter has issues
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 5 Nov 2014 08:18:38 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141104185849.GE55611@xxxxxxxxxxxxxxx>
References: <1415105601-6455-1-git-send-email-david@xxxxxxxxxxxxx> <1415105601-6455-4-git-send-email-david@xxxxxxxxxxxxx> <20141104185849.GE55611@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

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