[NTOSKRNL]
authorPierre Schweitzer <pierre@reactos.org>
Thu, 26 May 2016 12:09:05 +0000 (12:09 +0000)
committerPierre Schweitzer <pierre@reactos.org>
Thu, 26 May 2016 12:09:05 +0000 (12:09 +0000)
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

reactos/ntoskrnl/cc/cacheman.c
reactos/ntoskrnl/cc/pin.c
reactos/ntoskrnl/cc/view.c
reactos/ntoskrnl/include/internal/cc.h
reactos/ntoskrnl/mm/section.c

index 111170d..2ac8df1 100644 (file)
@@ -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);
 }
 
 /*
index 74e7634..9df011e 100644 (file)
@@ -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);
     }
 }
index 9a43485..df7060f 100644 (file)
@@ -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(&current->Lock, Wait);
-        if (!Locked)
+        Status = KeWaitForSingleObject(&current->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(&current->Lock);
+            KeReleaseMutex(&current->Mutex, FALSE);
             current->SharedCacheMap->Callbacks->ReleaseFromLazyWrite(
                 current->SharedCacheMap->LazyWriteContext);
             CcRosVacbDecRefCount(current);
@@ -222,7 +228,7 @@ CcRosFlushDirtyPages (
 
         Status = CcRosFlushVacb(current);
 
-        ExReleaseResourceLite(&current->Lock);
+        KeReleaseMutex(&current->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(&current->Lock, TRUE);
+            if (InterlockedCompareExchange(&current->PinCount, 0, 0) == 0)
+            {
+                KeWaitForSingleObject(&current->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(&current->Lock);
-    ExAcquireResourceExclusiveLite(&current->Lock, TRUE);
+    current->PinCount = 0;
+    KeInitializeMutex(&current->Mutex, 0);
+    KeWaitForSingleObject(&current->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(&current->Lock, TRUE);
+            if (InterlockedCompareExchange(&current->PinCount, 0, 0) == 0)
+            {
+                KeWaitForSingleObject(&current->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(&current->Lock);
+
+                if (InterlockedCompareExchange(&current->PinCount, 0, 0) == 0)
+                {
+                    KeReleaseMutex(&current->Mutex, FALSE);
+                }
 
                 KeAcquireGuardedMutex(&ViewLock);
                 KeAcquireSpinLock(&SharedCacheMap->CacheMapLock, &oldIrql);
index 98fad41..ece2001 100644 (file)
@@ -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
index 4943dab..7ece609 100644 (file)
@@ -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