xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/6] xfs: bulkstat main loop logic is a mess
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 6 Nov 2014 11:32:07 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1415279699-8739-5-git-send-email-david@xxxxxxxxxxxxx>
References: <1415279699-8739-1-git-send-email-david@xxxxxxxxxxxxx> <1415279699-8739-5-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Fri, Nov 07, 2014 at 12:14:57AM +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>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---

This one contains the changes to the outer loop to move the ubleft
checks from the iteration logic to within the loop (before the end_of_ag
check). Changes look fine to me:

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

>  fs/xfs/xfs_itable.c | 56 
> +++++++++++++++++++++++------------------------------
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 7ea2b11..acae335 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -348,30 +348,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;
> @@ -396,8 +389,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 (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;
> @@ -407,10 +405,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
> @@ -435,7 +429,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;
>               }
>  
> @@ -448,7 +442,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;
>                       }
>  
> @@ -470,7 +464,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();
> @@ -491,7 +485,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,
> @@ -502,17 +496,15 @@ 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 we've run out of space, we are done */
> +             if (ac.ac_ubleft < statstruct_size)
>                       break;
> +
> +             if (end_of_ag) {
> +                     agno++;
> +                     agino = 0;
> +             } else
> +                     agino = XFS_INO_TO_AGINO(mp, lastino);
>       }
>       /*
>        * Done, we're either out of filesystem or space to put the data.
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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