Windows: AFSTearDownExtents may experience active extents

If there are extents with a non-zero ActiveCount when AFSTearDownExtents()
is executed, it must leave them alone and attached to the File Control
Block.  This has implications for its callers, especially AFSCleanupFcb()
since it may be the case that a Cleanup cannot be completed.

The AFSPrimaryVolumeWorker thread must therefore check after calling
AFSCleanupFcb() whether or not the Fcb ExtentCount is zero before
calling AFSRemoveFcb().

Change-Id: I164dbe24d2bfe69aba0fcb5d845f66415d5bb0c3
Reviewed-on: http://gerrit.openafs.org/7406
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Jeffrey Altman <jaltman@secure-endpoints.com>
Tested-by: Jeffrey Altman <jaltman@secure-endpoints.com>

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp b/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp
index 62bcb21..08b5a02 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp
@@ -487,25 +487,8 @@ AFSClose( IN PDEVICE_OBJECT LibDeviceObject,
                     // Tear 'em down, we'll not be needing them again
                     //
 
-                    if( AFSTearDownFcbExtents( pFcb,
-                                               &pCcb->AuthGroup))
-                    {
-
-                        //
-                        // Indicate to the service that the file required complete flushing to the
-                        // server.
-                        //
-
-                        AFSProcessRequest( AFS_REQUEST_TYPE_FLUSH_FILE,
-                                           AFS_REQUEST_FLAG_SYNCHRONOUS,
-                                           &pCcb->AuthGroup,
-                                           NULL,
-                                           &pFcb->ObjectInformation->FileId,
-                                           NULL,
-                                           0,
-                                           NULL,
-                                           NULL);
-                    }
+                    AFSTearDownFcbExtents( pFcb,
+                                           &pCcb->AuthGroup);
                 }
                 else
                 {

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSExtentsSupport.cpp b/src/WINNT/afsrdr/kernel/lib/AFSExtentsSupport.cpp
index 63f2f95..705ae6c 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSExtentsSupport.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSExtentsSupport.cpp
@@ -102,17 +102,25 @@ AFSLockForExtentsTrimNoWait( IN AFSFcb *Fcb)
     return TRUE;
 }
 //
-// Pull all the extents away from the FCB.
+// AFSTearDownFcbExtents was originally written to
+// remove all of the extents from an FCB.  For that to happen
+// it must be an invariant that the extent list cannot change
+// from the moment the caller decides to execute AFSTearDownFcbExtents
+// until it returns.  This invariant does not hold because the
+// the decision to call AFSTearDownFcbExtents is made without
+// holding the ExtentsResource and it is possible that extents
+// are in active use. Therefore, AFSTearDownFcbExtents now releases
+// as many non-active extents as it can.
 //
-BOOLEAN
+VOID
 AFSTearDownFcbExtents( IN AFSFcb *Fcb,
                        IN GUID *AuthGroup)
 {
-    BOOLEAN             bFoundExtents = FALSE;
     AFSNonPagedFcb      *pNPFcb = Fcb->NPFcb;
-    LIST_ENTRY          *le;
+    LIST_ENTRY          *le, *leNext;
     AFSExtent           *pEntry;
-    ULONG                ulCount = 0, ulReleaseCount = 0, ulProcessCount = 0;
+    LONG                 lExtentCount = 0;
+    ULONG                ulReleaseCount = 0, ulProcessCount = 0;
     size_t               sz;
     AFSReleaseExtentsCB *pRelease = NULL;
     BOOLEAN              locked = FALSE;
@@ -172,63 +180,43 @@ AFSTearDownFcbExtents( IN AFSFcb *Fcb,
             try_return ( ntStatus = STATUS_INSUFFICIENT_RESOURCES );
         }
 
-        le = Fcb->Specific.File.ExtentsLists[AFS_EXTENTS_LIST].Flink;
-
-        ulCount = Fcb->Specific.File.ExtentCount;
+        AFSAcquireExcl( &pNPFcb->Specific.File.DirtyExtentsListLock,
+                        TRUE);
 
-        while( ulReleaseCount < ulCount)
+        for( le = Fcb->Specific.File.ExtentsLists[AFS_EXTENTS_LIST].Flink,
+             lExtentCount = 0;
+             lExtentCount < Fcb->Specific.File.ExtentCount;
+             lExtentCount += ulProcessCount)
         {
 
-            bFoundExtents = TRUE;
-
             RtlZeroMemory( pRelease,
                            sizeof( AFSReleaseExtentsCB ) +
                            (AFS_MAXIMUM_EXTENT_RELEASE_COUNT * sizeof ( AFSFileExtentCB )));
 
-            if( ulCount - ulReleaseCount <= AFS_MAXIMUM_EXTENT_RELEASE_COUNT)
-            {
-                ulProcessCount = ulCount - ulReleaseCount;
-            }
-            else
+            for( ulProcessCount = 0, ulReleaseCount = 0;
+                 !IsListEmpty( le) && ulReleaseCount < AFS_MAXIMUM_EXTENT_RELEASE_COUNT;
+                 ulProcessCount++, le = leNext)
             {
-                ulProcessCount = AFS_MAXIMUM_EXTENT_RELEASE_COUNT;
-            }
-
-            pRelease->Flags = AFS_EXTENT_FLAG_RELEASE;
-            pRelease->ExtentCount = ulProcessCount;
 
-            //
-            // Update the metadata for this call
-            //
+                leNext = le->Flink;
 
-            pRelease->AllocationSize = Fcb->ObjectInformation->EndOfFile;
-            pRelease->CreateTime = Fcb->ObjectInformation->CreationTime;
-            pRelease->ChangeTime = Fcb->ObjectInformation->ChangeTime;
-            pRelease->LastAccessTime = Fcb->ObjectInformation->LastAccessTime;
-            pRelease->LastWriteTime = Fcb->ObjectInformation->LastWriteTime;
+                pEntry = ExtentFor( le, AFS_EXTENTS_LIST );
 
-            ulProcessCount = 0;
+                if( pEntry->ActiveCount == 0)
+                {
 
-            while (ulProcessCount < pRelease->ExtentCount)
-            {
-                pEntry = ExtentFor( le, AFS_EXTENTS_LIST );
+                    ulReleaseCount++;
 
-                pRelease->FileExtents[ulProcessCount].Flags = AFS_EXTENT_FLAG_RELEASE;
+                    pRelease->FileExtents[ulProcessCount].Flags = AFS_EXTENT_FLAG_RELEASE;
 
 #if GEN_MD5
-                RtlCopyMemory( pRelease->FileExtents[ulProcessCount].MD5,
-                               pEntry->MD5,
-                               sizeof(pEntry->MD5));
+                    RtlCopyMemory( pRelease->FileExtents[ulProcessCount].MD5,
+                                   pEntry->MD5,
+                                   sizeof(pEntry->MD5));
 
-                pRelease->FileExtents[ulProcessCount].Flags |= AFS_EXTENT_FLAG_MD5_SET;
+                    pRelease->FileExtents[ulProcessCount].Flags |= AFS_EXTENT_FLAG_MD5_SET;
 #endif
 
-                if( BooleanFlagOn( pEntry->Flags, AFS_EXTENT_DIRTY))
-                {
-
-                    AFSAcquireExcl( &pNPFcb->Specific.File.DirtyExtentsListLock,
-                                    TRUE);
-
                     if( BooleanFlagOn( pEntry->Flags, AFS_EXTENT_DIRTY))
                     {
 
@@ -244,115 +232,131 @@ AFSTearDownFcbExtents( IN AFSFcb *Fcb,
                         ASSERT( dirtyCount >= 0);
                     }
 
-                    AFSReleaseResource( &pNPFcb->Specific.File.DirtyExtentsListLock);
-                }
+                    AFSDbgLogMsg( AFS_SUBSYSTEM_EXTENT_PROCESSING,
+                                  AFS_TRACE_LEVEL_VERBOSE,
+                                  "AFSTearDownFcbExtents Releasing extent %p fid %08lX-%08lX-%08lX-%08lX Offset %08lX-%08lX Len %08lX\n",
+                                  pEntry,
+                                  Fcb->ObjectInformation->FileId.Cell,
+                                  Fcb->ObjectInformation->FileId.Volume,
+                                  Fcb->ObjectInformation->FileId.Vnode,
+                                  Fcb->ObjectInformation->FileId.Unique,
+                                  pEntry->FileOffset.HighPart,
+                                  pEntry->FileOffset.LowPart,
+                                  pEntry->Size);
 
-                AFSDbgLogMsg( AFS_SUBSYSTEM_EXTENT_PROCESSING,
-                              AFS_TRACE_LEVEL_VERBOSE,
-                              "AFSTearDownFcbExtents Releasing extent %p fid %08lX-%08lX-%08lX-%08lX Offset %08lX-%08lX Len %08lX\n",
-                              pEntry,
-                              Fcb->ObjectInformation->FileId.Cell,
-                              Fcb->ObjectInformation->FileId.Volume,
-                              Fcb->ObjectInformation->FileId.Vnode,
-                              Fcb->ObjectInformation->FileId.Unique,
-                              pEntry->FileOffset.HighPart,
-                              pEntry->FileOffset.LowPart,
-                              pEntry->Size);
+                    pRelease->FileExtents[ulProcessCount].Length = pEntry->Size;
+                    pRelease->FileExtents[ulProcessCount].DirtyLength = pEntry->Size;
+                    pRelease->FileExtents[ulProcessCount].DirtyOffset = 0;
+                    pRelease->FileExtents[ulProcessCount].CacheOffset = pEntry->CacheOffset;
+                    pRelease->FileExtents[ulProcessCount].FileOffset = pEntry->FileOffset;
 
-                pRelease->FileExtents[ulProcessCount].Length = pEntry->Size;
-                pRelease->FileExtents[ulProcessCount].DirtyLength = pEntry->Size;
-                pRelease->FileExtents[ulProcessCount].DirtyOffset = 0;
-                pRelease->FileExtents[ulProcessCount].CacheOffset = pEntry->CacheOffset;
-                pRelease->FileExtents[ulProcessCount].FileOffset = pEntry->FileOffset;
+                    InterlockedExchangeAdd( &pControlDevExt->Specific.Control.ExtentsHeldLength, -((LONG)(pEntry->Size/1024)));
 
-                InterlockedExchangeAdd( &pControlDevExt->Specific.Control.ExtentsHeldLength, -((LONG)(pEntry->Size/1024)));
+                    InterlockedExchangeAdd( &Fcb->Specific.File.ExtentLength, -((LONG)(pEntry->Size/1024)));
 
-                InterlockedExchangeAdd( &Fcb->Specific.File.ExtentLength, -((LONG)(pEntry->Size/1024)));
+                    RemoveEntryList( le);
 
-                ASSERT( pEntry->ActiveCount == 0);
+                    AFSExFreePool( pEntry);
 
-                ulProcessCount ++;
-                le = le->Flink;
-                AFSExFreePool( pEntry);
+                    lCount = InterlockedDecrement( &Fcb->Specific.File.ExtentCount);
 
-                lCount = InterlockedDecrement( &Fcb->Specific.File.ExtentCount);
-
-                lCount = InterlockedDecrement( &pControlDevExt->Specific.Control.ExtentCount);
+                    lCount = InterlockedDecrement( &pControlDevExt->Specific.Control.ExtentCount);
 
-                if( lCount == 0)
-                {
+                    if( lCount == 0)
+                    {
 
-                    KeSetEvent( &pControlDevExt->Specific.Control.ExtentsHeldEvent,
-                                0,
-                                FALSE);
+                        KeSetEvent( &pControlDevExt->Specific.Control.ExtentsHeldEvent,
+                                    0,
+                                    FALSE);
+                    }
                 }
             }
 
-            //
-            // Send the request down.  We cannot send this down
-            // asynchronously - if we did that we could request them
-            // back before the service got this request and then this
-            // request would be a corruption.
-            //
+            if ( ulReleaseCount > 0)
+            {
 
-            sz = sizeof( AFSReleaseExtentsCB ) + (ulProcessCount * sizeof ( AFSFileExtentCB ));
+                pRelease->ExtentCount = ulReleaseCount;
 
-            ntStatus = AFSProcessRequest( AFS_REQUEST_TYPE_RELEASE_FILE_EXTENTS,
-                                          AFS_REQUEST_FLAG_SYNCHRONOUS,
-                                          pAuthGroup,
-                                          NULL,
-                                          &Fcb->ObjectInformation->FileId,
-                                          pRelease,
-                                          sz,
-                                          NULL,
-                                          NULL);
+                pRelease->Flags = AFS_EXTENT_FLAG_RELEASE;
 
-            if( !NT_SUCCESS(ntStatus))
-            {
+                //
+                // Update the metadata for this call
+                //
+
+                pRelease->AllocationSize = Fcb->ObjectInformation->EndOfFile;
+                pRelease->CreateTime = Fcb->ObjectInformation->CreationTime;
+                pRelease->ChangeTime = Fcb->ObjectInformation->ChangeTime;
+                pRelease->LastAccessTime = Fcb->ObjectInformation->LastAccessTime;
+                pRelease->LastWriteTime = Fcb->ObjectInformation->LastWriteTime;
 
                 //
-                // Regardless of whether or not the AFSProcessRequest() succeeded, the extents
-                // were released (if AFS_EXTENT_FLAG_RELEASE was set).  Log the error so it is known.
+                // Send the request down.  We cannot send this down
+                // asynchronously - if we did that we could request them
+                // back before the service got this request and then this
+                // request would be a corruption.
                 //
 
-                AFSDbgLogMsg( AFS_SUBSYSTEM_EXTENT_PROCESSING,
-                              AFS_TRACE_LEVEL_ERROR,
-                              "AFSTearDownFcbExtents AFS_REQUEST_TYPE_RELEASE_FILE_EXTENTS failed fid %08lX-%08lX-%08lX-%08lX Status %08lX\n",
-                              Fcb->ObjectInformation->FileId.Cell,
-                              Fcb->ObjectInformation->FileId.Volume,
-                              Fcb->ObjectInformation->FileId.Vnode,
-                              Fcb->ObjectInformation->FileId.Unique,
-                              ntStatus);
+                sz = sizeof( AFSReleaseExtentsCB ) + (ulProcessCount * sizeof ( AFSFileExtentCB ));
 
-            }
+                ntStatus = AFSProcessRequest( AFS_REQUEST_TYPE_RELEASE_FILE_EXTENTS,
+                                              AFS_REQUEST_FLAG_SYNCHRONOUS,
+                                              pAuthGroup,
+                                              NULL,
+                                              &Fcb->ObjectInformation->FileId,
+                                              pRelease,
+                                              sz,
+                                              NULL,
+                                              NULL);
 
-            ulReleaseCount += ulProcessCount;
-        }
+                if( !NT_SUCCESS(ntStatus))
+                {
 
-        //
-        // Reinitialize the skip lists
-        //
+                    //
+                    // Regardless of whether or not the AFSProcessRequest() succeeded, the extents
+                    // were released (if AFS_EXTENT_FLAG_RELEASE was set).  Log the error so it is known.
+                    //
 
-        ASSERT( Fcb->Specific.File.ExtentCount == 0);
+                    AFSDbgLogMsg( AFS_SUBSYSTEM_EXTENT_PROCESSING,
+                                  AFS_TRACE_LEVEL_ERROR,
+                                  "AFSTearDownFcbExtents AFS_REQUEST_TYPE_RELEASE_FILE_EXTENTS failed fid %08lX-%08lX-%08lX-%08lX Status %08lX\n",
+                                  Fcb->ObjectInformation->FileId.Cell,
+                                  Fcb->ObjectInformation->FileId.Volume,
+                                  Fcb->ObjectInformation->FileId.Vnode,
+                                  Fcb->ObjectInformation->FileId.Unique,
+                                  ntStatus);
 
-        for (ULONG i = 0; i < AFS_NUM_EXTENT_LISTS; i++)
-        {
-            InitializeListHead(&Fcb->Specific.File.ExtentsLists[i]);
+                }
+            }
         }
 
+        AFSReleaseResource( &pNPFcb->Specific.File.DirtyExtentsListLock);
+
         //
-        // Reinitialize the dirty list as well
+        // if all extents have been released, reinitialize the skip lists
         //
 
-        AFSAcquireExcl( &pNPFcb->Specific.File.DirtyExtentsListLock,
-                        TRUE);
+        if( Fcb->Specific.File.ExtentCount == 0)
+        {
 
-        ASSERT( Fcb->Specific.File.ExtentsDirtyCount == 0);
+            for (ULONG i = 0; i < AFS_NUM_EXTENT_LISTS; i++)
+            {
+                InitializeListHead(&Fcb->Specific.File.ExtentsLists[i]);
+            }
 
-        Fcb->NPFcb->Specific.File.DirtyListHead = NULL;
-        Fcb->NPFcb->Specific.File.DirtyListTail = NULL;
+            //
+            // Reinitialize the dirty list as well
+            //
 
-        AFSReleaseResource( &pNPFcb->Specific.File.DirtyExtentsListLock);
+            AFSAcquireExcl( &pNPFcb->Specific.File.DirtyExtentsListLock,
+                            TRUE);
+
+            ASSERT( Fcb->Specific.File.ExtentsDirtyCount == 0);
+
+            Fcb->NPFcb->Specific.File.DirtyListHead = NULL;
+            Fcb->NPFcb->Specific.File.DirtyListTail = NULL;
+
+            AFSReleaseResource( &pNPFcb->Specific.File.DirtyExtentsListLock);
+        }
 
         Fcb->NPFcb->Specific.File.ExtentsRequestStatus = STATUS_SUCCESS;
 
@@ -376,8 +380,6 @@ try_exit:
             AFSExFreePool( pRelease);
         }
     }
-
-    return bFoundExtents;
 }
 
 static PAFSExtent

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
index 72b0a78..0209a3e 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSGeneric.cpp
@@ -1793,8 +1793,8 @@ AFSInvalidateObject( IN OUT AFSObjectInfoCB **ppObjectInfo,
                 // for any writes or reads to the cache to complete)
                 //
 
-                (VOID) AFSTearDownFcbExtents( (*ppObjectInfo)->Fcb,
-                                              NULL);
+                AFSTearDownFcbExtents( (*ppObjectInfo)->Fcb,
+                                       NULL);
             }
 
             (*ppObjectInfo)->DataVersion.QuadPart = (ULONGLONG)-1;
@@ -3284,8 +3284,8 @@ AFSSetVolumeState( IN AFSVolumeStatusCB *VolumeStatus)
                     // for any writes or reads to the cache to complete)
                     //
 
-                    (VOID) AFSTearDownFcbExtents( pFcb,
-                                                  NULL);
+                    AFSTearDownFcbExtents( pFcb,
+                                           NULL);
                 }
 
                 pCurrentObject = (AFSObjectInfoCB *)pCurrentObject->ListEntry.fLink;
@@ -8914,8 +8914,8 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo,
                     // for any writes or reads to the cache to complete)
                     //
 
-                    (VOID) AFSTearDownFcbExtents( ObjectInfo->Fcb,
-                                                  NULL);
+                    AFSTearDownFcbExtents( ObjectInfo->Fcb,
+                                           NULL);
                 }
 
                 break;
@@ -8986,8 +8986,8 @@ AFSPerformObjectInvalidate( IN AFSObjectInfoCB *ObjectInfo,
 
                                 bLocked = FALSE;
 
-                                (VOID) AFSTearDownFcbExtents( ObjectInfo->Fcb,
-                                                              NULL);
+                                AFSTearDownFcbExtents( ObjectInfo->Fcb,
+                                                       NULL);
                             }
                             else
                             {

diff --git a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
index f0ec3eb..b33658d 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
+++ b/src/WINNT/afsrdr/kernel/lib/AFSWorker.cpp
@@ -1412,7 +1412,10 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
                                                         TRUE);
                                     }
 
-                                    if( pCurrentChildObject->ObjectReferenceCount <= 0)
+                                    if( pCurrentChildObject->ObjectReferenceCount <= 0 &&
+                                        ( pCurrentChildObject->Fcb == NULL ||
+                                          pCurrentChildObject->Fcb->OpenReferenceCount == 0 &&
+                                          pCurrentChildObject->Fcb->Specific.File.ExtentCount == 0))
                                     {
 
                                         if( pCurrentChildObject->Fcb != NULL)
@@ -1558,7 +1561,8 @@ AFSPrimaryVolumeWorkerThread( IN PVOID Context)
                         if( BooleanFlagOn( pCurrentObject->Flags, AFS_OBJECT_FLAGS_DELETED) &&
                             pCurrentObject->ObjectReferenceCount <= 0 &&
                             ( pCurrentObject->Fcb == NULL ||
-                              pCurrentObject->Fcb->OpenReferenceCount == 0))
+                              pCurrentObject->Fcb->OpenReferenceCount == 0 &&
+                              pCurrentObject->Fcb->Specific.File.ExtentCount == 0))
                         {
 
                             if( pCurrentObject->Fcb != NULL)

diff --git a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
index 39cb790..5ba3bd0 100644 (file)
--- a/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
+++ b/src/WINNT/afsrdr/kernel/lib/Include/AFSCommon.h
@@ -408,7 +408,7 @@ AFSMarkDirty( IN AFSFcb *pFcb,
               IN LARGE_INTEGER *StartingByte,
               IN BOOLEAN DerefExtents);
 
-BOOLEAN
+VOID
 AFSTearDownFcbExtents( IN AFSFcb *Fcb,
                        IN GUID *AuthGroup);