From: Jeffrey Altman Date: Mon, 14 May 2012 15:11:57 +0000 (-0400) Subject: Windows: AFSTearDownExtents may experience active extents X-Git-Tag: openafs-stable-1_8_0pre1~2404 X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=94f96c6aae142478bf0824e7c3a3a810494a123d;hp=a9a768fb7ac06c887c45f6ed144c312fe357ab1e 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 Reviewed-by: Jeffrey Altman Tested-by: Jeffrey Altman --- diff --git a/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp b/src/WINNT/afsrdr/kernel/lib/AFSClose.cpp index 62bcb21..08b5a02 100644 --- 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 --- 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 --- 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 --- 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 --- 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);