From 780e497b32a927e008474a63b0427eca5d5a8877 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Thu, 16 Feb 2012 23:50:18 -0500 Subject: [PATCH] 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 --- src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp | 124 ++++++++-------------- 1 file changed, 46 insertions(+), 78 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp index 72d8fe2115..b4adbeca46 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) - { + pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentChildObject->Fcb, NULL, (PVOID)pCurrentChildObject->Fcb); - AFSCleanupFcb( pCurrentChildObject->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( &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,33 +1529,23 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) pCurrentObject->Fcb->OpenReferenceCount == 0)) { + pFcb = (AFSFcb *) InterlockedCompareExchangePointer( (PVOID *)&pCurrentObject->Fcb, NULL, (PVOID)pCurrentObject->Fcb); + + if( pFcb != NULL) + { + + AFSCleanupFcb( pFcb, + TRUE); + + AFSRemoveFcb( &pFcb); + } + 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); @@ -1577,28 +1565,8 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context) else if( pCurrentObject->Fcb != NULL) { - AFSReleaseResource( pVolumeCB->ObjectInfoTree.TreeLock); - - if( AFSAcquireExcl( pVolumeCB->ObjectInfoTree.TreeLock, - FALSE)) - { - - AFSCleanupFcb( pCurrentObject->Fcb, - FALSE); - - AFSConvertToShared( pVolumeCB->ObjectInfoTree.TreeLock); - - pCurrentObject = pNextObject; - - continue; - } - else - { - - bReleaseVolumeLock = FALSE; - - break; - } + AFSCleanupFcb( pCurrentObject->Fcb, + FALSE); } }