xfs
[Top] [All Lists]

Re: [PATCH 4/6] xfs: bulkstat main loop logic is a mess

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 4/6] xfs: bulkstat main loop logic is a mess
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 5 Nov 2014 08:20:42 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141104185908.GF55611@xxxxxxxxxxxxxxx>
References: <1415105601-6455-1-git-send-email-david@xxxxxxxxxxxxx> <1415105601-6455-5-git-send-email-david@xxxxxxxxxxxxx> <20141104185908.GF55611@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Nov 04, 2014 at 01:59:08PM -0500, Brian Foster wrote:
> On Tue, Nov 04, 2014 at 11:53:19PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> > 
> > There are a bunch of variables tha tare more wildy scoped than they
> > need to be, obfuscated user buffer checks and tortured "next inode"
> > tracking. This all needs cleaning up to expose the real issues that
> > need fixing.
> > 
> > cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_itable.c | 55 
> > ++++++++++++++++++++++-------------------------------
> >  1 file changed, 23 insertions(+), 32 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> > index ff31965..fc4cf5d 100644
> > --- a/fs/xfs/xfs_itable.c
> > +++ b/fs/xfs/xfs_itable.c
> > @@ -345,30 +345,23 @@ xfs_bulkstat(
> >     xfs_agino_t             agino;  /* inode # in allocation group */
> >     xfs_agnumber_t          agno;   /* allocation group number */
> >     xfs_btree_cur_t         *cur;   /* btree cursor for ialloc btree */
> > -   int                     end_of_ag; /* set if we've seen the ag end */
> > -   int                     error;  /* error code */
> > -   int                     icount; /* count of inodes good in irbuf */
> >     size_t                  irbsize; /* size of irec buffer in bytes */
> > -   xfs_ino_t               ino;    /* inode number (filesystem) */
> > -   xfs_inobt_rec_incore_t  *irbp;  /* current irec buffer pointer */
> >     xfs_inobt_rec_incore_t  *irbuf; /* start of irec buffer */
> > -   xfs_inobt_rec_incore_t  *irbufend; /* end of good irec buffer entries */
> >     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 */
> > -   int                     stat;
> >     struct xfs_bulkstat_agichunk ac;
> > +   int                     error = 0;
> >  
> >     /*
> >      * Get the last inode value, see if there's nothing to do.
> >      */
> > -   ino = (xfs_ino_t)*lastinop;
> > -   lastino = ino;
> > -   agno = XFS_INO_TO_AGNO(mp, ino);
> > -   agino = XFS_INO_TO_AGINO(mp, ino);
> > +   lastino = *lastinop;
> > +   agno = XFS_INO_TO_AGNO(mp, lastino);
> > +   agino = XFS_INO_TO_AGINO(mp, lastino);
> >     if (agno >= mp->m_sb.sb_agcount ||
> > -       ino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> > +       lastino != XFS_AGINO_TO_INO(mp, agno, agino)) {
> >             *done = 1;
> >             *ubcountp = 0;
> >             return 0;
> > @@ -393,8 +386,13 @@ xfs_bulkstat(
> >      * inode returned; 0 means start of the allocation group.
> >      */
> >     rval = 0;
> > -   while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) 
> > {
> > -           cond_resched();
> > +   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;
> > +           bool                            end_of_ag = false;
> > +           int                             icount = 0;
> > +           int                             stat;
> > +
> >             error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
> >             if (error)
> >                     break;
> > @@ -404,10 +402,6 @@ xfs_bulkstat(
> >              */
> >             cur = xfs_inobt_init_cursor(mp, NULL, agbp, agno,
> >                                         XFS_BTNUM_INO);
> > -           irbp = irbuf;
> > -           irbufend = irbuf + nirbuf;
> > -           end_of_ag = 0;
> > -           icount = 0;
> >             if (agino > 0) {
> >                     /*
> >                      * In the middle of an allocation group, we need to get
> > @@ -432,7 +426,7 @@ xfs_bulkstat(
> >                     error = xfs_inobt_lookup(cur, 0, XFS_LOOKUP_GE, &stat);
> >             }
> >             if (error || stat == 0) {
> > -                   end_of_ag = 1;
> > +                   end_of_ag = true;
> >                     goto del_cursor;
> >             }
> >  
> > @@ -445,7 +439,7 @@ xfs_bulkstat(
> >  
> >                     error = xfs_inobt_get_rec(cur, &r, &stat);
> >                     if (error || stat == 0) {
> > -                           end_of_ag = 1;
> > +                           end_of_ag = true;
> >                             goto del_cursor;
> >                     }
> >  
> > @@ -467,7 +461,7 @@ xfs_bulkstat(
> >                     agino = r.ir_startino + XFS_INODES_PER_CHUNK;
> >                     error = xfs_btree_increment(cur, 0, &stat);
> >                     if (error || stat == 0) {
> > -                           end_of_ag = 1;
> > +                           end_of_ag = true;
> >                             goto del_cursor;
> >                     }
> >                     cond_resched();
> > @@ -488,7 +482,7 @@ del_cursor:
> >              */
> >             irbufend = irbp;
> >             for (irbp = irbuf;
> > -                irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> > +                irbp < irbufend && ac.ac_ubleft >= statstruct_size;
> >                  irbp++) {
> >                     error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
> >                                     formatter, statstruct_size, &ac,
> > @@ -499,17 +493,14 @@ del_cursor:
> >                     cond_resched();
> >             }
> >  
> > -           /*
> > -            * Set up for the next loop iteration.
> > -            */
> > -           if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
> > -                   if (end_of_ag) {
> > -                           agno++;
> > -                           agino = 0;
> > -                   } else
> > -                           agino = XFS_INO_TO_AGINO(mp, lastino);
> > -           } else
> > +           if (error)
> >                     break;
> 
> This stood out to me as potentially broken as it introduces a
> non-deterministic error path. The preceding loop can overwrite error
> since it iterates until either all records are handled or the buffer is
> consumed.

That's true. Again an issue from splitting the larger patch up and
putting a chunk in the wrong patch. I'll move it to the next patch.

Cheers,

Dave.

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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