From: Jeffrey Altman Date: Thu, 24 May 2012 08:57:19 +0000 (-0400) Subject: Windows: Avoid deadlock on VolumeCB->VolumeLock X-Git-Tag: openafs-stable-1_8_0pre1~2362 X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=06a602bfd40661ef89b3d6b39dd8574015a15b92 Windows: Avoid deadlock on VolumeCB->VolumeLock AFSPrimaryVolumeWorkerThread() holds VolumeCB->VolumeLock SHARED across the call to AFSCleanupFcb() -> CcPurgeCacheSection(). If a filter driver such as Sophos (savonaccessfilter.sys -> savonaccesscontrol.sys) triggers an AFSCreate() in response to the cache section being purged that will force the evaluation of the file path by AFSLocateNameEntry(). If the path contains a mount point that requires validation, AFSBuildMountPointTarget() is called which in turn required the VolumeCB->VolumeLock EXCL. AFSBuildMountPointTarget() only requires the VolumeCB->VolumeLock if the VolumeCB->RootFcb == NULL. That should only be true if the VolumeCB was allocated by AFSInitVolume() or under very rare race conditions. This patchset refactors AFSInitVolume() to ensure that it holds an extra VolumeCB->VolumeReferenceCount reference. This reference is used to assist in the refactoring of AFSBuildRootVolume() and AFSBuildMountPointTarget() to avoid races with volume root object invalidation as well as permitting the VolumeCB->VolumeLock to be ignored in the common case. Avoiding the acquisition of VolumeCB->VolumeLock during mount point target evaluation has the additional benefit of reducing lock contention during path evaluation. FIXES 130812 Change-Id: Id9b0dcc2bfd91277d522f3724893b60ce4d947f5 Reviewed-on: http://gerrit.openafs.org/7474 Tested-by: BuildBot Reviewed-by: Peter Scott Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp index 9f1a979..71f90c4 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSFcbSupport.cpp @@ -430,8 +430,6 @@ AFSInitVolume( IN GUID *AuthGroup, AFSAcquireExcl( pVolumeCB->VolumeLock, TRUE); - lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount); - *VolumeCB = pVolumeCB; try_return( ntStatus); @@ -526,10 +524,10 @@ AFSInitVolume( IN GUID *AuthGroup, AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING, AFS_TRACE_LEVEL_VERBOSE, - "AFSInitVolume Initializing count (1) on volume %08lX\n", + "AFSInitVolume Initializing count (2) on volume %08lX\n", pVolumeCB); - pVolumeCB->VolumeReferenceCount = 1; + pVolumeCB->VolumeReferenceCount = 2; AFSAcquireExcl( pVolumeCB->VolumeLock, TRUE); diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp index 0209a3e..a8804f6 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp @@ -7731,6 +7731,7 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit) NTSTATUS ntStatus = STATUS_SUCCESS; AFSDeviceExt *pControlDevExt = NULL; ULONG ulTimeIncrement = 0; + LONG lCount; __Enter { @@ -7817,6 +7818,8 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit) "AFSInitializeLibrary AFSInitRootFcb failure %08lX\n", ntStatus); + lCount = InterlockedDecrement( &AFSGlobalRoot->VolumeReferenceCount); + AFSReleaseResource( AFSGlobalRoot->VolumeLock); try_return( ntStatus); @@ -7843,6 +7846,8 @@ AFSInitializeLibrary( IN AFSLibraryInitCB *LibraryInit) AFSInitVolumeWorker( AFSGlobalRoot); + lCount = InterlockedDecrement( &AFSGlobalRoot->VolumeReferenceCount); + AFSReleaseResource( AFSGlobalRoot->VolumeLock); AFSReleaseResource( AFSGlobalRoot->ObjectInformation.Fcb->Header.Resource); diff --git a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp index 743c3c5..43c0637 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSNameSupport.cpp @@ -4067,6 +4067,7 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup, AFSVolumeCB *pVolumeCB = NULL; AFSFileID stTargetFileID; LONG lCount; + BOOLEAN bReleaseVolumeLock = FALSE; __Enter { @@ -4221,40 +4222,46 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup, try_return( ntStatus); } - } - else - { // - // Check if this volume has been deleted or taken offline + // pVolumeCB->VolumeLock held exclusive and + // pVolumeCB->VolumeReferenceCount has been incremented + // pVolumeCB->RootFcb == NULL // - if( BooleanFlagOn( pVolumeCB->Flags, AFS_VOLUME_FLAGS_OFFLINE)) - { - - AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock); - - try_return( ntStatus = STATUS_FILE_IS_OFFLINE); - } + bReleaseVolumeLock = TRUE; + } + else + { // - // Just to ensure that things don't get torn down AND we don't create a - // deadlock with invalidation + // AFSInitVolume returns with a VolumeReferenceCount + // obtain one to match // lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount); - AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock); - - AFSAcquireExcl( pVolumeCB->VolumeLock, - TRUE); + AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSBuildMountPointTarget Increment count on volume %08lX Cnt %d\n", + pVolumeCB, + lCount); - lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount); + AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock); } if( pVolumeCB->RootFcb == NULL) { + if ( bReleaseVolumeLock == FALSE) + { + + AFSAcquireExcl( pVolumeCB->VolumeLock, + TRUE); + + bReleaseVolumeLock = TRUE; + } + // // Initialize the root fcb for this volume // @@ -4265,6 +4272,8 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup, if( !NT_SUCCESS( ntStatus)) { + lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount); + AFSReleaseResource( pVolumeCB->VolumeLock); try_return( ntStatus); @@ -4277,6 +4286,12 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup, AFSReleaseResource( &pVolumeCB->RootFcb->NPFcb->Resource); } + if ( bReleaseVolumeLock == TRUE) + { + + AFSReleaseResource( pVolumeCB->VolumeLock); + } + AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, AFS_TRACE_LEVEL_VERBOSE_2, "AFSBuildMountPointTarget Evaluated target of %wZ FID %08lX-%08lX-%08lX-%08lX as root volume\n", @@ -4286,16 +4301,6 @@ AFSBuildMountPointTarget( IN GUID *AuthGroup, pVolumeCB->ObjectInformation.FileId.Vnode, pVolumeCB->ObjectInformation.FileId.Unique); - lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount); - - AFSReleaseResource( pVolumeCB->VolumeLock); - - AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING, - AFS_TRACE_LEVEL_VERBOSE, - "AFSBuildMountPointTarget Increment count on volume %08lX Cnt %d\n", - pVolumeCB, - lCount); - *TargetVolumeCB = pVolumeCB; try_exit: @@ -4323,6 +4328,7 @@ AFSBuildRootVolume( IN GUID *AuthGroup, ULONGLONG ullIndex = 0; AFSVolumeCB *pVolumeCB = NULL; LONG lCount; + BOOLEAN bReleaseVolumeLock = FALSE; __Enter { @@ -4390,29 +4396,47 @@ AFSBuildRootVolume( IN GUID *AuthGroup, try_return( ntStatus); } + + // + // pVolumeCB->VolumeLock is held exclusive + // pVolumeCB->VolumeReferenceCount has been incremented + // pVolumeCB->RootFcb == NULL + // + + bReleaseVolumeLock = TRUE; } else { // - // Just to ensure that things don't get torn down AND we don't create a - // deadlock with invalidation + // AFSInitVolume returns with a VolumeReferenceCount + // obtain one to match // lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount); - AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock); - - AFSAcquireExcl( pVolumeCB->VolumeLock, - TRUE); + AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSBuildRootVolume Increment count on volume %08lX Cnt %d\n", + pVolumeCB, + lCount); - lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount); + AFSReleaseResource( &pDevExt->Specific.RDR.VolumeTreeLock); } if( pVolumeCB->RootFcb == NULL) { + if ( bReleaseVolumeLock == FALSE) + { + + AFSAcquireExcl( pVolumeCB->VolumeLock, + TRUE); + + bReleaseVolumeLock = TRUE; + } + // // Initialize the root fcb for this volume // @@ -4423,6 +4447,8 @@ AFSBuildRootVolume( IN GUID *AuthGroup, if( !NT_SUCCESS( ntStatus)) { + lCount = InterlockedDecrement( &pVolumeCB->VolumeReferenceCount); + AFSReleaseResource( pVolumeCB->VolumeLock); try_return( ntStatus); @@ -4435,6 +4461,12 @@ AFSBuildRootVolume( IN GUID *AuthGroup, AFSReleaseResource( &pVolumeCB->RootFcb->NPFcb->Resource); } + if ( bReleaseVolumeLock == TRUE) + { + + AFSReleaseResource( pVolumeCB->VolumeLock); + } + AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING, AFS_TRACE_LEVEL_VERBOSE_2, "AFSBuildRootVolume Evaluated target of %wZ FID %08lX-%08lX-%08lX-%08lX as root volume\n", @@ -4444,16 +4476,6 @@ AFSBuildRootVolume( IN GUID *AuthGroup, pVolumeCB->ObjectInformation.FileId.Vnode, pVolumeCB->ObjectInformation.FileId.Unique); - lCount = InterlockedIncrement( &pVolumeCB->VolumeReferenceCount); - - AFSReleaseResource( pVolumeCB->VolumeLock); - - AFSDbgLogMsg( AFS_SUBSYSTEM_VOLUME_REF_COUNTING, - AFS_TRACE_LEVEL_VERBOSE, - "AFSBuildRootVolume Increment count on volume %08lX Cnt %d\n", - pVolumeCB, - lCount); - *TargetVolumeCB = pVolumeCB; try_exit: