From 6097787b1f3c7f9b4f8dca68f66d474eaa60b1d1 Mon Sep 17 00:00:00 2001 From: Tad Marshall Date: Mon, 9 Apr 2012 12:21:32 -0400 Subject: SERVER-2942 Windows remapPrivateView() should unmap & remap Change remapPrivateView() for Windows to use UnMapViewOfFile() and MapViewOfFileEx() along with a mutex to reset the memory map of the private view. --- src/mongo/util/concurrency/remap_lock.h | 31 ++++++++++++++++ src/mongo/util/mmap_win.cpp | 57 +++++++++++++++--------------- src/mongo/util/net/message_server_port.cpp | 12 ++++++- 3 files changed, 71 insertions(+), 29 deletions(-) create mode 100644 src/mongo/util/concurrency/remap_lock.h (limited to 'src/mongo/util') diff --git a/src/mongo/util/concurrency/remap_lock.h b/src/mongo/util/concurrency/remap_lock.h new file mode 100644 index 00000000000..c45d0d3dafb --- /dev/null +++ b/src/mongo/util/concurrency/remap_lock.h @@ -0,0 +1,31 @@ +// remap_lock.h + +/* + * Copyright 2012 10gen Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Used to synchronize thread creation in initAndListen() in mongod with +// MemoryMappedFile::remapPrivateView() in Windows. A dummy in mongos. +// The functional constructor & destructor for mongod are in mmap_win.cpp. +// The no-op constructor & destructor for mongos are in server.cpp. + +namespace mongo { + + struct RemapLock { + RemapLock(); + ~RemapLock(); + }; + +} diff --git a/src/mongo/util/mmap_win.cpp b/src/mongo/util/mmap_win.cpp index 2bf97930089..2a2c2f88337 100644 --- a/src/mongo/util/mmap_win.cpp +++ b/src/mongo/util/mmap_win.cpp @@ -22,6 +22,7 @@ #include "../db/concurrency.h" #include "../db/memconcept.h" #include "mongo/util/timer.h" +#include "mongo/util/concurrency/remap_lock.h" namespace mongo { @@ -31,6 +32,17 @@ namespace mongo { MAdvise::MAdvise(void *,unsigned, Advice) { } MAdvise::~MAdvise() { } + // SERVER-2942 -- We do it this way because RemapLock is used in both mongod and mongos but + // we need different effects. When called in mongod it needs to be a mutex and in mongos it + // needs to be a no-op. This is the mongod version, the no-op mongos version is in server.cpp. + SimpleMutex _remapLock( "remapLock" ); + RemapLock::RemapLock() { + _remapLock.lock(); + } + RemapLock::~RemapLock() { + _remapLock.unlock(); + } + /** notification on unmapping so we can clear writable bits */ void MemoryMappedFile::clearWritableBits(void *p) { for( unsigned i = ((size_t)p)/ChunkSize; i <= (((size_t)p)+len)/ChunkSize; i++ ) { @@ -224,40 +236,29 @@ namespace mongo { void* MemoryMappedFile::remapPrivateView(void *oldPrivateAddr) { d.dbMutex.assertWriteLocked(); // short window where we are unmapped so must be exclusive - // the mapViewMutex is to assure we get the same address on the remap - scoped_lock lk(mapViewMutex); + RemapLock lk; // Interlock with PortMessageServer::acceptedMP() to stop thread creation clearWritableBits(oldPrivateAddr); -#if 1 - // https://jira.mongodb.org/browse/SERVER-2942 - DWORD old; - bool ok = VirtualProtect(oldPrivateAddr, (SIZE_T) len, PAGE_READONLY, &old); - if( !ok ) { - DWORD e = GetLastError(); - log() << "VirtualProtect failed in remapPrivateView " << filename() << hex << oldPrivateAddr << ' ' << len << ' ' << errnoWithDescription(e) << endl; - verify(false); - } - return oldPrivateAddr; -#else if( !UnmapViewOfFile(oldPrivateAddr) ) { - DWORD e = GetLastError(); - log() << "UnMapViewOfFile failed " << filename() << ' ' << errnoWithDescription(e) << endl; - verify(false); + DWORD dosError = GetLastError(); + log() << "UnMapViewOfFile for " << filename() + << " failed with error " << errnoWithDescription( dosError ) << endl; + fassertFailed( 16147 ); } - // we want the new address to be the same as the old address in case things keep pointers around (as namespaceindex does). - void *p = MapViewOfFileEx(maphandle, FILE_MAP_READ, 0, 0, - /*dwNumberOfBytesToMap 0 means to eof*/0 /*len*/, - oldPrivateAddr); - - if ( p == 0 ) { - DWORD e = GetLastError(); - log() << "MapViewOfFileEx failed " << filename() << " " << errnoWithDescription(e) << endl; - verify(p); + void* newPrivateView = MapViewOfFileEx( + maphandle, // file mapping handle + FILE_MAP_READ, // access + 0, 0, // file offset, high and low + 0, // bytes to map, 0 == all + oldPrivateAddr ); // we want the same address we had before + if ( 0 == newPrivateView ) { + DWORD dosError = GetLastError(); + log() << "MapViewOfFileEx for " << filename() + << " failed with error " << errnoWithDescription( dosError ) << endl; } - verify(p == oldPrivateAddr); - return p; -#endif + fassert( 16148, newPrivateView == oldPrivateAddr ); + return newPrivateView; } class WindowsFlushable : public MemoryMappedFile::Flushable { diff --git a/src/mongo/util/net/message_server_port.cpp b/src/mongo/util/net/message_server_port.cpp index 2dd0821cf3d..0b8c2c54725 100644 --- a/src/mongo/util/net/message_server_port.cpp +++ b/src/mongo/util/net/message_server_port.cpp @@ -27,6 +27,7 @@ #include "../../db/cmdline.h" #include "../../db/lasterror.h" #include "../../db/stats/counters.h" +#include "mongo/util/concurrency/remap_lock.h" #include "mongo/util/concurrency/ticketholder.h" #ifdef __linux__ // TODO: consider making this ifndef _WIN32 @@ -133,7 +134,16 @@ namespace mongo { try { #ifndef __linux__ // TODO: consider making this ifdef _WIN32 - boost::thread thr( boost::bind( &pms::threadRun , p ) ); + { +#ifdef _WIN32 + // This Windows-only lock is to protect MemoryMappedFile::remapPrivateView ... + // it unmaps and remaps the private map and needs to get the previous address, + // and if we let a new thread get created between those calls, its thread + // stack could be created within that block, leading to an fassert ... + RemapLock lk; +#endif + boost::thread thr( boost::bind( &pms::threadRun , p ) ); + } #else pthread_attr_t attrs; pthread_attr_init(&attrs); -- cgit v1.2.1