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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
