Re: [ros-svn] [hbirr] 13570: - Started the rewriting of the cache manger.

[Filip Navara]

[e-mail] wrote:
> Started the rewriting of the cache manger.
> The new cache manager uses section objects and maps the content of cached files
>
> into the kernel address space for functions like CcCopyRead.
> The implementation isn't complete. It works for me to compile ros on ros if enough ram is available.

Very good work!

I tried it and needed the attached changes in order to boot...
[cc02.diff] Don't try to map data past the end of file in VFAT.
[cc03.diff] Fix incorrect check in CcMapData.

Also I've patch which fixes fast mutex implementation and it's usage, 
which depends on these cache manager changes.

Regards,
Filip

[Hartmut Birr]

Filip Navara schrieb:
> I tried it and needed the attached changes in order to boot...

Thanks for testing my changes.
> [cc02.diff] Don't try to map data past the end of file in VFAT.

I think that this patch isn't necessary, because the real problem is in 
pin.c.
> [cc03.diff] Fix incorrect check in CcMapData.

The check which you have changed was correct. Currently the file size is 
limited to the number of cache view entries. I will implement a x-level 
tree structure in the future. But some other checks are incorrect. I've 
add a patch.
> Also I've patch which fixes fast mutex implementation and it's usage, 
> which depends on these cache manager changes.

The global lock (fast mutex) implementation is a little bit dirty. 
Currently I implement a function which does remove the unneeded data 
section objects after some time. My problem is the sequence to acquire 
the segment lock and the global lock.

- Hartmut

E:\Sandbox\ros_cache\reactos>set SVN_EDITOR=notepad

E:\Sandbox\ros_cache\reactos>d:\programme\subversion\bin\svn.exe diff ntoskrnl\cc\pin.c        
Index: ntoskrnl/cc/pin.c
===================================================================
--- ntoskrnl/cc/pin.c	(Revision 13570)
+++ ntoskrnl/cc/pin.c	(Arbeitskopie)
@@ -67,20 +67,26 @@

Bcb = FileObject->SectionObjectPointer->SharedCacheMap;

-   if (FileOffset->QuadPart + Length > Bcb->FileSizes.AllocationSize.QuadPart)
+   if (FileOffset->QuadPart >= Bcb->FileSizes.FileSize.QuadPart)
    {
-      DPRINT("%d %I64d %I64d\n", Length, FileOffset->QuadPart + Length, Bcb->FileSizes.AllocationSize.QuadPart);
-//      KEBUGCHECK(0);
+      CHECKPOINT1;
+      *piBcb = NULL;
+      *pBuffer = NULL;
+      return FALSE;
    }

-   if (FileOffset->QuadPart + Length - ROUND_DOWN(FileOffset->QuadPart, CACHE_VIEW_SIZE) > CACHE_VIEW_SIZE)
+   if (FileOffset->QuadPart + Length > Bcb->FileSizes.FileSize.QuadPart)
    {
-      /* not implemented */
+      CHECKPOINT1;
+      Length = Bcb->FileSizes.FileSize.QuadPart - FileOffset->QuadPart;
+   }
+
+   if (ROUND_DOWN(FileOffset->QuadPart, CACHE_VIEW_SIZE) + CACHE_VIEW_SIZE != ROUND_UP(FileOffset->QuadPart + Length, CACHE_VIEW_SIZE))
+   {
+      /* Mapping over the border of a cache view isn't implemented */
       KEBUGCHECK(0);
    }

-
-
    if (Bcb->FileSizes.AllocationSize.QuadPart > sizeof(Bcb->CacheView) / sizeof(Bcb->CacheView[0]) * CACHE_VIEW_SIZE)
    {
       /* not implemented */

[Filip Navara]

Hartmut Birr wrote:
> > [cc02.diff] Don't try to map data past the end of file in VFAT.
>
> I think that this patch isn't necessary, because the real problem is 
> in pin.c.

At first I did the same thing as you did in the attached patch. Then I 
actually followed it by some testing on Windows and _it is possible to 
map data past the end of file_ with CcMapData. I haven't tried to access 
the mapped memory though...
> [cc03.diff] Fix incorrect check in CcMapData.
>
> The check which you have changed was correct. Currently the file size 
> is limited to the number of cache view entries. I will implement a 
> x-level tree structure in the future. But some other checks are 
> incorrect. I've add a patch.

Oh, sorry, I didn't realize it's checked because of multi-level VACB 
tables. A comment would have been helpful ;-)
> > Also I've patch which fixes fast mutex implementation and it's usage, 
> > which depends on these cache manager changes.

[snip]

To clarify this, the fix I have fixes some small issues with FAST_MUTEX 
implementation and changes it to actually raise to APC_LEVEL instead of 
entring critical section. Many people (especially Mike Nordell ;-) have 
been complaining about it for some time and all previous attempts to fix 
it failed because of the broken locking in cache manager. Now that you 
have done the dirty work, it's finally possible to correctly fix this 
issue... ;-)

Regards,
Filip

[Filip Navara]

Filip Navara wrote:
> Hartmut Birr wrote:
> > > [cc02.diff] Don't try to map data past the end of file in VFAT.
> >
> > I think that this patch isn't necessary, because the real problem is 
> > in pin.c.
>
> At first I did the same thing as you did in the attached patch. Then I 
> actually followed it by some testing on Windows and _it is possible to 
> map data past the end of file_ with CcMapData. I haven't tried to 
> access the mapped memory though...

I did more testing and the result is the attached patch (comment 
inside). Does it look ok?

Regards,
Filip

Index: section.c
===================================================================
--- section.c	(revision 13615)
+++ section.c	(working copy)
@@ -866,15 +866,20 @@

FileOffset.QuadPart = SegOffset + SectionData->Segment->FileOffset;
    Length = PageCount * PAGE_SIZE;
-   if (SegOffset > SectionData->Segment->RawLength)
+
+   if (SegOffset >= SectionData->Segment->RawLength)
    {
-      KEBUGCHECK(0);
+      /* It is perfectly valid to map area past the end of file with CcMapData
+       * and then access the pages. In this case the page fault doesn't result
+       * in I/O request and we get zeroed pages.
+       */
+      return STATUS_SUCCESS;
    }
+
    if (Length + SegOffset > SectionData->Segment->RawLength)
    {
       Length = SectionData->Segment->RawLength - SegOffset;
    }
-

Status = MmspRawReadPages(SectionData->Section->FileObject,
                              SectorSize,
@@ -1717,7 +1722,7 @@
       Status = MmspReadSectionSegmentPages(SectionData, Offset, PageCount, Pfn);
       if (!NT_SUCCESS(Status))
       {
-         DPRINT1("MiReadPage failed (Status %x)\n", Status);
+         DPRINT1("MmspReadSectionSegmentPages failed (Status %x)\n", Status);
       }
       MmLockAddressSpace(AddressSpace);
       if (!NT_SUCCESS(Status))

[Hartmut Birr]

Filip Navara schrieb:
> Filip Navara wrote:
> > Hartmut Birr wrote:
> > > > [cc02.diff] Don't try to map data past the end of file in VFAT.
> > >
> > > I think that this patch isn't necessary, because the real problem is 
> > > in pin.c.
> >
> > At first I did the same thing as you did in the attached patch. Then 
> > I actually followed it by some testing on Windows and _it is possible 
> > to map data past the end of file_ with CcMapData. I haven't tried to 
> > access the mapped memory though...
>
> I did more testing and the result is the attached patch (comment 
> inside). Does it look ok?
>
> Regards,
> Filip

I think that we must add more checks and that the change is at the wrong 
position. The zero pages at the end of the file should be mapped like 
the image sections with segments whose raw length is shorter than the 
real length. What should we do if pages past the end of the file are 
modified and the section object is deleted?

- Hartmut