[ros-diffs] [pschweitzer] 71406: [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 fo...

Author: pschweitzer
Date: Thu May 26 12:09:05 2016
New Revision: 71406

URL: http://svn.reactos.org/svn/reactos?rev=71406&view=rev
Log:
[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

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

Modified: trunk/reactos/ntoskrnl/cc/cacheman.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/cc/cacheman.c?rev…
==============================================================================
--- trunk/reactos/ntoskrnl/cc/cacheman.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/cc/cacheman.c	[iso-8859-1] Thu May 26 12:09:05 2016
@@ -122,9 +122,13 @@
     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);
 }
 
 /*

Modified: trunk/reactos/ntoskrnl/cc/pin.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/cc/pin.c?rev=7140…
==============================================================================
--- trunk/reactos/ntoskrnl/cc/pin.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/cc/pin.c	[iso-8859-1] Thu May 26 12:09:05 2016
@@ -4,7 +4,8 @@
  * FILE:            ntoskrnl/cc/pin.c
  * PURPOSE:         Implements cache managers pinning interface
  *
- * PROGRAMMERS:
+ * PROGRAMMERS:     ?
+                    Pierre Schweitzer (pierre(a)reactos.org)
  */
 
 /* INCLUDES ******************************************************************/
@@ -113,7 +114,9 @@
     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 @@
     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,41 +254,49 @@
 CcUnpinData (
     IN PVOID Bcb)
 {
+    CCTRACE(CC_API_DEBUG, "Bcb=%p\n", Bcb);
+
+    CcUnpinDataForThread(Bcb, (ERESOURCE_THREAD)PsGetCurrentThread());
+}
+
+/*
+ * @unimplemented
+ */
+VOID
+NTAPI
+CcUnpinDataForThread (
+    IN	PVOID Bcb,
+    IN	ERESOURCE_THREAD ResourceThreadId)
+{
     PINTERNAL_BCB iBcb = Bcb;
 
-    CCTRACE(CC_API_DEBUG, "Bcb=%p\n", Bcb);
+    CCTRACE(CC_API_DEBUG, "Bcb=%p ResourceThreadId=%lu\n", Bcb,
ResourceThreadId);
+
+    if (iBcb->Pinned)
+    {
+        ExReleaseResourceForThreadLite(&iBcb->Lock, ResourceThreadId);
+        iBcb->Pinned = FALSE;
+        if (InterlockedDecrement(&iBcb->Vacb->PinCount) == 0)
+        {
+            KeWaitForSingleObject(&iBcb->Vacb->Mutex,
+                                  Executive,
+                                  KernelMode,
+                                  FALSE,
+                                  NULL);
+        }
+    }
 
     CcRosReleaseVacb(iBcb->Vacb->SharedCacheMap,
                      iBcb->Vacb,
                      TRUE,
                      iBcb->Dirty,
                      FALSE);
+
     if (--iBcb->RefCount == 0)
     {
+        ExDeleteResourceLite(&iBcb->Lock);
         ExFreeToNPagedLookasideList(&iBcbLookasideList, iBcb);
     }
-}
-
-/*
- * @unimplemented
- */
-VOID
-NTAPI
-CcUnpinDataForThread (
-    IN	PVOID Bcb,
-    IN	ERESOURCE_THREAD ResourceThreadId)
-{
-    PINTERNAL_BCB iBcb = Bcb;
-
-    CCTRACE(CC_API_DEBUG, "Bcb=%p ResourceThreadId=%lu\n", Bcb,
ResourceThreadId);
-
-    if (iBcb->OwnerPointer != (PVOID)ResourceThreadId)
-    {
-        DPRINT1("Invalid owner! Caller: %p, Owner: %p\n",
(PVOID)ResourceThreadId, iBcb->OwnerPointer);
-        return;
-    }
-
-    CcUnpinData(Bcb);
 }
 
 /*
@@ -300,7 +334,11 @@
         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 @@
             {
                 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);
     }
 }

Modified: trunk/reactos/ntoskrnl/cc/view.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/cc/view.c?rev=714…
==============================================================================
--- trunk/reactos/ntoskrnl/cc/view.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/cc/view.c	[iso-8859-1] Thu May 26 12:09:05 2016
@@ -166,10 +166,12 @@
     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 @@
             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 @@
         /* 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 @@
 
         Status = CcRosFlushVacb(current);
 
-        ExReleaseResourceLite(&current->Lock);
+        KeReleaseMutex(&current->Mutex, FALSE);
         current->SharedCacheMap->Callbacks->ReleaseFromLazyWrite(
             current->SharedCacheMap->LazyWriteContext);
 
@@ -419,7 +425,10 @@
 
     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 @@
             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 @@
 
     KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql);
     KeReleaseGuardedMutex(&ViewLock);
-    ExReleaseResourceLite(&Vacb->Lock);
+    KeReleaseMutex(&Vacb->Mutex, FALSE);
 
     return STATUS_SUCCESS;
 }
@@ -564,7 +580,7 @@
 
     KeReleaseSpinLock(&SharedCacheMap->CacheMapLock, oldIrql);
     KeReleaseGuardedMutex(&ViewLock);
-    ExReleaseResourceLite(&Vacb->Lock);
+    KeReleaseMutex(&Vacb->Mutex, FALSE);
 
     return STATUS_SUCCESS;
 }
@@ -665,8 +681,13 @@
     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 @@
                         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 @@
                      CcFreeCachePage,
                      NULL);
     MmUnlockAddressSpace(MmGetKernelAddressSpace());
-    ExDeleteResourceLite(&Vacb->Lock);
 
     ExFreeToNPagedLookasideList(&VacbLookasideList, Vacb);
     return STATUS_SUCCESS;
@@ -932,7 +959,11 @@
                         IoStatus->Status = Status;
                     }
                 }
-                ExReleaseResourceLite(&current->Lock);
+
+                if (InterlockedCompareExchange(&current->PinCount, 0, 0) == 0)
+                {
+                    KeReleaseMutex(&current->Mutex, FALSE);
+                }
 
                 KeAcquireGuardedMutex(&ViewLock);
                 KeAcquireSpinLock(&SharedCacheMap->CacheMapLock, &oldIrql);

Modified: trunk/reactos/ntoskrnl/include/internal/cc.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/cc.h	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/include/internal/cc.h	[iso-8859-1] Thu May 26 12:09:05 2016
@@ -165,8 +165,6 @@
     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 @@
     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 _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

Modified: trunk/reactos/ntoskrnl/mm/section.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/section.c?rev=…
==============================================================================
--- trunk/reactos/ntoskrnl/mm/section.c	[iso-8859-1] (original)
+++ trunk/reactos/ntoskrnl/mm/section.c	[iso-8859-1] Thu May 26 12:09:05 2016
@@ -1110,7 +1110,6 @@
          * 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 @@
                               &Vacb);
         if (!NT_SUCCESS(Status))
         {
-            KeLeaveCriticalRegion();
             return(Status);
         }
         if (!UptoDate)
@@ -1132,7 +1130,6 @@
             if (!NT_SUCCESS(Status))
             {
                 CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE);
-                KeLeaveCriticalRegion();
                 return Status;
             }
         }
@@ -1147,7 +1144,6 @@
                                        FileOffset - BaseOffset).LowPart >>
PAGE_SHIFT;
 
         CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, TRUE);
-        KeLeaveCriticalRegion();
     }
     else
     {
@@ -1167,7 +1163,6 @@
         {
             return(Status);
         }
-        KeEnterCriticalRegion();
         Status = CcRosGetVacb(SharedCacheMap,
                               FileOffset,
                               &BaseOffset,
@@ -1176,7 +1171,6 @@
                               &Vacb);
         if (!NT_SUCCESS(Status))
         {
-            KeLeaveCriticalRegion();
             return(Status);
         }
         if (!UptoDate)
@@ -1189,7 +1183,6 @@
             if (!NT_SUCCESS(Status))
             {
                 CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE);
-                KeLeaveCriticalRegion();
                 return Status;
             }
         }
@@ -1219,7 +1212,6 @@
                                   &Vacb);
             if (!NT_SUCCESS(Status))
             {
-                KeLeaveCriticalRegion();
                 return(Status);
             }
             if (!UptoDate)
@@ -1232,7 +1224,6 @@
                 if (!NT_SUCCESS(Status))
                 {
                     CcRosReleaseVacb(SharedCacheMap, Vacb, FALSE, FALSE, FALSE);
-                    KeLeaveCriticalRegion();
                     return Status;
                 }
             }
@@ -1248,7 +1239,6 @@
         }
         MiUnmapPageInHyperSpace(Process, PageAddr, Irql);
         CcRosReleaseVacb(SharedCacheMap, Vacb, TRUE, FALSE, FALSE);
-        KeLeaveCriticalRegion();
     }
     return(STATUS_SUCCESS);
 }
@@ -3242,12 +3232,10 @@
     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