Re: [PATCH] mm: fix page_mkclean_one (was: 2.6.19 file content corruption on ext3)

["Jari Sundell" <sundell.software@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On 12/27/06, Linus Torvalds <torvalds@osdl.org> wrote:
<snip>
>  - It never uses mprotect on the shared mappings, but it _does_ do:
>         "mincore()" - but the return values don't much matter (it's used
>                       as a heuristic on which parts to hash, apparently)
>
>                       I double- and triple-checked this one, because I
>                       did make changes to "mincore()", but those didn't go
>                       into the affected kernels anyway (ie they are not in
>                       plain 2.6.19, nor in 2.6.18.3 either)

Correct, mincore is only used to check if it should delay the hash checking.

>         "madvise(MADV_WILLNEED)"
>         "msync(MS_ASYNC)" (or MS_SYNC if you use a command line flag)
>         "munmap()" of course
>
>  - it never seems to mix mmap() and write() - it does _only_ mmap.
>
>  - it seems to mmap/munmap the shared files in nice 64-page chunks, all
>    64-page aligned in the file (ie it does NOT create one big mapping, it
>    has some kind of LRU of thse 64-page chunks). The only exception being
>    the last chunk, which it maps byte-accurate to the size.

The length of the chunks is only page aligned on single file torrents,
not so on multi-file torrents. I've attached a patch for rtorrent that
will extend the length to the page boundary.

>  - I haven't checked whether it only ever has the same chunk mapped once
>    at a time.

This should be the case, but two mapped chunks may share a page,
sometimes with different r/w permissions.

Jari Sundell

[-- Attachment #2: extend_mapping.diff --]
[-- Type: application/octet-stream, Size: 1887 bytes --]

Index: libtorrent/src/data/socket_file.cc
===================================================================
--- libtorrent/src/data/socket_file.cc	(revision 827)
+++ libtorrent/src/data/socket_file.cc	(working copy)
@@ -162,20 +162,27 @@
 MemoryChunk
 SocketFile::create_chunk(uint64_t offset, uint32_t length, int prot, int flags) const {
   if (!is_open())
-    throw internal_error("SocketFile::get_chunk() called on a closed file");
+    throw internal_error("SocketFile::get_chunk() called on a closed file.");
 
   if (((prot & MemoryChunk::prot_read) && !is_readable()) ||
       ((prot & MemoryChunk::prot_write) && !is_writable()))
-    throw storage_error("SocketFile::get_chunk() permission denied");
+    throw storage_error("SocketFile::get_chunk() permission denied.");
 
+  uint64_t fileSize = size();
+
   // For some reason mapping beyond the extent of the file does not
   // cause mmap to complain, so we need to check manually here.
-  if (offset < 0 || length == 0 || offset > size() || offset + length > size())
+  if (offset < 0 || length == 0 || offset > fileSize || offset + length > fileSize)
     return MemoryChunk();
 
-  uint64_t align = offset % MemoryChunk::page_size();
+  uint64_t align     = offset % MemoryChunk::page_size();
+  uint64_t mapLength = std::min(((length + align + MemoryChunk::page_size() - 1) / MemoryChunk::page_size()) * MemoryChunk::page_size(),
+                                fileSize - (offset - align));
 
-  char* ptr = (char*)mmap(NULL, length + align, prot, flags, m_fd, offset - align);
+  if (offset - align + mapLength != fileSize && (offset - align + mapLength) % MemoryChunk::page_size() != 0)
+    throw internal_error("SocketFile::create_chunk(...) Length not page aligned.");
+
+  char* ptr = (char*)mmap(NULL, mapLength, prot, flags, m_fd, offset - align);
   
   if (ptr == MAP_FAILED)
     return MemoryChunk();