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 62bcb21258b518afa64bafeac97e0fed456a8412..08b5a02c412b81b9490522187d5d43a57d734276 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 63f2f954c7e0d8763ebd0df6b8b1124895ab9de2..705ae6ca0208ef74747bdc0d8568bf28d4ddd907 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 72b0a7883a6b99408817782e1a1853fbf77b7150..0209a3ec14b1f0374923d78251243ec496c2522f 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 f0ec3eb8d4466e748355f72d2d287aaf22afc7fa..b33658d0e1fc9c9d429abe25d6780d7061186bf8 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 39cb790c37983403deeeb1097aaa54e9b8db477a..5ba3bd0e1a8bc733aeb6bd351966889e85b8313f 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);