xfs
[Top] [All Lists]

Re: [PATCH 2/6] xfs: bulkstat chunk formatting cursor is broken

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/6] xfs: bulkstat chunk formatting cursor is broken
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Tue, 4 Nov 2014 13:58:32 -0500
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1415105601-6455-3-git-send-email-david@xxxxxxxxxxxxx>
References: <1415105601-6455-1-git-send-email-david@xxxxxxxxxxxxx> <1415105601-6455-3-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Tue, Nov 04, 2014 at 11:53:17PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> The xfs_bulkstat_agichunk formatting cursor takes buffer values from
> the main loop and passes them via the structure to the chunk
> formatter, and the writes the chnged values back into the main loop
> local variables. UNfortunately, this complex dance is full of corner
> cases that aren't handled correctly.
> 
> The biggest problem is that it is double handling the information in
> both the main loop and the chunk formatting function, leading to
> inconsistent updates and endless loops where progress is not made.
> 
> To fix this, push the struct xfs_bulkstat_agichunk outwards to be
> the primary holder of user buffer information. this removes the
> double handling in the main loop.
> 
> Also, pass the last inode processed by the chunk formatter as a
> separate parameter as it purely an output variable and is not
> related to the user buffer consumption cursor.
> 
> Finally, the chunk formatting code is not shared by anyone, so make
> it local to xfs_itable.c.
> 
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

The commit log suggests this patch is fixing some problem(s), but I see
mostly a refactor to pull up the ac structure and eliminate the variable
swapping between xfs_bulkstat() and xfs_bulkstat_ag_ichunk(). The
lastino separation doesn't appear to change semantics that I can tell
either. The refactor looks good to me:

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

... but is there something subtle in here I'm missing about the problem
being fixed? If not, could we update the commit title to suggest this is
a clean up?

Brian

>  fs/xfs/xfs_itable.c | 59 
> +++++++++++++++++++++++++----------------------------
>  fs/xfs/xfs_itable.h | 16 ---------------
>  2 files changed, 28 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/xfs/xfs_itable.c b/fs/xfs/xfs_itable.c
> index 16737cb..50a3e59 100644
> --- a/fs/xfs/xfs_itable.c
> +++ b/fs/xfs/xfs_itable.c
> @@ -262,20 +262,26 @@ xfs_bulkstat_grab_ichunk(
>  
>  #define XFS_BULKSTAT_UBLEFT(ubleft)  ((ubleft) >= statstruct_size)
>  
> +struct xfs_bulkstat_agichunk {
> +     char            __user **ac_ubuffer;/* pointer into user's buffer */
> +     int             ac_ubleft;      /* bytes left in user's buffer */
> +     int             ac_ubelem;      /* spaces used in user's buffer */
> +};
> +
>  /*
>   * Process inodes in chunk with a pointer to a formatter function
>   * that will iget the inode and fill in the appropriate structure.
>   */
> -int
> +static int
>  xfs_bulkstat_ag_ichunk(
>       struct xfs_mount                *mp,
>       xfs_agnumber_t                  agno,
>       struct xfs_inobt_rec_incore     *irbp,
>       bulkstat_one_pf                 formatter,
>       size_t                          statstruct_size,
> -     struct xfs_bulkstat_agichunk    *acp)
> +     struct xfs_bulkstat_agichunk    *acp,
> +     xfs_ino_t                       *lastino)
>  {
> -     xfs_ino_t                       lastino = acp->ac_lastino;
>       char                            __user **ubufp = acp->ac_ubuffer;
>       int                             ubleft = acp->ac_ubleft;
>       int                             ubelem = acp->ac_ubelem;
> @@ -295,7 +301,7 @@ xfs_bulkstat_ag_ichunk(
>  
>               /* Skip if this inode is free */
>               if (XFS_INOBT_MASK(chunkidx) & irbp->ir_free) {
> -                     lastino = ino;
> +                     *lastino = ino;
>                       continue;
>               }
>  
> @@ -313,7 +319,7 @@ xfs_bulkstat_ag_ichunk(
>                               ubleft = 0;
>                               break;
>                       }
> -                     lastino = ino;
> +                     *lastino = ino;
>                       continue;
>               }
>               if (fmterror == BULKSTAT_RV_GIVEUP) {
> @@ -325,10 +331,9 @@ xfs_bulkstat_ag_ichunk(
>                       *ubufp += ubused;
>               ubleft -= ubused;
>               ubelem++;
> -             lastino = ino;
> +             *lastino = ino;
>       }
>  
> -     acp->ac_lastino = lastino;
>       acp->ac_ubleft = ubleft;
>       acp->ac_ubelem = ubelem;
>  
> @@ -355,7 +360,6 @@ xfs_bulkstat(
>       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                     fmterror;/* bulkstat formatter result */
>       int                     icount; /* count of inodes good in irbuf */
>       size_t                  irbsize; /* size of irec buffer in bytes */
>       xfs_ino_t               ino;    /* inode number (filesystem) */
> @@ -366,10 +370,8 @@ xfs_bulkstat(
>       int                     nirbuf; /* size of irbuf */
>       int                     rval;   /* return value error code */
>       int                     ubcount; /* size of user's buffer */
> -     int                     ubleft; /* bytes left in user's buffer */
> -     char                    __user *ubufp;  /* pointer into user's buffer */
> -     int                     ubelem; /* spaces used in user's buffer */
>       int                     stat;
> +     struct xfs_bulkstat_agichunk ac;
>  
>       /*
>        * Get the last inode value, see if there's nothing to do.
> @@ -386,11 +388,13 @@ xfs_bulkstat(
>       }
>  
>       ubcount = *ubcountp; /* statstruct's */
> -     ubleft = ubcount * statstruct_size; /* bytes */
> -     *ubcountp = ubelem = 0;
> +     ac.ac_ubuffer = &ubuffer;
> +     ac.ac_ubleft = ubcount * statstruct_size; /* bytes */;
> +     ac.ac_ubelem = 0;
> +
> +     *ubcountp = 0;
>       *done = 0;
> -     fmterror = 0;
> -     ubufp = ubuffer;
> +
>       irbuf = kmem_zalloc_greedy(&irbsize, PAGE_SIZE, PAGE_SIZE * 4);
>       if (!irbuf)
>               return -ENOMEM;
> @@ -402,7 +406,7 @@ xfs_bulkstat(
>        * inode returned; 0 means start of the allocation group.
>        */
>       rval = 0;
> -     while (XFS_BULKSTAT_UBLEFT(ubleft) && agno < mp->m_sb.sb_agcount) {
> +     while (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft) && agno < mp->m_sb.sb_agcount) 
> {
>               cond_resched();
>               error = xfs_ialloc_read_agi(mp, NULL, agno, &agbp);
>               if (error)
> @@ -497,28 +501,21 @@ del_cursor:
>                */
>               irbufend = irbp;
>               for (irbp = irbuf;
> -                  irbp < irbufend && XFS_BULKSTAT_UBLEFT(ubleft); irbp++) {
> -                     struct xfs_bulkstat_agichunk ac;
> -
> -                     ac.ac_lastino = lastino;
> -                     ac.ac_ubuffer = &ubuffer;
> -                     ac.ac_ubleft = ubleft;
> -                     ac.ac_ubelem = ubelem;
> +                  irbp < irbufend && XFS_BULKSTAT_UBLEFT(ac.ac_ubleft);
> +                  irbp++) {
>                       error = xfs_bulkstat_ag_ichunk(mp, agno, irbp,
> -                                     formatter, statstruct_size, &ac);
> +                                     formatter, statstruct_size, &ac,
> +                                     &lastino);
>                       if (error)
>                               rval = error;
>  
> -                     lastino = ac.ac_lastino;
> -                     ubleft = ac.ac_ubleft;
> -                     ubelem = ac.ac_ubelem;
> -
>                       cond_resched();
>               }
> +
>               /*
>                * Set up for the next loop iteration.
>                */
> -             if (XFS_BULKSTAT_UBLEFT(ubleft)) {
> +             if (XFS_BULKSTAT_UBLEFT(ac.ac_ubleft)) {
>                       if (end_of_ag) {
>                               agno++;
>                               agino = 0;
> @@ -531,11 +528,11 @@ del_cursor:
>        * Done, we're either out of filesystem or space to put the data.
>        */
>       kmem_free(irbuf);
> -     *ubcountp = ubelem;
> +     *ubcountp = ac.ac_ubelem;
>       /*
>        * Found some inodes, return them now and return the error next time.
>        */
> -     if (ubelem)
> +     if (ac.ac_ubelem)
>               rval = 0;
>       if (agno >= mp->m_sb.sb_agcount) {
>               /*
> diff --git a/fs/xfs/xfs_itable.h b/fs/xfs/xfs_itable.h
> index aaed080..6ea8b39 100644
> --- a/fs/xfs/xfs_itable.h
> +++ b/fs/xfs/xfs_itable.h
> @@ -30,22 +30,6 @@ typedef int (*bulkstat_one_pf)(struct xfs_mount    *mp,
>                              int              *ubused,
>                              int              *stat);
>  
> -struct xfs_bulkstat_agichunk {
> -     xfs_ino_t       ac_lastino;     /* last inode returned */
> -     char            __user **ac_ubuffer;/* pointer into user's buffer */
> -     int             ac_ubleft;      /* bytes left in user's buffer */
> -     int             ac_ubelem;      /* spaces used in user's buffer */
> -};
> -
> -int
> -xfs_bulkstat_ag_ichunk(
> -     struct xfs_mount                *mp,
> -     xfs_agnumber_t                  agno,
> -     struct xfs_inobt_rec_incore     *irbp,
> -     bulkstat_one_pf                 formatter,
> -     size_t                          statstruct_size,
> -     struct xfs_bulkstat_agichunk    *acp);
> -
>  /*
>   * Values for stat return value.
>   */
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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