Re: crash with latest code drop.

Peter Leckie
2008-10-23T03:22:38Z

Hey Dave I'll push it now, I assume it's ok to go in with out the check
for a inode first
as the combine linux/XFS inode patch is in.

Thanks,
Pete

Dave Chinner wrote:
> 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;
>>
>>
>>
>>
>
>