From 1837a115bb25d0186fdf46ac5d66798ac8f821d3 Mon Sep 17 00:00:00 2001 From: Dong Heng Date: Tue, 22 Jan 2019 11:21:29 +0800 Subject: [PATCH 1/4] fix(nv_flash): Fix page selection algo to consider free entry counts as well Current page selection algorithm selects a page for compaction based on just erased counts and gives up when it does not find any page with erased count greater than 0. This is problematic since the current allocation procedure skips the active page if there is not enough room for the item in that page leaving free chunks on the pages. This change modifies the algorithm to consider both erased as well as free counts on the candidate pages. esp-idf commit ID: 7e79471e --- components/nvs_flash/src/nvs_pagemanager.cpp | 20 ++++++++++-------- .../nvs_flash/test_nvs_host/test_nvs.cpp | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/components/nvs_flash/src/nvs_pagemanager.cpp b/components/nvs_flash/src/nvs_pagemanager.cpp index 943f54f2..cb274619 100644 --- a/components/nvs_flash/src/nvs_pagemanager.cpp +++ b/components/nvs_flash/src/nvs_pagemanager.cpp @@ -125,17 +125,18 @@ esp_err_t PageManager::requestNewPage() } // find the page with the higest number of erased items - TPageListIterator maxErasedItemsPageIt; - size_t maxErasedItems = 0; + TPageListIterator maxUnusedItemsPageIt; + size_t maxUnusedItems = 0; for (auto it = begin(); it != end(); ++it) { - auto erased = it->getErasedEntryCount(); - if (erased > maxErasedItems) { - maxErasedItemsPageIt = it; - maxErasedItems = erased; + + auto unused = Page::ENTRY_COUNT - it->getUsedEntryCount(); + if (unused > maxUnusedItems) { + maxUnusedItemsPageIt = it; + maxUnusedItems = unused; } } - if (maxErasedItems == 0) { + if (maxUnusedItems == 0) { return ESP_ERR_NVS_NOT_ENOUGH_SPACE; } @@ -146,7 +147,8 @@ esp_err_t PageManager::requestNewPage() Page* newPage = &mPageList.back(); - Page* erasedPage = maxErasedItemsPageIt; + Page* erasedPage = maxUnusedItemsPageIt; + #ifndef NDEBUG size_t usedEntries = erasedPage->getUsedEntryCount(); #endif @@ -172,7 +174,7 @@ esp_err_t PageManager::requestNewPage() assert(usedEntries == newPage->getUsedEntryCount()); #endif - mPageList.erase(maxErasedItemsPageIt); + mPageList.erase(maxUnusedItemsPageIt); mFreePageList.push_back(erasedPage); return ESP_OK; diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index f419256c..0979bb40 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -1246,6 +1246,27 @@ TEST_CASE("multiple partitions access check", "[nvs]") CHECK(v2 == 0xcafebabe); } +TEST_CASE("nvs page selection takes into account free entries also not just erased entries", "[nvs]") +{ + const size_t blob_size = Page::BLOB_MAX_SIZE; + uint8_t blob[blob_size] = {0}; + SpiFlashEmulator emu(3); + TEST_ESP_OK( nvs_flash_init_custom(NVS_DEFAULT_PART_NAME, 0, 3) ); + nvs_handle handle; + TEST_ESP_OK( nvs_open("test", NVS_READWRITE, &handle) ); + // Fill first page + TEST_ESP_OK( nvs_set_blob(handle, "1a", blob, blob_size/3) ); + TEST_ESP_OK( nvs_set_blob(handle, "1b", blob, blob_size) ); + // Fill second page + TEST_ESP_OK( nvs_set_blob(handle, "2a", blob, blob_size) ); + TEST_ESP_OK( nvs_set_blob(handle, "2b", blob, blob_size) ); + + // The item below should be able to fit the first page. + TEST_ESP_OK( nvs_set_blob(handle, "3a", blob, 4) ); + TEST_ESP_OK( nvs_commit(handle) ); + nvs_close(handle); +} + TEST_CASE("dump all performance data", "[nvs]") { std::cout << "====================" << std::endl << "Dumping benchmarks" << std::endl; From ab9b14195326dfac8b5167c3ea31c53cba97f6df Mon Sep 17 00:00:00 2001 From: Dong Heng Date: Tue, 22 Jan 2019 11:30:05 +0800 Subject: [PATCH 2/4] fis(nvs_flash): Fix recovery after power-off during erase operation Current code for recovery after power-off do not clean-up partially erased items for FULL pages. If the erasure was part of modification operation, this gets luckily cleaned-up because of duplicate detection logic. For erase-only operation, the problem still exists. This patch adds the recovery for FULL pages also. esp-idf commit ID: 9a3c4b71 --- components/nvs_flash/src/nvs_page.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index 2a17cff2..a5ee0b6c 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -594,6 +594,16 @@ esp_err_t Page::mLoadEntryTable() assert(item.span > 0); size_t span = item.span; + + if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { + for (size_t j = i + 1; j < i + span; ++j) { + if (mEntryTable.get(j) != EntryState::WRITTEN) { + eraseEntryAndSpan(i); + break; + } + } + } + i += span - 1; } From e5c9b74005e79a1220e9cbcc213dd23ed485b411 Mon Sep 17 00:00:00 2001 From: Dong Heng Date: Tue, 22 Jan 2019 11:46:35 +0800 Subject: [PATCH 3/4] =?UTF-8?q?fix(nvs=5Fflash):=20don=E2=80=99t=20expect?= =?UTF-8?q?=20items=20with=20bad=20CRC=20to=20be=20in=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When erasing a variable length item with an incorrect CRC32, the span value of the item can not be trusted, so the item will be erased with span = 1. Subsequent entries represent the data of the variable length item, and these will be treated as separate items. For each entry CRC32 is checked, the check most likely fails (because the entry contains arbitrary data, and not a proper NVS item), and the entry is erased. Erase function assumed that every item should be present in cache, but it is not the case for the entries which are just parts of item’s payload. This change allows for the item to be not found in the hashlist, if the CRC32 check fails. esp-idf commit ID: 2c3644a0 --- .../nvs_flash/src/nvs_item_hash_list.cpp | 6 +++-- .../nvs_flash/src/nvs_item_hash_list.hpp | 2 +- components/nvs_flash/src/nvs_page.cpp | 22 ++++++++------- .../nvs_flash/test_nvs_host/test_nvs.cpp | 27 ++++++++++++++++--- 4 files changed, 40 insertions(+), 17 deletions(-) diff --git a/components/nvs_flash/src/nvs_item_hash_list.cpp b/components/nvs_flash/src/nvs_item_hash_list.cpp index e483c596..845dd091 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.cpp +++ b/components/nvs_flash/src/nvs_item_hash_list.cpp @@ -60,7 +60,7 @@ void HashList::insert(const Item& item, size_t index) newBlock->mCount++; } -void HashList::erase(size_t index) +void HashList::erase(size_t index, bool itemShouldExist) { for (auto it = mBlockList.begin(); it != mBlockList.end();) { bool haveEntries = false; @@ -82,7 +82,9 @@ void HashList::erase(size_t index) ++it; } } - assert(false && "item should have been present in cache"); + if (itemShouldExist) { + assert(false && "item should have been present in cache"); + } } size_t HashList::find(size_t start, const Item& item) diff --git a/components/nvs_flash/src/nvs_item_hash_list.hpp b/components/nvs_flash/src/nvs_item_hash_list.hpp index 3f8dcc85..e759cd81 100644 --- a/components/nvs_flash/src/nvs_item_hash_list.hpp +++ b/components/nvs_flash/src/nvs_item_hash_list.hpp @@ -29,7 +29,7 @@ public: ~HashList(); void insert(const Item& item, size_t index); - void erase(const size_t index); + void erase(const size_t index, bool itemShouldExist=true); size_t find(size_t start, const Item& item); void clear(); diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index a5ee0b6c..bf9007ae 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -318,7 +318,6 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) { auto state = mEntryTable.get(index); assert(state == EntryState::WRITTEN || state == EntryState::EMPTY); - mHashList.erase(index); size_t span = 1; if (state == EntryState::WRITTEN) { @@ -328,6 +327,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } if (item.calculateCrc32() != item.crc32) { + mHashList.erase(index, false); rc = alterEntryState(index, EntryState::ERASED); --mUsedEntryCount; ++mErasedEntryCount; @@ -335,6 +335,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index) return rc; } } else { + mHashList.erase(index); span = item.span; for (ptrdiff_t i = index + span - 1; i >= static_cast(index); --i) { if (mEntryTable.get(i) == EntryState::WRITTEN) { @@ -515,11 +516,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - - // search for potential duplicate item - size_t duplicateIndex = mHashList.find(0, item); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -529,6 +525,10 @@ esp_err_t Page::mLoadEntryTable() continue; } + mHashList.insert(item, i); + + // search for potential duplicate item + size_t duplicateIndex = mHashList.find(0, item); if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { span = item.span; @@ -580,8 +580,6 @@ esp_err_t Page::mLoadEntryTable() return err; } - mHashList.insert(item, i); - if (item.crc32 != item.calculateCrc32()) { err = eraseEntryAndSpan(i); if (err != ESP_OK) { @@ -590,9 +588,9 @@ esp_err_t Page::mLoadEntryTable() } continue; } - assert(item.span > 0); + mHashList.insert(item, i); size_t span = item.span; if (item.datatype == ItemType::BLOB || item.datatype == ItemType::SZ) { @@ -740,7 +738,11 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si auto crc32 = item.calculateCrc32(); if (item.crc32 != crc32) { - eraseEntryAndSpan(i); + rc = eraseEntryAndSpan(i); + if (rc != ESP_OK) { + mState = PageState::INVALID; + return rc; + } continue; } diff --git a/components/nvs_flash/test_nvs_host/test_nvs.cpp b/components/nvs_flash/test_nvs_host/test_nvs.cpp index 0979bb40..c23f5d66 100644 --- a/components/nvs_flash/test_nvs_host/test_nvs.cpp +++ b/components/nvs_flash/test_nvs_host/test_nvs.cpp @@ -132,14 +132,14 @@ TEST_CASE("when writing and erasing, used/erased counts are updated correctly", CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.getUsedEntryCount() == Page::ENTRY_COUNT - 1); CHECK(page.getErasedEntryCount() == 1); for (size_t i = 0; i < Page::ENTRY_COUNT - 2; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.eraseItem(1, itemTypeOf(), name) == ESP_OK); } CHECK(page.getUsedEntryCount() == 1); @@ -153,7 +153,7 @@ TEST_CASE("when page is full, adding an element fails", "[nvs]") CHECK(page.load(0) == ESP_OK); for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char name[16]; - snprintf(name, sizeof(name), "i%ld", i); + snprintf(name, sizeof(name), "i%ld", (long int)i); CHECK(page.writeItem(1, name, i) == ESP_OK); } CHECK(page.writeItem(1, "foo", 64UL) == ESP_ERR_NVS_PAGE_FULL); @@ -259,6 +259,25 @@ TEST_CASE("Page validates blob size", "[nvs]") TEST_ESP_OK(page.writeItem(1, ItemType::BLOB, "2", buf, Page::BLOB_MAX_SIZE)); } +TEST_CASE("Page handles invalid CRC of variable length items", "[nvs][cur]") +{ + SpiFlashEmulator emu(4); + { + Page page; + TEST_ESP_OK(page.load(0)); + char buf[128] = {0}; + TEST_ESP_OK(page.writeItem(1, ItemType::BLOB, "1", buf, sizeof(buf))); + } + // corrupt header of the item (64 is the offset of the first item in page) + uint32_t overwrite_buf = 0; + emu.write(64, &overwrite_buf, 4); + // load page again + { + Page page; + TEST_ESP_OK(page.load(0)); + } +} + TEST_CASE("can init PageManager in empty flash", "[nvs]") { SpiFlashEmulator emu(4); @@ -1123,7 +1142,7 @@ TEST_CASE("crc errors in item header are handled", "[nvs]") // add more items to make the page full for (size_t i = 0; i < Page::ENTRY_COUNT; ++i) { char item_name[Item::MAX_KEY_LENGTH + 1]; - snprintf(item_name, sizeof(item_name), "item_%ld", i); + snprintf(item_name, sizeof(item_name), "item_%ld", (long int)i); TEST_ESP_OK(storage.writeItem(1, item_name, static_cast(i))); } From b83c3a0c4d28fcd8187f1dbc469e29487bd4c86b Mon Sep 17 00:00:00 2001 From: Dong Heng Date: Tue, 22 Jan 2019 11:53:48 +0800 Subject: [PATCH 4/4] fix(nvs_flash): Fix recovery from power-off while page is being freed Currently when page is being freed, items are individually moved from FREEING page to ACTIVE page and erased. If power-off happens during the process, the remaining entries are moved to ACTIVE page during recovery. The problem with this approach is there may not be enough space on ACTIVE page for all items if an item was partially written before power-off and erased during recovery. This change moves all the items from FREEING to ACTIVE page and then erased the FREEING page, If power-off happens during the process, then ACTIVE page is erased and the process is restarted. esp-idf commit ID: 7ae1df1c --- components/nvs_flash/src/nvs_page.cpp | 44 +++++++++++++------- components/nvs_flash/src/nvs_page.hpp | 2 +- components/nvs_flash/src/nvs_pagemanager.cpp | 43 +++++++++---------- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/components/nvs_flash/src/nvs_page.cpp b/components/nvs_flash/src/nvs_page.cpp index bf9007ae..efadc185 100644 --- a/components/nvs_flash/src/nvs_page.cpp +++ b/components/nvs_flash/src/nvs_page.cpp @@ -386,7 +386,7 @@ void Page::updateFirstUsedEntry(size_t index, size_t span) } } -esp_err_t Page::moveItem(Page& other) +esp_err_t Page::copyItems(Page& other) { if (mFirstUsedEntry == INVALID_ENTRY) { return ESP_ERR_NVS_NOT_FOUND; @@ -400,29 +400,41 @@ esp_err_t Page::moveItem(Page& other) } Item entry; - auto err = readEntry(mFirstUsedEntry, entry); - if (err != ESP_OK) { - return err; - } - other.mHashList.insert(entry, other.mNextFreeEntry); - err = other.writeEntry(entry); - if (err != ESP_OK) { - return err; - } + size_t readEntryIndex = mFirstUsedEntry; - size_t span = entry.span; - size_t end = mFirstUsedEntry + span; + while (readEntryIndex < ENTRY_COUNT) { - assert(mFirstUsedEntry != INVALID_ENTRY || span == 1); + if (mEntryTable.get(readEntryIndex) != EntryState::WRITTEN) { + assert(readEntryIndex != mFirstUsedEntry); + readEntryIndex++; + continue; + } + auto err = readEntry(readEntryIndex, entry); + if (err != ESP_OK) { + return err; + } - for (size_t i = mFirstUsedEntry + 1; i < end; ++i) { - readEntry(i, entry); + other.mHashList.insert(entry, other.mNextFreeEntry); err = other.writeEntry(entry); if (err != ESP_OK) { return err; } + size_t span = entry.span; + size_t end = readEntryIndex + span; + + assert(end <= ENTRY_COUNT); + + for (size_t i = readEntryIndex + 1; i < end; ++i) { + readEntry(i, entry); + err = other.writeEntry(entry); + if (err != ESP_OK) { + return err; + } + } + readEntryIndex = end; + } - return eraseEntryAndSpan(mFirstUsedEntry); + return ESP_OK; } esp_err_t Page::mLoadEntryTable() diff --git a/components/nvs_flash/src/nvs_page.hpp b/components/nvs_flash/src/nvs_page.hpp index 7731e403..413da458 100644 --- a/components/nvs_flash/src/nvs_page.hpp +++ b/components/nvs_flash/src/nvs_page.hpp @@ -127,7 +127,7 @@ public: esp_err_t markFreeing(); - esp_err_t moveItem(Page& other); + esp_err_t copyItems(Page& other); esp_err_t erase(); diff --git a/components/nvs_flash/src/nvs_pagemanager.cpp b/components/nvs_flash/src/nvs_pagemanager.cpp index cb274619..31240d98 100644 --- a/components/nvs_flash/src/nvs_pagemanager.cpp +++ b/components/nvs_flash/src/nvs_pagemanager.cpp @@ -67,7 +67,9 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) if (lastItemIndex != SIZE_MAX) { auto last = PageManager::TPageListIterator(&lastPage); for (auto it = begin(); it != last; ++it) { - if (it->eraseItem(item.nsIndex, item.datatype, item.key) == ESP_OK) { + + if ((it->state() != Page::PageState::FREEING) && + (it->eraseItem(item.nsIndex, item.datatype, item.key) == ESP_OK)) { break; } } @@ -77,23 +79,26 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) for (auto it = begin(); it!= end(); ++it) { if (it->state() == Page::PageState::FREEING) { Page* newPage = &mPageList.back(); - if (newPage->state() != Page::PageState::ACTIVE) { - auto err = activatePage(); + if (newPage->state() == Page::PageState::ACTIVE) { + auto err = newPage->erase(); if (err != ESP_OK) { return err; } - newPage = &mPageList.back(); + mPageList.erase(newPage); + mFreePageList.push_back(newPage); } - while (true) { - auto err = it->moveItem(*newPage); - if (err == ESP_ERR_NVS_NOT_FOUND) { - break; - } else if (err != ESP_OK) { - return err; - } + auto err = activatePage(); + if (err != ESP_OK) { + return err; + } + newPage = &mPageList.back(); + + err = it->copyItems(*newPage); + if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { + return err; } - auto err = it->erase(); + err = it->erase(); if (err != ESP_OK) { return err; } @@ -109,7 +114,7 @@ esp_err_t PageManager::load(uint32_t baseSector, uint32_t sectorCount) if (mFreePageList.size() == 0) { return ESP_ERR_NVS_NO_FREE_PAGES; } - + return ESP_OK; } @@ -156,13 +161,9 @@ esp_err_t PageManager::requestNewPage() if (err != ESP_OK) { return err; } - while (true) { - err = erasedPage->moveItem(*newPage); - if (err == ESP_ERR_NVS_NOT_FOUND) { - break; - } else if (err != ESP_OK) { - return err; - } + err = erasedPage->copyItems(*newPage); + if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) { + return err; } err = erasedPage->erase(); @@ -173,7 +174,7 @@ esp_err_t PageManager::requestNewPage() #ifndef NDEBUG assert(usedEntries == newPage->getUsedEntryCount()); #endif - + mPageList.erase(maxUnusedItemsPageIt); mFreePageList.push_back(erasedPage);