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

[Dave Chinner <david@fromorbit.com>]

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@redhat.com>
> > 
> > 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@vger.kernel.org>
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs