Merge branch 'bugfix/nvs_page_compaction' into 'master'

nvs: Fix recovery from power-off while page is being freed

See merge request idf/esp-idf!2327

components/nvs_flash/src/nvs_page.cpp

@@ -382,7 +382,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;
@@ -396,29 +396,41 @@ esp_err_t Page::moveItem(Page& other)
     }
 
     Item entry;
-    auto err = readEntry(mFirstUsedEntry, entry);
+    size_t readEntryIndex = mFirstUsedEntry;
+
+    while (readEntryIndex < ENTRY_COUNT) {
+
+        if (mEntryTable.get(readEntryIndex) != EntryState::WRITTEN) {
+            assert(readEntryIndex != mFirstUsedEntry);
+            readEntryIndex++;
+            continue;
+        }
+        auto err = readEntry(readEntryIndex, 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 span = entry.span;
-    size_t end = mFirstUsedEntry + span;
+        size_t end = readEntryIndex + span;
 
-    assert(mFirstUsedEntry != INVALID_ENTRY || span == 1);
+        assert(end <= ENTRY_COUNT);
 
-    for (size_t i = mFirstUsedEntry + 1; i < end; ++i) {
+        for (size_t i = readEntryIndex + 1; i < end; ++i) {
             readEntry(i, entry);
             err = other.writeEntry(entry);
             if (err != ESP_OK) {
                 return err;
             }
         }
-    return eraseEntryAndSpan(mFirstUsedEntry);
+        readEntryIndex = end;
+
+    }
+    return ESP_OK;
 }
 
 esp_err_t Page::mLoadEntryTable()

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();
 

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) {
+            if (newPage->state() == Page::PageState::ACTIVE) {
+                auto err = newPage->erase();
+                if (err != ESP_OK) {
+                    return err;
+                }
+                mPageList.erase(newPage);
+                mFreePageList.push_back(newPage);
+            }
             auto err = activatePage();
             if (err != ESP_OK) {
                 return err;
             }
             newPage = &mPageList.back();
-            }
+
-            while (true) {
+            err = it->copyItems(*newPage);
-                auto err = it->moveItem(*newPage);
+            if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
-                if (err == ESP_ERR_NVS_NOT_FOUND) {
-                    break;
-                } else if (err != ESP_OK) {
                 return err;
             }
-            }
 
-            auto err = it->erase();
+            err = it->erase();
             if (err != ESP_OK) {
                 return err;
             }
@@ -156,14 +161,10 @@ esp_err_t PageManager::requestNewPage()
     if (err != ESP_OK) {
         return err;
     }
-    while (true) {
+    err = erasedPage->copyItems(*newPage);
-        err = erasedPage->moveItem(*newPage);
+    if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
-        if (err == ESP_ERR_NVS_NOT_FOUND) {
-            break;
-        } else if (err != ESP_OK) {
         return err;
     }
-    }
 
     err = erasedPage->erase();
     if (err != ESP_OK) {

components/nvs_flash/test_nvs_host/test_nvs.cpp

@@ -1458,6 +1458,48 @@ TEST_CASE("Recovery from power-off when the entry being erased is not on active
     nvs_close(handle);
 }
 
+TEST_CASE("Recovery from power-off when page is being freed.", "[nvs]")
+{
+    const size_t blob_size = (Page::ENTRY_COUNT-3) * Page::ENTRY_SIZE;
+    size_t read_size = blob_size/2;
+    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/3));
+    TEST_ESP_OK(nvs_set_blob(handle, "1c", blob, blob_size/4));
+    // Fill second page
+    TEST_ESP_OK(nvs_set_blob(handle, "2a", blob, blob_size/2));
+    TEST_ESP_OK(nvs_set_blob(handle, "2b", blob, blob_size/2));
+
+    TEST_ESP_OK(nvs_erase_key(handle, "1c"));
+
+    emu.clearStats();
+    emu.failAfter(6 * Page::ENTRY_COUNT);
+    TEST_ESP_ERR(nvs_set_blob(handle, "1d", blob, blob_size/4), ESP_ERR_FLASH_OP_FAIL);
+
+    TEST_ESP_OK(nvs_flash_init_custom(NVS_DEFAULT_PART_NAME, 0, 3));
+
+    read_size = blob_size/3;
+    TEST_ESP_OK( nvs_get_blob(handle, "1a", blob, &read_size));
+    TEST_ESP_OK( nvs_get_blob(handle, "1b", blob, &read_size));
+
+    read_size = blob_size /4;
+    TEST_ESP_ERR( nvs_get_blob(handle, "1c", blob, &read_size), ESP_ERR_NVS_NOT_FOUND);
+    TEST_ESP_ERR( nvs_get_blob(handle, "1d", blob, &read_size), ESP_ERR_NVS_NOT_FOUND);
+
+    read_size = blob_size /2;
+    TEST_ESP_OK( nvs_get_blob(handle, "2a", blob, &read_size));
+    TEST_ESP_OK( nvs_get_blob(handle, "2b", blob, &read_size));
+
+    TEST_ESP_OK(nvs_commit(handle));
+    nvs_close(handle);
+}
+
+
 /* Add new tests above */
 /* This test has to be the final one */