Merge pull request #9200 from davidsaada/david_tdbstore_gc_if_corrupt

TDBStore: Perform garbage collection on failed writes

Author: 0xc0170

features/storage/TESTS/kvstore/general_tests_phase_1/main.cpp

@@ -258,9 +258,7 @@ static void set_several_keys_multithreaded()
     for (i = 0; i < num_of_threads; i++) {
         res = kvstore->get(keys[i], buffer, buffer_size, &actual_size, 0);
         TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
-
         TEST_ASSERT_EQUAL_STRING_LEN(buffer, data, data_size);
-
     }
 
     for (i = 0; i < num_of_threads; i++) {
@@ -329,6 +327,7 @@ static void set_key_value_two_byte_size()
     TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
 
     res = kvstore->get(key, buffer, buffer_size, &actual_size, 0);
+    TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
     TEST_ASSERT_EQUAL_STRING_LEN(buffer, data_two, 1);
     memset(buffer, 0, buffer_size);
 
@@ -346,6 +345,7 @@ static void set_key_value_five_byte_size()
     TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
 
     res = kvstore->get(key, buffer, buffer_size, &actual_size, 0);
+    TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
     TEST_ASSERT_EQUAL_STRING_LEN(buffer, data_five, 4);
     memset(buffer, 0, buffer_size);
 
@@ -363,6 +363,7 @@ static void set_key_value_fifteen_byte_size()
     TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
 
     res = kvstore->get(key, buffer, buffer_size, &actual_size, 0);
+    TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
     TEST_ASSERT_EQUAL_STRING_LEN(buffer, data_fif, 14);
     memset(buffer, 0, buffer_size);
 
@@ -380,6 +381,7 @@ static void set_key_value_seventeen_byte_size()
     TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
 
     res = kvstore->get(key, buffer, buffer_size, &actual_size, 0);
+    TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
     TEST_ASSERT_EQUAL_STRING_LEN(buffer, data_fif, 16);
     memset(buffer, 0, buffer_size);
 
@@ -451,7 +453,8 @@ static void set_key_init_deinit()
     TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
 
     res = kvstore->get(key, buffer, buffer_size, &actual_size, 0);
-    TEST_ASSERT_EQUAL_STRING(buffer, data);
+    TEST_ASSERT_EQUAL_ERROR_CODE(MBED_SUCCESS, res);
+    TEST_ASSERT_EQUAL_STRING(data, buffer);
     memset(buffer, 0, buffer_size);
 
     res = kvstore->remove(key);

features/storage/kvstore/tdbstore/TDBStore.cpp

@@ -390,6 +390,7 @@ int TDBStore::set_start(set_handle_t *handle, const char *key, size_t final_data
     uint32_t offset;
     uint32_t hash, ram_table_ind;
     inc_set_handle_t *ih;
+    bool need_gc = false;
 
     if (!is_valid_key(key)) {
         return MBED_ERROR_INVALID_ARGUMENT;
@@ -485,14 +486,19 @@ int TDBStore::set_start(set_handle_t *handle, const char *key, size_t final_data
     // Write key now
     ret = write_area(_active_area, ih->bd_curr_offset, ih->header.key_size, key);
     if (ret) {
+        need_gc = true;
         goto fail;
     }
     ih->bd_curr_offset += ih->header.key_size;
     goto end;
 
 fail:
+    if ((need_gc) && (ih->bd_base_offset != _master_record_offset)) {
+        garbage_collection();
+    }
     // mark handle as invalid by clearing magic field in header
     ih->header.magic = 0;
+
     _mutex.unlock();
 
 end:
@@ -503,6 +509,7 @@ int TDBStore::set_add_data(set_handle_t handle, const void *value_data, size_t d
 {
     int ret = MBED_SUCCESS;
     inc_set_handle_t *ih;
+    bool need_gc = false;
 
     if (handle != _inc_set_handle) {
         return MBED_ERROR_INVALID_ARGUMENT;
@@ -532,12 +539,17 @@ int TDBStore::set_add_data(set_handle_t handle, const void *value_data, size_t d
     // Write the data chunk
     ret = write_area(_active_area, ih->bd_curr_offset, data_size, value_data);
     if (ret) {
+        need_gc = true;
         goto end;
     }
     ih->bd_curr_offset += data_size;
     ih->offset_in_data += data_size;
 
 end:
+    if ((need_gc) && (ih->bd_base_offset != _master_record_offset)) {
+        garbage_collection();
+    }
+
     _inc_set_mutex.unlock();
     return ret;
 }
@@ -548,6 +560,8 @@ int TDBStore::set_finalize(set_handle_t handle)
     inc_set_handle_t *ih;
     ram_table_entry_t *ram_table = (ram_table_entry_t *) _ram_table;
     ram_table_entry_t *entry;
+    bool need_gc = false;
+    uint32_t actual_data_size, hash, flags, next_offset;
 
     if (handle != _inc_set_handle) {
         return MBED_ERROR_INVALID_ARGUMENT;
@@ -563,21 +577,22 @@ int TDBStore::set_finalize(set_handle_t handle)
 
     if (ih->offset_in_data != ih->header.data_size) {
         ret = MBED_ERROR_INVALID_SIZE;
-        // Need GC as otherwise our storage is left in a non-usable state
-        garbage_collection();
+        need_gc = true;
         goto end;
     }
 
     // Write header
     ret = write_area(_active_area, ih->bd_base_offset, sizeof(record_header_t), &ih->header);
     if (ret) {
+        need_gc = true;
         goto end;
     }
 
     // Need to flush buffered BD as our record is totally written now
     os_ret = _buff_bd->sync();
     if (os_ret) {
         ret = MBED_ERROR_WRITE_FAILED;
+        need_gc = true;
         goto end;
     }
 
@@ -586,6 +601,16 @@ int TDBStore::set_finalize(set_handle_t handle)
         goto end;
     }
 
+    // Writes may fail without returning a failure (specially in flash components). Reread the record
+    // to ensure write success (this won't read the data anywhere - just use the CRC calculation).
+    ret = read_record(_active_area, ih->bd_base_offset, 0, 0, (uint32_t) -1,
+                      actual_data_size, 0, false, false, false, false,
+                      hash, flags, next_offset);
+    if (ret) {
+        need_gc = true;
+        goto end;
+    }
+
     // Update RAM table
     if (ih->header.flags & delete_flag) {
         _num_keys--;
@@ -611,10 +636,15 @@ int TDBStore::set_finalize(set_handle_t handle)
     _free_space_offset = align_up(ih->bd_curr_offset, _prog_size);
 
 end:
+    if ((need_gc) && (ih->bd_base_offset != _master_record_offset)) {
+        garbage_collection();
+    }
+
     // mark handle as invalid by clearing magic field in header
     ih->header.magic = 0;
 
     _inc_set_mutex.unlock();
+
     if (ih->bd_base_offset != _master_record_offset) {
         _mutex.unlock();
     }