From 27311ca420c2ee29b38aa2994993cf24d7d769b1 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Sun, 25 Aug 2013 20:07:44 -0400 Subject: [PATCH] Windows: Hold Fcb Resource across CcPurgeSection Now that the Fcb Resource and SectionObjectResource are held in the FastIo pathway and the Trend Micro deadlock has been addressed by holding a reference on the FileObject it is time to fix the lock acquisition ordering. For each CcPurgeSection call the Fcb Resource will be held exclusive before the SectionObjectResource. Change-Id: Ica9e3674b39e2789c35bcf13d9fa1f2326420119 Reviewed-on: http://gerrit.openafs.org/10192 Tested-by: BuildBot Reviewed-by: Jeffrey Altman --- src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp | 19 ++--- src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp | 17 +++++ src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp | 103 ++++++++++++++++++++++------ 3 files changed, 110 insertions(+), 29 deletions(-) diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp index 493fefe..27e301f 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp @@ -234,6 +234,16 @@ AFSCleanup( IN PDEVICE_OBJECT LibDeviceObject, // We may be performing some cleanup on the Fcb so grab it exclusive to ensure no collisions // + + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSCleanup Acquiring Fcb lock %p EXCL %08lX\n", + &pFcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSAcquireExcl( &pFcb->NPFcb->Resource, + TRUE); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING|AFS_SUBSYSTEM_SECTION_OBJECT, AFS_TRACE_LEVEL_VERBOSE, "AFSCleanup Acquiring Fcb SectionObject lock %p EXCL %08lX\n", @@ -343,15 +353,6 @@ AFSCleanup( IN PDEVICE_OBJECT LibDeviceObject, AFSReleaseResource( &pFcb->NPFcb->SectionObjectResource); - AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, - AFS_TRACE_LEVEL_VERBOSE, - "AFSCleanup Acquiring Fcb lock %p EXCL %08lX\n", - &pFcb->NPFcb->Resource, - PsGetCurrentThread())); - - AFSAcquireExcl( &pFcb->NPFcb->Resource, - TRUE); - // // Unlock all outstanding locks on the file, again, unconditionally // diff --git a/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp b/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp index 8a02f74..470aefa 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp @@ -2201,6 +2201,15 @@ AFSSetDispositionInfo( IN PIRP Irp, { BOOLEAN bMmFlushed; + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSSetDispositionInfo Acquiring Fcb lock %p EXCL %08lX\n", + &pFcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSAcquireExcl( &pFcb->NPFcb->Resource, + TRUE); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING|AFS_SUBSYSTEM_SECTION_OBJECT, AFS_TRACE_LEVEL_VERBOSE, "AFSSetDispositionInfo Acquiring Fcb SectionObject lock %p EXCL %08lX\n", @@ -2280,6 +2289,14 @@ AFSSetDispositionInfo( IN PIRP Irp, AFSReleaseResource( &pFcb->NPFcb->SectionObjectResource); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSSetDispositionInfo Releasing Fcb lock %p EXCL %08lX\n", + &pFcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSReleaseResource( &pFcb->NPFcb->Resource); + if ( !bMmFlushed) { diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp index 668d420..e623623 100644 --- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp +++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp @@ -1870,6 +1870,15 @@ AFSInvalidateObject( IN OUT AFSObjectInfoCB **ppObjectInfo, (*ppObjectInfo)->FileId.Vnode, (*ppObjectInfo)->FileId.Unique)); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSInvalidateObject Flush/purge Acquiring Fcb lock %p EXCL %08lX\n", + &(*ppObjectInfo)->Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSAcquireExcl( &(*ppObjectInfo)->Fcb->NPFcb->Resource, + TRUE); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING|AFS_SUBSYSTEM_SECTION_OBJECT, AFS_TRACE_LEVEL_VERBOSE, "AFSInvalidateObject Flush/purge Acquiring Fcb SectionObject lock %p EXCL %08lX\n", @@ -1950,6 +1959,14 @@ AFSInvalidateObject( IN OUT AFSObjectInfoCB **ppObjectInfo, AFSReleaseResource( &(*ppObjectInfo)->Fcb->NPFcb->SectionObjectResource); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSInvalidateObject Flush/purge Releasing Fcb lock %p EXCL %08lX\n", + &(*ppObjectInfo)->Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSReleaseResource( &(*ppObjectInfo)->Fcb->NPFcb->Resource); + // // Clear out the extents // Get rid of them (note this involves waiting @@ -3050,6 +3067,15 @@ AFSVerifyEntry( IN GUID *AuthGroup, pObjectInfo->FileId.Vnode, pObjectInfo->FileId.Unique)); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSVerifyEntry Acquiring Fcb lock %p EXCL %08lX\n", + &pObjectInfo->Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSAcquireExcl( &pObjectInfo->Fcb->NPFcb->Resource, + TRUE); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING|AFS_SUBSYSTEM_SECTION_OBJECT, AFS_TRACE_LEVEL_VERBOSE, "AFSVerifyEntry Acquiring Fcb SectionObject lock %p EXCL %08lX\n", @@ -3131,6 +3157,14 @@ AFSVerifyEntry( IN GUID *AuthGroup, AFSReleaseResource( &pObjectInfo->Fcb->NPFcb->SectionObjectResource); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSVerifyEntry Releasing Fcb lock %p EXCL %08lX\n", + &pObjectInfo->Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSReleaseResource( &pObjectInfo->Fcb->NPFcb->Resource); + AFSFlushExtents( pObjectInfo->Fcb, AuthGroup); @@ -4214,14 +4248,6 @@ AFSValidateEntry( IN AFSDirectoryCB *DirEntry, AFSAcquireExcl( &pCurrentFcb->NPFcb->SectionObjectResource, TRUE); - // - // Release Fcb->Resource to avoid Trend Micro deadlock - // - - AFSReleaseResource( &pCurrentFcb->NPFcb->Resource); - - bReleaseFcb = FALSE; - __try { @@ -4305,7 +4331,10 @@ AFSValidateEntry( IN AFSDirectoryCB *DirEntry, SetFlag( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA); } + } + if (bReleaseFcb) + { AFSReleaseResource( &pCurrentFcb->NPFcb->Resource); bReleaseFcb = FALSE; @@ -6865,11 +6894,11 @@ AFSCleanupFcb( IN AFSFcb *Fcb, AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, - "AFSCleanupEntry Acquiring Fcb lock %p SHARED %08lX\n", + "AFSCleanupEntry Acquiring Fcb lock %p EXCL %08lX\n", &Fcb->NPFcb->Resource, PsGetCurrentThread())); - AFSAcquireShared( &Fcb->NPFcb->Resource, + AFSAcquireExcl( &Fcb->NPFcb->Resource, TRUE); if( Fcb->OpenReferenceCount > 0) @@ -6957,7 +6986,7 @@ AFSCleanupFcb( IN AFSFcb *Fcb, AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, AFS_TRACE_LEVEL_VERBOSE, - "AFSCleanupEntry Releasing Fcb lock %p SHARED %08lX\n", + "AFSCleanupEntry Releasing Fcb lock %p EXCL %08lX\n", &Fcb->NPFcb->Resource, PsGetCurrentThread())); @@ -7041,6 +7070,24 @@ AFSCleanupFcb( IN AFSFcb *Fcb, (AFS_SERVER_PURGE_SLEEP * pControlDeviceExt->Specific.Control.FcbPurgeTimeCount.QuadPart)))) { + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSCleanupFcb Acquiring Fcb lock %p EXCL %08lX\n", + &Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + if ( AFSAcquireExcl( &Fcb->NPFcb->Resource, ForceFlush) == FALSE) + { + + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSCleanupFcb Failed to Acquire Fcb lock %p EXCL %08lX\n", + &Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + try_return( ntStatus = STATUS_RETRY); + } + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING|AFS_SUBSYSTEM_SECTION_OBJECT, AFS_TRACE_LEVEL_VERBOSE, "AFSCleanupFcb Acquiring Fcb SectionObject lock %p EXCL %08lX\n", @@ -7119,6 +7166,14 @@ AFSCleanupFcb( IN AFSFcb *Fcb, AFSReleaseResource( &Fcb->NPFcb->SectionObjectResource); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSCleanupFcb Releasing Fcb lock %p EXCL %08lX\n", + &Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSReleaseResource( &Fcb->NPFcb->Resource); + if( Fcb->OpenReferenceCount <= 0) { @@ -7139,6 +7194,14 @@ AFSCleanupFcb( IN AFSFcb *Fcb, &Fcb->NPFcb->SectionObjectResource, PsGetCurrentThread())); + AFSDbgTrace(( AFS_SUBSYSTEM_LOCK_PROCESSING, + AFS_TRACE_LEVEL_VERBOSE, + "AFSCleanupFcb Releasing Fcb lock %p EXCL %08lX\n", + &Fcb->NPFcb->Resource, + PsGetCurrentThread())); + + AFSReleaseResource( &Fcb->NPFcb->Resource); + ntStatus = STATUS_RETRY; } } @@ -9118,7 +9181,7 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo, AFSAcquireExcl( &ObjectInfo->Fcb->NPFcb->SectionObjectResource, TRUE); - __try + __try { if( ObjectInfo->Fcb->NPFcb->SectionObjectPointers.DataSectionObject != NULL && @@ -9257,10 +9320,6 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo, AFSAcquireExcl( &ObjectInfo->Fcb->NPFcb->SectionObjectResource, TRUE); - AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->Resource); - - bLocked = FALSE; - __try { @@ -9311,6 +9370,10 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo, PsGetCurrentThread())); AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->SectionObjectResource); + + AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->Resource); + + bLocked = FALSE; } } else @@ -9329,10 +9392,6 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo, AFSAcquireExcl( &ObjectInfo->Fcb->NPFcb->SectionObjectResource, TRUE); - AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->Resource); - - bLocked = FALSE; - // // Must build a list of non-dirty ranges from the beginning of the file // to the end. There can be at most (Fcb->Specific.File.ExtentsDirtyCount + 1) @@ -9551,6 +9610,10 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo, PsGetCurrentThread())); AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->SectionObjectResource); + + AFSReleaseResource( &ObjectInfo->Fcb->NPFcb->Resource); + + bLocked = FALSE; } } -- 1.9.4