Re: crash with latest code drop.

[Dave Chinner <david@fromorbit.com>]

Ping?

Can this patch be applied to the new sync code before it is pushed
to Linus?

Cheers,

Dave.

On Wed, Oct 15, 2008 at 05:19:17PM +1100, Dave Chinner wrote:
> On Wed, Oct 15, 2008 at 03:50:48PM +1000, Peter Leckie wrote:
> > Dave Chinner wrote:
> >> Update below.
> >>
> >> Cheers,
> >>
> >> Dave.
> >>   
> > The original patch appeared to fix the issue, however the latest one  
> > Oops as follows:
> 
> Well, I think the problem dhould be obvious - it's the same as
> the first report - deferencing the linux inode without first having
> a refernce on it.
> 
> FWIW, if you apply the "combine linux/xfs inode" patch series,
> this failure will go away.
> 
> Updated patch below.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> XFS: avoid all reclaimable inodes in xfs_sync_inodes_ag
> 
> If we are syncing data in xfs_sync_inodes_ag(), the VFS
> inode must still be referencable as the dirty data state
> is carried on the VFS inode. hence if we can't get a
> reference via igrab(), the inode must be in reclaim which
> implies that it has no dirty data attached.
> 
> Leave such inodes to the reclaim code to flush the dirty
> inode state to disk and so avoid attempting to access the
> VFS inode when it may not exist in xfs_sync_inodes_ag().
> 
> Version 4:
> o don't reference liinux inode untiil after igrab() succeeds
> 
> Version 3:
> o converted unlock/rele to an xfs_iput() call.
> 
> Version 2:
> o change igrab logic to be more linear
> o remove initial reclaimable inode check now that we are using
>   igrab() failure to find reclaimable inodes
> o assert that igrab failure occurs only on reclaimable inodes
> o clean up inode locking - only grab the iolock if we are doing
>   a SYNC_DELWRI call and we have a dirty inode.
> ---
>  fs/xfs/linux-2.6/xfs_sync.c |   75 ++++++++++--------------------------------
>  1 files changed, 18 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/xfs/linux-2.6/xfs_sync.c b/fs/xfs/linux-2.6/xfs_sync.c
> index 08b2acf..39ed7f3 100644
> --- a/fs/xfs/linux-2.6/xfs_sync.c
> +++ b/fs/xfs/linux-2.6/xfs_sync.c
> @@ -63,25 +63,16 @@ xfs_sync_inodes_ag(
>  	int		error = 0;
>  	int		last_error = 0;
>  	int		fflag = XFS_B_ASYNC;
> -	int		lock_flags = XFS_ILOCK_SHARED;
>  
>  	if (flags & SYNC_DELWRI)
>  		fflag = XFS_B_DELWRI;
>  	if (flags & SYNC_WAIT)
>  		fflag = 0;		/* synchronous overrides all */
>  
> -	if (flags & SYNC_DELWRI) {
> -		/*
> -		 * We need the I/O lock if we're going to call any of
> -		 * the flush/inval routines.
> -		 */
> -		lock_flags |= XFS_IOLOCK_SHARED;
> -	}
> -
>  	do {
>  		struct inode	*inode;
> -		boolean_t	inode_refed;
>  		xfs_inode_t	*ip = NULL;
> +		int		lock_flags = XFS_ILOCK_SHARED;
>  
>  		/*
>  		 * use a gang lookup to find the next inode in the tree
> @@ -109,22 +100,6 @@ xfs_sync_inodes_ag(
>  			break;
>  		}
>  
> -		/*
> -		 * skip inodes in reclaim. Let xfs_syncsub do that for
> -		 * us so we don't need to worry.
> -		 */
> -		if (xfs_iflags_test(ip, (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
> -			read_unlock(&pag->pag_ici_lock);
> -			continue;
> -		}
> -
> -		/* bad inodes are dealt with elsewhere */
> -		inode = VFS_I(ip);
> -		if (is_bad_inode(inode)) {
> -			read_unlock(&pag->pag_ici_lock);
> -			continue;
> -		}
> -
>  		/* nothing to sync during shutdown */
>  		if (XFS_FORCED_SHUTDOWN(mp)) {
>  			read_unlock(&pag->pag_ici_lock);
> @@ -132,42 +107,34 @@ xfs_sync_inodes_ag(
>  		}
>  
>  		/*
> -		 * If we can't get a reference on the VFS_I, the inode must be
> -		 * in reclaim. If we can get the inode lock without blocking,
> -		 * it is safe to flush the inode because we hold the tree lock
> -		 * and xfs_iextract will block right now. Hence if we lock the
> -		 * inode while holding the tree lock, xfs_ireclaim() is
> -		 * guaranteed to block on the inode lock we now hold and hence
> -		 * it is safe to reference the inode until we drop the inode
> -		 * locks completely.
> +		 * If we can't get a reference on the inode, it must be
> +		 * in reclaim. Leave it for the reclaim code to flush.
>  		 */
> -		inode_refed = B_FALSE;
> -		if (igrab(inode)) {
> -			read_unlock(&pag->pag_ici_lock);
> -			xfs_ilock(ip, lock_flags);
> -			inode_refed = B_TRUE;
> -		} else {
> -			if (!xfs_ilock_nowait(ip, lock_flags)) {
> -				/* leave it to reclaim */
> -				read_unlock(&pag->pag_ici_lock);
> -				continue;
> -			}
> +		inode = VFS_I(ip);
> +		if (!igrab(inode)) {
>  			read_unlock(&pag->pag_ici_lock);
> +			continue;
> +		}
> +		read_unlock(&pag->pag_ici_lock);
> +
> +		/* bad inodes are dealt with elsewhere */
> +		if (is_bad_inode(inode)) {
> +			IRELE(ip);
> +			continue;
>  		}
>  
>  		/*
>  		 * If we have to flush data or wait for I/O completion
> -		 * we need to drop the ilock that we currently hold.
> -		 * If we need to drop the lock, insert a marker if we
> -		 * have not already done so.
> +		 * we need to hold the iolock.
>  		 */
>  		if ((flags & SYNC_DELWRI) && VN_DIRTY(inode)) {
> -			xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +			xfs_ilock(ip, XFS_IOLOCK_SHARED);
> +			lock_flags |= XFS_IOLOCK_SHARED;
>  			error = xfs_flush_pages(ip, 0, -1, fflag, FI_NONE);
>  			if (flags & SYNC_IOWAIT)
>  				vn_iowait(ip);
> -			xfs_ilock(ip, XFS_ILOCK_SHARED);
>  		}
> +		xfs_ilock(ip, XFS_ILOCK_SHARED);
>  
>  		if ((flags & SYNC_ATTR) && !xfs_inode_clean(ip)) {
>  			if (flags & SYNC_WAIT) {
> @@ -183,13 +150,7 @@ xfs_sync_inodes_ag(
>  					xfs_ifunlock(ip);
>  			}
>  		}
> -
> -		if (lock_flags)
> -			xfs_iunlock(ip, lock_flags);
> -
> -		if (inode_refed) {
> -			IRELE(ip);
> -		}
> +		xfs_iput(ip, lock_flags);
>  
>  		if (error)
>  			last_error = error;
> 
> 
> 

-- 
Dave Chinner
david@fromorbit.com