Windows: Hold Fcb Resource across CcPurgeSection
authorJeffrey Altman <jaltman@your-file-system.com>
Mon, 26 Aug 2013 00:07:44 +0000 (20:07 -0400)
committerJeffrey Altman <jaltman@your-file-system.com>
Fri, 30 Aug 2013 19:12:34 +0000 (12:12 -0700)
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 <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com>

src/WINNT/afsrdr/kernel/lib/AFSCleanup.cpp
src/WINNT/afsrdr/kernel/lib/AFSFileInfo.cpp
src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp

index 493fefe..27e301f 100644 (file)
@@ -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
                 //
index 8a02f74..470aefa 100644 (file)
@@ -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)
                 {
 
index 668d420..e623623 100644 (file)
@@ -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;
                             }
                         }