From: Jeffrey Altman Date: Fri, 17 Feb 2012 04:50:18 +0000 (-0500) Subject: Windows: VolumeCB->ObjectInfoTree.TreeLock Deadlock X-Git-Tag: openafs-stable-1_8_0pre1~2753 X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=780e497b32a927e008474a63b0427eca5d5a8877 Windows: VolumeCB->ObjectInfoTree.TreeLock Deadlock AFSPrimaryVolumeWorkerThread held the VolumeCB->ObjectInfoTree.TreeLock exclusively across calls to AFSCleanupFcb() which in turn triggers a file extent release to the service which can in turn result in an object invalidation. Processing the invalidation requires shared access to VolumeCB->ObjectInfoTree.TreeLock which results in a deadlock. This patch alters the processing of AFSPrimaryVolumeWorkerThread so that the VolumeCB->ObjectInfoTree.TreeLock is not held across the AFSCleanupFcb() calls. FIXES 130431 Change-Id: I3726df02ab47d2dcc83a32c75957a5dafcfbf20e Reviewed-on: http://gerrit.openafs.org/6724 Tested-by: BuildBot Reviewed-by: Peter Scott Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp index 72d8fe2..b4adbec 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp @@ -982,6 +982,8 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) AFSDirectoryCB *pCurrentDirEntry = NULL, *pNextDirEntry = NULL; BOOLEAN bReleaseVolumeLock = FALSE; AFSVolumeCB *pVolumeCB = NULL, *pNextVolume = NULL; + AFSFcb *pFcb = NULL; + LONG lFileType; LARGE_INTEGER liCurrentTime; BOOLEAN bVolumeObject = FALSE; LONG lCount; @@ -1189,18 +1191,6 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) if( pCurrentObject->Fcb != NULL) { - // - // Acquire and drop the Fcb resource to synchronize - // with a potentially active AFSCleanup() which sets - // the OpenReferenceCount to zero while holding the - // resource. - // - - AFSAcquireExcl( &pCurrentObject->Fcb->NPFcb->Resource, - TRUE); - - AFSReleaseResource( &pCurrentObject->Fcb->NPFcb->Resource); - AFSRemoveFcb( &pCurrentObject->Fcb); } @@ -1224,7 +1214,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) AFSDbgLogMsg( AFS_SUBSYSTEM_CLEANUP_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, - "AFSPrimaryWorker Deleting deleted object %08lX\n", + "AFSPrimaryVolumeWorkerThread Deleting deleted object %08lX\n", pCurrentObject); AFSDeleteObjectInfo( pCurrentObject); @@ -1377,9 +1367,11 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) pCurrentChildObject = pCurrentDirEntry->ObjectInformation; + pFcb = NULL; + AFSDbgLogMsg( AFS_SUBSYSTEM_CLEANUP_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, - "AFSPrimaryWorker Deleting DE %wZ Object %08lX\n", + "AFSPrimaryVolumeWorkerThread Deleting DE %wZ Object %08lX\n", &pCurrentDirEntry->NameInformation.FileName, pCurrentChildObject); @@ -1392,26 +1384,9 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) if( pCurrentChildObject->Fcb != NULL) { - if( pCurrentChildObject->FileType == AFS_FILE_TYPE_FILE) - { - - AFSCleanupFcb( pCurrentChildObject->Fcb, - TRUE); - } + pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentChildObject->Fcb, NULL, (PVOID)pCurrentChildObject->Fcb); - // - // Acquire and drop the Fcb resource to synchronize - // with a potentially active AFSCleanup() which sets - // the OpenReferenceCount to zero while holding the - // resource. - // - - AFSAcquireExcl( &pCurrentChildObject->Fcb->NPFcb->Resource, - TRUE); - - AFSReleaseResource( &pCurrentChildObject->Fcb->NPFcb->Resource); - - AFSRemoveFcb( &pCurrentChildObject->Fcb); + lFileType = pCurrentChildObject->FileType; } if( pCurrentChildObject->FileType == AFS_FILE_TYPE_DIRECTORY && @@ -1435,13 +1410,36 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) AFSDbgLogMsg( AFS_SUBSYSTEM_CLEANUP_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, - "AFSPrimaryWorker Deleting object %08lX\n", + "AFSPrimaryVolumeWorkerThread Deleting object %08lX\n", pCurrentChildObject); AFSDeleteObjectInfo( pCurrentChildObject); } pCurrentDirEntry = pNextDirEntry; + + if ( pFcb != NULL) + { + + if( lFileType == AFS_FILE_TYPE_FILE) + { + // + // We must not hold pVolumeCB->ObjectInfoTree.TreeLock exclusive + // across an AFSCleanupFcb call since it can deadlock with an + // invalidation call from the service. + // + + AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); + + AFSCleanupFcb( pFcb, + TRUE); + + AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, + TRUE); + } + + AFSRemoveFcb( &pFcb); + } } pCurrentObject->Specific.Directory.DirectoryNodeListHead = NULL; @@ -1458,7 +1456,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) AFSDbgLogMsg( AFS_SUBSYSTEM_DIR_NODE_COUNT, AFS_TRACE_LEVEL_VERBOSE, - "AFSPrimaryWorker Reset count to 0 on parent FID %08lX-%08lX-%08lX-%08lX\n", + "AFSPrimaryVolumeWorkerThread Reset count to 0 on parent FID %08lX-%08lX-%08lX-%08lX\n", pCurrentObject->FileId.Cell, pCurrentObject->FileId.Volume, pCurrentObject->FileId.Vnode, @@ -1531,51 +1529,16 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) pCurrentObject->Fcb->OpenReferenceCount == 0)) { - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - - if( AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, - FALSE)) - { - - if( pCurrentObject->Fcb != NULL) - { - - AFSCleanupFcb( pCurrentObject->Fcb, - TRUE); - - // - // Acquire and drop the Fcb resource to synchronize - // with a potentially active AFSCleanup() which sets - // the OpenReferenceCount to zero while holding the - // resource. - // - - AFSAcquireExcl( &pCurrentObject->Fcb->NPFcb->Resource, - TRUE); - - AFSReleaseResource( &pCurrentObject->Fcb->NPFcb->Resource); - - AFSRemoveFcb( &pCurrentObject->Fcb); - } - - AFSDeleteObjectInfo( pCurrentObject); - - AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock); - - pCurrentObject = pNextObject; + pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentObject->Fcb, NULL, (PVOID)pCurrentObject->Fcb); - continue; - } - else + if( pFcb != NULL) { - bReleaseVolumeLock = FALSE; + AFSCleanupFcb( pFcb, + TRUE); - break; + AFSRemoveFcb( &pFcb); } - } - else if( pCurrentObject->Fcb != NULL) - { AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); @@ -1583,8 +1546,7 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) FALSE)) { - AFSCleanupFcb( pCurrentObject->Fcb, - FALSE); + AFSDeleteObjectInfo( pCurrentObject); AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock); @@ -1600,6 +1562,12 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) break; } } + else if( pCurrentObject->Fcb != NULL) + { + + AFSCleanupFcb( pCurrentObject->Fcb, + FALSE); + } } pCurrentObject = pNextObject;