Windows: Refactor AFSValidateEntry

Refactor AFSValidateEntry to avoid obtaining the
ObjectInformation->Fcb->Resource when it isn't necessary.
This will avoid contention and improve performance.

The only time that the Fcb->Resource is required is when
the object requires verification, the object is a FILE,
and the object was successfully evaluated.

Even with this reorganization there is a small window
of opportunity for a deadlock to occur if a CcPurgeCacheSection()
which is called with the Fcb->Resource held triggers a filter
driver to issue a CreateFile and in between the two operations
an invalidate object is received.

Change-Id: Iddf64f030c6a608ac29a10b7a9a7e130ae6c4249
Reviewed-on: http://gerrit.openafs.org/7143
Reviewed-by: Derrick Brashear <shadow@dementix.org>
Tested-by: Derrick Brashear <shadow@dementix.org>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
index 5867625..6e0e94c 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSCreate.cpp
@@ -3247,8 +3247,6 @@ AFSProcessOverwriteSupersede( IN PDEVICE_OBJECT DeviceObject,
 
         KeQuerySystemTime( &pObjectInfo->LastAccessTime);
 
-        //KeQuerySystemTime( &pObjectInfo->CreationTime);
-
         KeQuerySystemTime( &pObjectInfo->LastWriteTime);
 
         ntStatus = AFSUpdateFileInformation( &pParentObjectInfo->FileId,

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
index add53f1..8c3ea26 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
@@ -3756,28 +3756,6 @@ AFSValidateEntry( IN AFSDirectoryCB *DirEntry,
             try_return( ntStatus);
         }
 
-        if( PurgeContent &&
-            pObjectInfo->Fcb != NULL)
-        {
-
-            pCurrentFcb = pObjectInfo->Fcb;
-
-            if( !ExIsResourceAcquiredLite( &pCurrentFcb->NPFcb->Resource))
-            {
-
-                AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING,
-                              AFS_TRACE_LEVEL_VERBOSE,
-                              "AFSValidateEntry Acquiring Fcb lock %08lX EXCL %08lX\n",
-                              &pCurrentFcb->NPFcb->Resource,
-                              PsGetCurrentThread());
-
-                AFSAcquireExcl( &pCurrentFcb->NPFcb->Resource,
-                                TRUE);
-
-                bReleaseFcb = TRUE;
-            }
-        }
-
         //
         // This routine ensures that the current entry is valid by:
         //
@@ -3905,109 +3883,120 @@ AFSValidateEntry( IN AFSDirectoryCB *DirEntry,
                 // Can't hold the Fcb resource while doing this
                 //
 
-                if( pCurrentFcb != NULL &&
+                if( PurgeContent &&
+                    pObjectInfo->Fcb != NULL &&
                     (pObjectInfo->DataVersion.QuadPart != pDirEnumEntry->DataVersion.QuadPart ||
                     BooleanFlagOn( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA)))
                 {
 
-                    IO_STATUS_BLOCK stIoStatus;
-                    BOOLEAN bPurgeExtents = FALSE;
+                    pCurrentFcb = pObjectInfo->Fcb;
 
-                    AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
-                                  AFS_TRACE_LEVEL_VERBOSE_2,
-                                  "AFSValidateEntry Flush/purge entry %wZ FID %08lX-%08lX-%08lX-%08lX\n",
-                                  &DirEntry->NameInformation.FileName,
-                                  pObjectInfo->FileId.Cell,
-                                  pObjectInfo->FileId.Volume,
-                                  pObjectInfo->FileId.Vnode,
-                                  pObjectInfo->FileId.Unique);
+                    if( !ExIsResourceAcquiredLite( &pCurrentFcb->NPFcb->Resource))
+                    {
+
+                        AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING,
+                                      AFS_TRACE_LEVEL_VERBOSE,
+                                      "AFSValidateEntry Acquiring Fcb lock %08lX EXCL %08lX\n",
+                                      &pCurrentFcb->NPFcb->Resource,
+                                      PsGetCurrentThread());
+
+                        AFSAcquireExcl( &pCurrentFcb->NPFcb->Resource,
+                                        TRUE);
 
-                    if ( BooleanFlagOn( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA))
+                        bReleaseFcb = TRUE;
+                    }
+
+                    if( pCurrentFcb != NULL)
                     {
-                        bPurgeExtents = TRUE;
+
+                        IO_STATUS_BLOCK stIoStatus;
+                        BOOLEAN bPurgeExtents = FALSE;
 
                         AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
-                                      AFS_TRACE_LEVEL_VERBOSE,
-                                      "AFSVerifyEntry Clearing VERIFY_DATA flag %wZ FID %08lX-%08lX-%08lX-%08lX\n",
+                                      AFS_TRACE_LEVEL_VERBOSE_2,
+                                      "AFSValidateEntry Flush/purge entry %wZ FID %08lX-%08lX-%08lX-%08lX\n",
                                       &DirEntry->NameInformation.FileName,
                                       pObjectInfo->FileId.Cell,
                                       pObjectInfo->FileId.Volume,
                                       pObjectInfo->FileId.Vnode,
                                       pObjectInfo->FileId.Unique);
 
-                        ClearFlag( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA);
-                    }
+                        if ( BooleanFlagOn( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA))
+                        {
+                            bPurgeExtents = TRUE;
 
-                    __try
-                    {
+                            AFSDbgLogMsg( AFS_SUBSYSTEM_FILE_PROCESSING,
+                                          AFS_TRACE_LEVEL_VERBOSE,
+                                          "AFSVerifyEntry Clearing VERIFY_DATA flag %wZ FID %08lX-%08lX-%08lX-%08lX\n",
+                                          &DirEntry->NameInformation.FileName,
+                                          pObjectInfo->FileId.Cell,
+                                          pObjectInfo->FileId.Volume,
+                                          pObjectInfo->FileId.Vnode,
+                                          pObjectInfo->FileId.Unique);
 
-                        CcFlushCache( &pCurrentFcb->NPFcb->SectionObjectPointers,
-                                      NULL,
-                                      0,
-                                      &stIoStatus);
+                            ClearFlag( pObjectInfo->Flags, AFS_OBJECT_FLAGS_VERIFY_DATA);
+                        }
 
-                        if( !NT_SUCCESS( stIoStatus.Status))
+                        __try
                         {
 
+                            CcFlushCache( &pCurrentFcb->NPFcb->SectionObjectPointers,
+                                          NULL,
+                                          0,
+                                          &stIoStatus);
+
+                            if( !NT_SUCCESS( stIoStatus.Status))
+                            {
+
+                                AFSDbgLogMsg( AFS_SUBSYSTEM_IO_PROCESSING,
+                                              AFS_TRACE_LEVEL_ERROR,
+                                              "AFSValidateEntry CcFlushCache failure %wZ FID %08lX-%08lX-%08lX-%08lX Status 0x%08lX Bytes 0x%08lX\n",
+                                              &DirEntry->NameInformation.FileName,
+                                              pObjectInfo->FileId.Cell,
+                                              pObjectInfo->FileId.Volume,
+                                              pObjectInfo->FileId.Vnode,
+                                              pObjectInfo->FileId.Unique,
+                                              stIoStatus.Status,
+                                              stIoStatus.Information);
+
+                                ntStatus = stIoStatus.Status;
+                            }
+
+                            if ( bPurgeExtents)
+                            {
+
+                                CcPurgeCacheSection( &pObjectInfo->Fcb->NPFcb->SectionObjectPointers,
+                                                     NULL,
+                                                     0,
+                                                     FALSE);
+                            }
+                        }
+                        __except( EXCEPTION_EXECUTE_HANDLER)
+                        {
+                            ntStatus = GetExceptionCode();
+
                             AFSDbgLogMsg( AFS_SUBSYSTEM_IO_PROCESSING,
                                           AFS_TRACE_LEVEL_ERROR,
-                                          "AFSValidateEntry CcFlushCache failure %wZ FID %08lX-%08lX-%08lX-%08lX Status 0x%08lX Bytes 0x%08lX\n",
+                                          "AFSValidateEntry CcFlushCache or CcPurgeCacheSection exception %wZ FID %08lX-%08lX-%08lX-%08lX Status 0x%08lX\n",
                                           &DirEntry->NameInformation.FileName,
                                           pObjectInfo->FileId.Cell,
                                           pObjectInfo->FileId.Volume,
                                           pObjectInfo->FileId.Vnode,
                                           pObjectInfo->FileId.Unique,
-                                          stIoStatus.Status,
-                                          stIoStatus.Information);
+                                          ntStatus);
 
-                            ntStatus = stIoStatus.Status;
                         }
 
+                        AFSReleaseResource( &pCurrentFcb->NPFcb->Resource);
+
+                        bReleaseFcb = FALSE;
+
                         if ( bPurgeExtents)
                         {
-
-                            CcPurgeCacheSection( &pObjectInfo->Fcb->NPFcb->SectionObjectPointers,
-                                                 NULL,
-                                                 0,
-                                                 FALSE);
+                            AFSFlushExtents( pCurrentFcb,
+                                             AuthGroup);
                         }
                     }
-                    __except( EXCEPTION_EXECUTE_HANDLER)
-                    {
-                        ntStatus = GetExceptionCode();
-
-                        AFSDbgLogMsg( AFS_SUBSYSTEM_IO_PROCESSING,
-                                      AFS_TRACE_LEVEL_ERROR,
-                                      "AFSValidateEntry CcFlushCache or CcPurgeCacheSection exception %wZ FID %08lX-%08lX-%08lX-%08lX Status 0x%08lX\n",
-                                      &DirEntry->NameInformation.FileName,
-                                      pObjectInfo->FileId.Cell,
-                                      pObjectInfo->FileId.Volume,
-                                      pObjectInfo->FileId.Vnode,
-                                      pObjectInfo->FileId.Unique,
-                                      ntStatus);
-
-                    }
-
-                    AFSReleaseResource( &pCurrentFcb->NPFcb->Resource);
-
-                    if ( bPurgeExtents)
-                    {
-                        AFSFlushExtents( pCurrentFcb,
-                                         AuthGroup);
-                    }
-
-                    //
-                    // Reacquire the Fcb to purge the cache
-                    //
-
-                    AFSDbgLogMsg( AFS_SUBSYSTEM_LOCK_PROCESSING,
-                                  AFS_TRACE_LEVEL_VERBOSE,
-                                  "AFSValidateEntry Acquiring Fcb lock %08lX EXCL %08lX\n",
-                                  &pCurrentFcb->NPFcb->Resource,
-                                  PsGetCurrentThread());
-
-                    AFSAcquireExcl( &pCurrentFcb->NPFcb->Resource,
-                                    TRUE);
                 }
 
                 //