From: Pierre Schweitzer Date: Thu, 26 May 2016 12:09:05 +0000 (+0000) Subject: [NTOSKRNL] X-Git-Tag: ReactOS-0.4.2~550 X-Git-Url: https://git.reactos.org/?p=reactos.git;a=commitdiff_plain;h=0b68260492f581723afa79044c31023c587898b3 [NTOSKRNL] Rework a bit the way mapping and pinning is working in ReactOS Cc. This is still wrong regarding the way Windows does it, but at least, it offers us more features for a more compatible behavior exposed to FSDs. First of all, reintroduce the mutex used to lock VACB. Moving there to a resource wasn't enough to work address the issue. This reverts r71387. Introduce the resource in the BCB, as it should be. This resource will be unused until pinning occurs. In such case, the VACB mutex gets unused so that we can get the BCB released by another thread without deadlocking the associated VACB. The VACB can be locked again after the last unpinning operation. Basically, that fixes drivers than pin in a thread and unpin in another thread, after having changed BCB owner. Until now, it would just have deadlock or crashed ReactOS. The implementation of this preserves hacks and stubplementations already in place in Cc (let's say it's another hack on top of hacks). It was successfully tested with Ext2Fsd 0.66. Short summary: - Replace the VACB resource by a mutex - Introduce a resource in the BCB - Fixed CcPinRead() implementation so that it respects PIN_EXCLUSIVE flag - Implement CcUnpinDataForThread() so that it can unpin data which ownership was changed - Implement CcSetBcbOwnerPointer() so that it properly set BCB owner and allows unpinning with CcUnpinDataForThread() CORE-11310 #resolve #comment Committed in r71406 svn path=/trunk/; revision=71406 --- diff --git a/reactos/ntoskrnl/cc/cacheman.c b/reactos/ntoskrnl/cc/cacheman.c index 111170d0cd5..2ac8df11417 100644 --- a/reactos/ntoskrnl/cc/cacheman.c +++ b/reactos/ntoskrnl/cc/cacheman.c @@ -122,9 +122,13 @@ CcSetBcbOwnerPointer ( CCTRACE(CC_API_DEBUG, "Bcb=%p Owner=%p\n", Bcb, Owner); - if (iBcb->OwnerPointer) - DPRINT1("OwnerPointer was already set?! Old: %p, New: %p\n", iBcb->OwnerPointer, Owner); - iBcb->OwnerPointer = Owner; + if (!ExIsResourceAcquiredExclusiveLite(&iBcb->Lock) && !ExIsResourceAcquiredSharedLite(&iBcb->Lock)) + { + DPRINT1("Current thread doesn't own resource!\n"); + return; + } + + ExSetResourceOwnerPointer(&iBcb->Lock, Owner); } /* diff --git a/reactos/ntoskrnl/cc/pin.c b/reactos/ntoskrnl/cc/pin.c index 74e7634c2dd..9df011ecd4f 100644 --- a/reactos/ntoskrnl/cc/pin.c +++ b/reactos/ntoskrnl/cc/pin.c @@ -4,7 +4,8 @@ * FILE: ntoskrnl/cc/pin.c * PURPOSE: Implements cache managers pinning interface * - * PROGRAMMERS: + * PROGRAMMERS: ? + Pierre Schweitzer (pierre@reactos.org) */ /* INCLUDES ******************************************************************/ @@ -113,7 +114,9 @@ CcMapData ( iBcb->PFCB.MappedFileOffset = *FileOffset; iBcb->Vacb = Vacb; iBcb->Dirty = FALSE; + iBcb->Pinned = FALSE; iBcb->RefCount = 1; + ExInitializeResourceLite(&iBcb->Lock); *pBcb = (PVOID)iBcb; CCTRACE(CC_API_DEBUG, "FileObject=%p FileOffset=%p Length=%lu Flags=0x%lx -> TRUE Bcb=%p\n", @@ -163,13 +166,36 @@ CcPinRead ( OUT PVOID * Bcb, OUT PVOID * Buffer) { + PINTERNAL_BCB iBcb; + CCTRACE(CC_API_DEBUG, "FileOffset=%p FileOffset=%p Length=%lu Flags=0x%lx\n", FileObject, FileOffset, Length, Flags); if (CcMapData(FileObject, FileOffset, Length, Flags, Bcb, Buffer)) { if (CcPinMappedData(FileObject, FileOffset, Length, Flags, Bcb)) + { + iBcb = *Bcb; + + ASSERT(iBcb->Pinned == FALSE); + + iBcb->Pinned = TRUE; + if (InterlockedIncrement(&iBcb->Vacb->PinCount) == 1) + { + KeReleaseMutex(&iBcb->Vacb->Mutex, FALSE); + } + + if (Flags & PIN_EXCLUSIVE) + { + ExAcquireResourceExclusiveLite(&iBcb->Lock, TRUE); + } + else + { + ExAcquireResourceSharedLite(&iBcb->Lock, TRUE); + } + return TRUE; + } else CcUnpinData(*Bcb); } @@ -228,19 +254,9 @@ VOID NTAPI CcUnpinData ( IN PVOID Bcb) { - PINTERNAL_BCB iBcb = Bcb; - CCTRACE(CC_API_DEBUG, "Bcb=%p\n", Bcb); - CcRosReleaseVacb(iBcb->Vacb->SharedCacheMap, - iBcb->Vacb, - TRUE, - iBcb->Dirty, - FALSE); - if (--iBcb->RefCount == 0) - { - ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); - } + CcUnpinDataForThread(Bcb, (ERESOURCE_THREAD)PsGetCurrentThread()); } /* @@ -256,13 +272,31 @@ CcUnpinDataForThread ( CCTRACE(CC_API_DEBUG, "Bcb=%p ResourceThreadId=%lu\n", Bcb, ResourceThreadId); - if (iBcb->OwnerPointer != (PVOID)ResourceThreadId) + if (iBcb->Pinned) { - DPRINT1("Invalid owner! Caller: %p, Owner: %p\n", (PVOID)ResourceThreadId, iBcb->OwnerPointer); - return; + ExReleaseResourceForThreadLite(&iBcb->Lock, ResourceThreadId); + iBcb->Pinned = FALSE; + if (InterlockedDecrement(&iBcb->Vacb->PinCount) == 0) + { + KeWaitForSingleObject(&iBcb->Vacb->Mutex, + Executive, + KernelMode, + FALSE, + NULL); + } } - CcUnpinData(Bcb); + CcRosReleaseVacb(iBcb->Vacb->SharedCacheMap, + iBcb->Vacb, + TRUE, + iBcb->Dirty, + FALSE); + + if (--iBcb->RefCount == 0) + { + ExDeleteResourceLite(&iBcb->Lock); + ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); + } } /* @@ -300,7 +334,11 @@ CcUnpinRepinnedBcb ( IoStatus->Information = 0; if (WriteThrough) { - ExAcquireResourceExclusiveLite(&iBcb->Vacb->Lock, TRUE); + KeWaitForSingleObject(&iBcb->Vacb->Mutex, + Executive, + KernelMode, + FALSE, + NULL); if (iBcb->Vacb->Dirty) { IoStatus->Status = CcRosFlushVacb(iBcb->Vacb); @@ -309,13 +347,27 @@ CcUnpinRepinnedBcb ( { IoStatus->Status = STATUS_SUCCESS; } - ExReleaseResourceLite(&iBcb->Vacb->Lock); + KeReleaseMutex(&iBcb->Vacb->Mutex, FALSE); } else { IoStatus->Status = STATUS_SUCCESS; } + if (iBcb->Pinned) + { + ExReleaseResourceLite(&iBcb->Lock); + iBcb->Pinned = FALSE; + if (InterlockedDecrement(&iBcb->Vacb->PinCount) == 0) + { + KeWaitForSingleObject(&iBcb->Vacb->Mutex, + Executive, + KernelMode, + FALSE, + NULL); + } + } + ExDeleteResourceLite(&iBcb->Lock); ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb); } } diff --git a/reactos/ntoskrnl/cc/view.c b/reactos/ntoskrnl/cc/view.c index 9a4348521c6..df7060f658d 100644 --- a/reactos/ntoskrnl/cc/view.c +++ b/reactos/ntoskrnl/cc/view.c @@ -166,10 +166,12 @@ CcRosFlushDirtyPages ( PROS_VACB current; BOOLEAN Locked; NTSTATUS Status; + LARGE_INTEGER ZeroTimeout; DPRINT("CcRosFlushDirtyPages(Target %lu)\n", Target); (*Count) = 0; + ZeroTimeout.QuadPart = 0; KeEnterCriticalRegion(); KeAcquireGuardedMutex(&ViewLock); @@ -197,8 +199,12 @@ CcRosFlushDirtyPages ( continue; } - Locked = ExAcquireResourceExclusiveLite(¤t->Lock, Wait); - if (!Locked) + Status = KeWaitForSingleObject(¤t->Mutex, + Executive, + KernelMode, + FALSE, + Wait ? NULL : &ZeroTimeout); + if (Status != STATUS_SUCCESS) { current->SharedCacheMap->Callbacks->ReleaseFromLazyWrite( current->SharedCacheMap->LazyWriteContext); @@ -211,7 +217,7 @@ CcRosFlushDirtyPages ( /* One reference is added above */ if (current->ReferenceCount > 2) { - ExReleaseResourceLite(¤t->Lock); + KeReleaseMutex(¤t->Mutex, FALSE); current->SharedCacheMap->Callbacks->ReleaseFromLazyWrite( current->SharedCacheMap->LazyWriteContext); CcRosVacbDecRefCount(current); @@ -222,7 +228,7 @@ CcRosFlushDirtyPages ( Status = CcRosFlushVacb(current); - ExReleaseResourceLite(¤t->Lock); + KeReleaseMutex(¤t->Mutex, FALSE); current->SharedCacheMap->Callbacks->ReleaseFromLazyWrite( current->SharedCacheMap->LazyWriteContext); @@ -419,7 +425,10 @@ CcRosReleaseVacb ( KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql); KeReleaseGuardedMutex(&ViewLock); - ExReleaseResourceLite(&Vacb->Lock); + if (InterlockedCompareExchange(&Vacb->PinCount, 0, 0) == 0) + { + KeReleaseMutex(&Vacb->Mutex, FALSE); + } return STATUS_SUCCESS; } @@ -456,7 +465,14 @@ CcRosLookupVacb ( CcRosVacbIncRefCount(current); KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql); KeReleaseGuardedMutex(&ViewLock); - ExAcquireResourceExclusiveLite(¤t->Lock, TRUE); + if (InterlockedCompareExchange(¤t->PinCount, 0, 0) == 0) + { + KeWaitForSingleObject(¤t->Mutex, + Executive, + KernelMode, + FALSE, + NULL); + } return current; } if (current->FileOffset.QuadPart > FileOffset) @@ -511,7 +527,7 @@ CcRosMarkDirtyVacb ( KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql); KeReleaseGuardedMutex(&ViewLock); - ExReleaseResourceLite(&Vacb->Lock); + KeReleaseMutex(&Vacb->Mutex, FALSE); return STATUS_SUCCESS; } @@ -564,7 +580,7 @@ CcRosUnmapVacb ( KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql); KeReleaseGuardedMutex(&ViewLock); - ExReleaseResourceLite(&Vacb->Lock); + KeReleaseMutex(&Vacb->Mutex, FALSE); return STATUS_SUCCESS; } @@ -665,8 +681,13 @@ CcRosCreateVacb ( current->DirtyVacbListEntry.Flink = NULL; current->DirtyVacbListEntry.Blink = NULL; current->ReferenceCount = 1; - ExInitializeResourceLite(¤t->Lock); - ExAcquireResourceExclusiveLite(¤t->Lock, TRUE); + current->PinCount = 0; + KeInitializeMutex(¤t->Mutex, 0); + KeWaitForSingleObject(¤t->Mutex, + Executive, + KernelMode, + FALSE, + NULL); KeAcquireGuardedMutex(&ViewLock); *Vacb = current; @@ -698,11 +719,18 @@ CcRosCreateVacb ( current); } #endif - ExReleaseResourceLite(&(*Vacb)->Lock); + KeReleaseMutex(&(*Vacb)->Mutex, FALSE); KeReleaseGuardedMutex(&ViewLock); ExFreeToNPagedLookasideList(&VacbLookasideList, *Vacb); *Vacb = current; - ExAcquireResourceExclusiveLite(¤t->Lock, TRUE); + if (InterlockedCompareExchange(¤t->PinCount, 0, 0) == 0) + { + KeWaitForSingleObject(¤t->Mutex, + Executive, + KernelMode, + FALSE, + NULL); + } return STATUS_SUCCESS; } if (current->FileOffset.QuadPart < FileOffset) @@ -868,7 +896,6 @@ CcRosInternalFreeVacb ( CcFreeCachePage, NULL); MmUnlockAddressSpace(MmGetKernelAddressSpace()); - ExDeleteResourceLite(&Vacb->Lock); ExFreeToNPagedLookasideList(&VacbLookasideList, Vacb); return STATUS_SUCCESS; @@ -932,7 +959,11 @@ CcFlushCache ( IoStatus->Status = Status; } } - ExReleaseResourceLite(¤t->Lock); + + if (InterlockedCompareExchange(¤t->PinCount, 0, 0) == 0) + { + KeReleaseMutex(¤t->Mutex, FALSE); + } KeAcquireGuardedMutex(&ViewLock); KeAcquireSpinLock(&SharedCacheMap->CacheMapLock, &oldIrql); diff --git a/reactos/ntoskrnl/include/internal/cc.h b/reactos/ntoskrnl/include/internal/cc.h index 98fad41acb5..ece2001635d 100644 --- a/reactos/ntoskrnl/include/internal/cc.h +++ b/reactos/ntoskrnl/include/internal/cc.h @@ -165,8 +165,6 @@ typedef struct _ROS_VACB PVOID BaseAddress; /* Memory area representing the region where the view's data is mapped. */ struct _MEMORY_AREA* MemoryArea; - /* Lock */ - ERESOURCE Lock; /* Are the contents of the view valid. */ BOOLEAN Valid; /* Are the contents of the view newer than those on disk. */ @@ -182,8 +180,12 @@ typedef struct _ROS_VACB LIST_ENTRY VacbLruListEntry; /* Offset in the file which this view maps. */ LARGE_INTEGER FileOffset; + /* Mutex */ + KMUTEX Mutex; /* Number of references. */ ULONG ReferenceCount; + /* How many times was it pinned? */ + volatile LONG PinCount; /* Pointer to the shared cache map for the file which this view maps data for. */ PROS_SHARED_CACHE_MAP SharedCacheMap; /* Pointer to the next VACB in a chain. */ @@ -191,11 +193,13 @@ typedef struct _ROS_VACB typedef struct _INTERNAL_BCB { + /* Lock */ + ERESOURCE Lock; PUBLIC_BCB PFCB; PROS_VACB Vacb; BOOLEAN Dirty; + BOOLEAN Pinned; CSHORT RefCount; /* (At offset 0x34 on WinNT4) */ - PVOID OwnerPointer; } INTERNAL_BCB, *PINTERNAL_BCB; VOID diff --git a/reactos/ntoskrnl/mm/section.c b/reactos/ntoskrnl/mm/section.c index 4943dabae31..7ece609b110 100644 --- a/reactos/ntoskrnl/mm/section.c +++ b/reactos/ntoskrnl/mm/section.c @@ -1110,7 +1110,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, * filesystems do because it is safe for us to use an offset with an * alignment less than the file system block size. */ - KeEnterCriticalRegion(); Status = CcRosGetVacb(SharedCacheMap, FileOffset, &BaseOffset, @@ -1119,7 +1118,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, &Vacb); if (!NT_SUCCESS(Status)) { - KeLeaveCriticalRegion(); return(Status); } if (!UptoDate) @@ -1132,7 +1130,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, if (!NT_SUCCESS(Status)) { CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE); - KeLeaveCriticalRegion(); return Status; } } @@ -1147,7 +1144,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, FileOffset - BaseOffset).LowPart >> PAGE_SHIFT; CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, TRUE); - KeLeaveCriticalRegion(); } else { @@ -1167,7 +1163,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, { return(Status); } - KeEnterCriticalRegion(); Status = CcRosGetVacb(SharedCacheMap, FileOffset, &BaseOffset, @@ -1176,7 +1171,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, &Vacb); if (!NT_SUCCESS(Status)) { - KeLeaveCriticalRegion(); return(Status); } if (!UptoDate) @@ -1189,7 +1183,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, if (!NT_SUCCESS(Status)) { CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE); - KeLeaveCriticalRegion(); return Status; } } @@ -1219,7 +1212,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, &Vacb); if (!NT_SUCCESS(Status)) { - KeLeaveCriticalRegion(); return(Status); } if (!UptoDate) @@ -1232,7 +1224,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, if (!NT_SUCCESS(Status)) { CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE); - KeLeaveCriticalRegion(); return Status; } } @@ -1248,7 +1239,6 @@ MiReadPage(PMEMORY_AREA MemoryArea, } MiUnmapPageInHyperSpace(Process, PageAddr, Irql); CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, FALSE); - KeLeaveCriticalRegion(); } return(STATUS_SUCCESS); } @@ -3242,12 +3232,10 @@ ExeFmtpReadFile(IN PVOID File, BufferSize = PAGE_ROUND_UP(BufferSize); /* Flush data since we're about to perform a non-cached read */ - KeEnterCriticalRegion(); CcFlushCache(FileObject->SectionObjectPointer, &FileOffset, BufferSize, &Iosb); - KeLeaveCriticalRegion(); /* * It's ok to use paged pool, because this is a temporary buffer only used in