From 1736cd0fb260d29ac0df3f05a092b99f7933bf07 Mon Sep 17 00:00:00 2001 From: robliao Date: Mon, 9 Nov 2015 16:13:18 -0800 Subject: Revert "[sql] Validate database files before enabling memory-mapping." This reverts commit de8784cf209f5c254f6c65718373985eb662b36f. Reason for Revert: Implicated in test failures: ChromeMainTest.ReuseBrowserInstanceWhenOpeningFile ChromeMainTest.SecondLaunch ChromeMainTest.SecondLaunchFromIncognitoWithNormalUrl DiagnosticsControllerTest.RecoverFromNssCertDbFailure DiagnosticsControllerTest.RecoverFromNssKeyDbFailure First failing bot: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/8908 Original Issue: https://codereview.chromium.org/1426743006 TBR=shess@chromium.org, isherman@chromium.org, rmcilroy@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=537742 Review URL: https://codereview.chromium.org/1432953002 Cr-Commit-Position: refs/heads/master@{#358709} --- sql/connection.cc | 156 +++--------------------------------------------------- sql/connection.h | 23 -------- 2 files changed, 8 insertions(+), 171 deletions(-) (limited to 'sql') diff --git a/sql/connection.cc b/sql/connection.cc index 185b955..44cb597 100644 --- a/sql/connection.cc +++ b/sql/connection.cc @@ -537,8 +537,6 @@ void Connection::Preload() { scoped_ptr buf(new char[page_size]); for (sqlite3_int64 pos = 0; pos < preload_size; pos += page_size) { rc = file->pMethods->xRead(file, buf.get(), page_size, pos); - - // TODO(shess): Consider calling OnSqliteError(). if (rc != SQLITE_OK) return; } @@ -863,144 +861,6 @@ std::string Connection::CollectCorruptionInfo() { return debug_info; } -size_t Connection::GetAppropriateMmapSize() { - AssertIOAllowed(); - - // TODO(shess): Using sql::MetaTable seems indicated, but mixing - // sql::MetaTable and direct access seems error-prone. It might make sense to - // simply integrate sql::MetaTable functionality into sql::Connection. - -#if defined(OS_IOS) - // iOS SQLite does not support memory mapping. - return 0; -#endif - - // If the database doesn't have a place to track progress, assume the worst. - // This will happen when new databases are created. - if (!DoesTableExist("meta")) { - RecordOneEvent(EVENT_MMAP_META_MISSING); - return 0; - } - - // Key into meta table to get status from a previous run. The value - // represents how much data in bytes has successfully been read from the - // database. |kMmapFailure| indicates that there was a read error and the - // database should not be memory-mapped, while |kMmapSuccess| indicates that - // the entire file was read at some point and can be memory-mapped without - // constraint. - const char* kMmapStatusKey = "mmap_status"; - static const sqlite3_int64 kMmapFailure = -2; - static const sqlite3_int64 kMmapSuccess = -1; - - // Start reading from 0 unless status is found in meta table. - sqlite3_int64 mmap_ofs = 0; - - // Retrieve the current status. It is fine for the status to be missing - // entirely, but any error prevents memory-mapping. - { - const char* kMmapStatusSql = "SELECT value FROM meta WHERE key = ?"; - Statement s(GetUniqueStatement(kMmapStatusSql)); - s.BindString(0, kMmapStatusKey); - if (s.Step()) { - mmap_ofs = s.ColumnInt64(0); - } else if (!s.Succeeded()) { - RecordOneEvent(EVENT_MMAP_META_FAILURE_READ); - return 0; - } - } - - // Database read failed in the past, don't memory map. - if (mmap_ofs == kMmapFailure) { - RecordOneEvent(EVENT_MMAP_FAILED); - return 0; - } else if (mmap_ofs != kMmapSuccess) { - // Continue reading from previous offset. - DCHECK_GE(mmap_ofs, 0); - - // TODO(shess): Could this reading code be shared with Preload()? It would - // require locking twice (this code wouldn't be able to access |db_size| so - // the helper would have to return amount read). - - // Read more of the database looking for errors. The VFS interface is used - // to assure that the reads are valid for SQLite. |g_reads_allowed| is used - // to limit checking to 20MB per run of Chromium. - sqlite3_file* file = NULL; - sqlite3_int64 db_size = 0; - if (SQLITE_OK != GetSqlite3FileAndSize(db_, &file, &db_size)) { - RecordOneEvent(EVENT_MMAP_VFS_FAILURE); - return 0; - } - - // Read the data left, or |g_reads_allowed|, whichever is smaller. - // |g_reads_allowed| limits the total amount of I/O to spend verifying data - // in a single Chromium run. - sqlite3_int64 amount = db_size - mmap_ofs; - if (amount < 0) - amount = 0; - if (amount > 0) { - base::AutoLock lock(g_sqlite_init_lock.Get()); - static sqlite3_int64 g_reads_allowed = 20 * 1024 * 1024; - if (g_reads_allowed < amount) - amount = g_reads_allowed; - g_reads_allowed -= amount; - } - - // |amount| can be <= 0 if |g_reads_allowed| ran out of quota, or if the - // database was truncated after a previous pass. - if (amount <= 0 && mmap_ofs < db_size) { - DCHECK_EQ(0, amount); - RecordOneEvent(EVENT_MMAP_SUCCESS_NO_PROGRESS); - } else { - static const int kPageSize = 4096; - char buf[kPageSize]; - while (amount > 0) { - int rc = file->pMethods->xRead(file, buf, sizeof(buf), mmap_ofs); - if (rc == SQLITE_OK) { - mmap_ofs += sizeof(buf); - amount -= sizeof(buf); - } else if (rc == SQLITE_IOERR_SHORT_READ) { - // Reached EOF for a database with page size < |kPageSize|. - mmap_ofs = db_size; - break; - } else { - // TODO(shess): Consider calling OnSqliteError(). - mmap_ofs = kMmapFailure; - break; - } - } - - // Log these events after update to distinguish meta update failure. - Events event; - if (mmap_ofs >= db_size) { - mmap_ofs = kMmapSuccess; - event = EVENT_MMAP_SUCCESS_NEW; - } else if (mmap_ofs > 0) { - event = EVENT_MMAP_SUCCESS_PARTIAL; - } else { - DCHECK_EQ(kMmapFailure, mmap_ofs); - event = EVENT_MMAP_FAILED_NEW; - } - - const char* kMmapUpdateStatusSql = "REPLACE INTO meta VALUES (?, ?)"; - Statement s(GetUniqueStatement(kMmapUpdateStatusSql)); - s.BindString(0, kMmapStatusKey); - s.BindInt64(1, mmap_ofs); - if (!s.Run()) { - RecordOneEvent(EVENT_MMAP_META_FAILURE_UPDATE); - return 0; - } - - RecordOneEvent(event); - } - } - - if (mmap_ofs == kMmapFailure) - return 0; - if (mmap_ofs == kMmapSuccess) - return 256 * 1024 * 1024; - return mmap_ofs; -} - void Connection::TrimMemory(bool aggressively) { if (!db_) return; @@ -1820,14 +1680,14 @@ bool Connection::OpenInternal(const std::string& file_name, } // Enable memory-mapped access. The explicit-disable case is because SQLite - // can be built to default-enable mmap. GetAppropriateMmapSize() calculates a - // safe range to memory-map based on past regular I/O. This value will be - // capped by SQLITE_MAX_MMAP_SIZE, which could be different between 32-bit and - // 64-bit platforms. - size_t mmap_size = mmap_disabled_ ? 0 : GetAppropriateMmapSize(); - std::string mmap_sql = - base::StringPrintf("PRAGMA mmap_size = %" PRIuS, mmap_size); - ignore_result(Execute(mmap_sql.c_str())); + // can be built to default-enable mmap. This value will be capped by + // SQLITE_MAX_MMAP_SIZE, which could be different between 32-bit and 64-bit + // platforms. + if (mmap_disabled_) { + ignore_result(Execute("PRAGMA mmap_size = 0")); + } else { + ignore_result(Execute("PRAGMA mmap_size = 268435456")); // 256MB. + } // Determine if memory-mapping has actually been enabled. The Execute() above // can succeed without changing the amount mapped. diff --git a/sql/connection.h b/sql/connection.h index 181131e..4e3ce26 100644 --- a/sql/connection.h +++ b/sql/connection.h @@ -204,19 +204,6 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { EVENT_COMMIT, EVENT_ROLLBACK, - // Track success and failure in GetAppropriateMmapSize(). - // GetAppropriateMmapSize() should record at most one of these per run. The - // case of mapping everything is not recorded. - EVENT_MMAP_META_MISSING, // No meta table present. - EVENT_MMAP_META_FAILURE_READ, // Failed reading meta table. - EVENT_MMAP_META_FAILURE_UPDATE, // Failed updating meta table. - EVENT_MMAP_VFS_FAILURE, // Failed to access VFS. - EVENT_MMAP_FAILED, // Failure from past run. - EVENT_MMAP_FAILED_NEW, // Read error in this run. - EVENT_MMAP_SUCCESS_NEW, // Read to EOF in this run. - EVENT_MMAP_SUCCESS_PARTIAL, // Read but did not reach EOF. - EVENT_MMAP_SUCCESS_NO_PROGRESS, // Read quota exhausted. - // Leave this at the end. // TODO(shess): |EVENT_MAX| causes compile fail on Windows. EVENT_MAX_VALUE @@ -702,16 +689,6 @@ class SQL_EXPORT Connection : public base::trace_event::MemoryDumpProvider { // Helper to collect diagnostic info for errors. std::string CollectErrorInfo(int error, Statement* stmt) const; - // Calculates a value appropriate to pass to "PRAGMA mmap_size = ". So errors - // can make it unsafe to map a file, so the file is read using regular I/O, - // with any errors causing 0 (don't map anything) to be returned. If the - // entire file is read without error, a large value is returned which will - // allow the entire file to be mapped in most cases. - // - // Results are recorded in the database's meta table for future reference, so - // the file should only be read through once. - size_t GetAppropriateMmapSize(); - // The actual sqlite database. Will be NULL before Init has been called or if // Init resulted in an error. sqlite3* db_; -- cgit v1.1