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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 04, 2014 at 01:58:32PM -0500, Brian Foster wrote:
> 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?

There is something subtle in this, but I didn't dig into it a great
deal.

Essentially, I had one big patch that changed everythign related to
the user buffer handling, but I felt it was too large to easily
review so I split it into two - one for the mainloop and one for the
chunk formatter. If I drop either of the two patches then bulkstat
hangs up in an endless loop. Neither of the two patches look to fix
obvious problems, but they do change behaviour.

So yes, there are subtle interactions with the rest of the main loop
state here, but yesterday afternoon I really didn't care much about
exactly what bugs the split patches were fixing, just that after
them it all worked correctly.

Cheers,

Dave.


> 
> 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
> 

-- 
Dave Chinner
david@xxxxxxxxxxxxx

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




[Index of Archives]     [Linux XFS Devel]     [Linux Filesystem Development]     [Filesystem Testing]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux