[nrf fromlist] settings: zms: add more robustness to the save function

When a power off happens after writing the settings name and before
writing the linked list node we cannot write the settings name again in
the future.
Fix this by writing the linked list node before writing the name.
When loading all settings, we already delete linked list node that do
not have any name or value written.
Adds as well a recover path if a power down happens in the middle of
unlinking an LL node after a delete.

Upstream PR #: 87792

Signed-off-by: Riadh Ghaddab <[email protected]>

Author: rghaddab

subsys/settings/include/settings/settings_zms.h

@@ -72,6 +72,7 @@ extern "C" {
 
 /* some useful macros */
 #define ZMS_NAME_ID_FROM_LL_NODE(x) (x & ~BIT(0))
+#define ZMS_LL_NODE_FROM_NAME_ID(x) (x | BIT(0))
 #define ZMS_UPDATE_COLLISION_NUM(x, y)                                                             \
 	((x & ~ZMS_COLLISIONS_MASK) | ((y << 1) & ZMS_COLLISIONS_MASK))
 #define ZMS_COLLISION_NUM(x)     ((x & ZMS_COLLISIONS_MASK) >> 1)

subsys/settings/src/settings_zms.c

@@ -72,11 +72,39 @@ static int settings_zms_unlink_ll_node(struct settings_zms *cf, uint32_t name_ha
 	struct settings_hash_linked_list settings_update_element;
 
 	/* let's update the linked list */
-	rc = zms_read(&cf->cf_zms, name_hash | 1, &settings_element,
+	rc = zms_read(&cf->cf_zms, ZMS_LL_NODE_FROM_NAME_ID(name_hash), &settings_element,
 		      sizeof(struct settings_hash_linked_list));
 	if (rc < 0) {
 		return rc;
 	}
+
+	/* update the previous element */
+	if (settings_element.previous_hash) {
+		rc = zms_read(&cf->cf_zms, settings_element.previous_hash, &settings_update_element,
+			      sizeof(struct settings_hash_linked_list));
+		if (rc < 0) {
+			return rc;
+		}
+		if (!settings_element.next_hash) {
+			/* we are deleting the last element of the linked list,
+			 * let's update the second_to_last_hash_id
+			 */
+			cf->second_to_last_hash_id = settings_update_element.previous_hash;
+		}
+		settings_update_element.next_hash = settings_element.next_hash;
+		rc = zms_write(&cf->cf_zms, settings_element.previous_hash,
+			       &settings_update_element, sizeof(struct settings_hash_linked_list));
+		if (rc < 0) {
+			return rc;
+		}
+	}
+
+	/* Now delete the current linked list element */
+	rc = zms_delete(&cf->cf_zms, ZMS_LL_NODE_FROM_NAME_ID(name_hash));
+	if (rc < 0) {
+		return rc;
+	}
+
 	/* update the next element */
 	if (settings_element.next_hash) {
 		rc = zms_read(&cf->cf_zms, settings_element.next_hash, &settings_update_element,
@@ -100,28 +128,8 @@ static int settings_zms_unlink_ll_node(struct settings_zms *cf, uint32_t name_ha
 		 */
 		cf->last_hash_id = settings_element.previous_hash;
 	}
-	/* update the previous element */
-	if (settings_element.previous_hash) {
-		rc = zms_read(&cf->cf_zms, settings_element.previous_hash, &settings_update_element,
-			      sizeof(struct settings_hash_linked_list));
-		if (rc < 0) {
-			return rc;
-		}
-		if (!settings_element.next_hash) {
-			/* we are deleting the last element of the linked list,
-			 * let's update the second_to_last_hash_id
-			 */
-			cf->second_to_last_hash_id = settings_update_element.previous_hash;
-		}
-		settings_update_element.next_hash = settings_element.next_hash;
-		rc = zms_write(&cf->cf_zms, settings_element.previous_hash,
-			       &settings_update_element, sizeof(struct settings_hash_linked_list));
-		if (rc < 0) {
-			return rc;
-		}
-	}
 
-	return rc;
+	return 0;
 }
 #endif /* CONFIG_SETTINGS_ZMS_NO_LL_DELETE */
 
@@ -182,11 +190,6 @@ static int settings_zms_delete(struct settings_zms *cf, uint32_t name_hash)
 		return rc;
 	}
 
-	/* Now delete the current linked list element */
-	rc = zms_delete(&cf->cf_zms, name_hash | 1);
-	if (rc < 0) {
-		return rc;
-	}
 #endif /* CONFIG_SETTINGS_ZMS_NO_LL_DELETE */
 #ifdef CONFIG_SETTINGS_ZMS_NAME_CACHE
 	/* Update the flag of the Settings entry in cache. */
@@ -513,16 +516,13 @@ static int settings_zms_save(struct settings_store *cs, const char *name, const
 
 	/* write the name if required */
 	if (write_name) {
-		rc = zms_write(&cf->cf_zms, name_hash, name, strlen(name));
-		if (rc < 0) {
-			return rc;
-		}
+		/* First let's update the linked list */
 #ifdef CONFIG_SETTINGS_ZMS_NO_LL_DELETE
 		if (ll_node_exist) {
 			goto no_ll_update;
 		}
 		/* verify that the ll_node doesn't exist otherwise do not update it */
-		rc = zms_read(&cf->cf_zms, name_hash | 1, &settings_element,
+		rc = zms_read(&cf->cf_zms, ZMS_LL_NODE_FROM_NAME_ID(name_hash), &settings_element,
 			      sizeof(struct settings_hash_linked_list));
 		if (rc >= 0) {
 			goto no_ll_update;
@@ -544,7 +544,7 @@ static int settings_zms_save(struct settings_store *cs, const char *name, const
 			}
 		}
 		settings_element.previous_hash = cf->last_hash_id;
-		rc = zms_write(&cf->cf_zms, name_hash | 1, &settings_element,
+		rc = zms_write(&cf->cf_zms, ZMS_LL_NODE_FROM_NAME_ID(name_hash), &settings_element,
 			       sizeof(struct settings_hash_linked_list));
 		if (rc < 0) {
 			return rc;
@@ -555,25 +555,31 @@ static int settings_zms_save(struct settings_store *cs, const char *name, const
 		}
 #endif
 		/* Now update the previous linked list element */
-		settings_element.next_hash = name_hash | 1;
+		settings_element.next_hash = ZMS_LL_NODE_FROM_NAME_ID(name_hash);
 		settings_element.previous_hash = cf->second_to_last_hash_id;
 		rc = zms_write(&cf->cf_zms, cf->last_hash_id, &settings_element,
 			       sizeof(struct settings_hash_linked_list));
 		if (rc < 0) {
 			return rc;
 		}
 		cf->second_to_last_hash_id = cf->last_hash_id;
-		cf->last_hash_id = name_hash | 1;
+		cf->last_hash_id = ZMS_LL_NODE_FROM_NAME_ID(name_hash);
 #ifdef CONFIG_SETTINGS_ZMS_LL_CACHE
 		if ((cf->ll_cache_next <= CONFIG_SETTINGS_ZMS_LL_CACHE_SIZE) &&
 		    (cf->ll_cache_next > 1)) {
 			cf->ll_cache[cf->ll_cache_next - 2] = settings_element;
 		}
 #endif
-	}
 #ifdef CONFIG_SETTINGS_ZMS_NO_LL_DELETE
 no_ll_update:
 #endif /* CONFIG_SETTINGS_ZMS_NO_LL_DELETE */
+		/* Now let's write the name */
+		rc = zms_write(&cf->cf_zms, name_hash, name, strlen(name));
+		if (rc < 0) {
+			return rc;
+		}
+	}
+
 #ifdef CONFIG_SETTINGS_ZMS_NAME_CACHE
 	/* Add the flags of the written settings entry in cache */
 	cache_flags = 0;
@@ -590,6 +596,7 @@ static int settings_zms_get_last_hash_ids(struct settings_zms *cf)
 {
 	struct settings_hash_linked_list settings_element;
 	uint32_t ll_last_hash_id = ZMS_LL_HEAD_HASH_ID;
+	uint32_t previous_ll_hash_id = 0;
 	int rc = 0;
 
 #ifdef CONFIG_SETTINGS_ZMS_LL_CACHE
@@ -621,6 +628,27 @@ static int settings_zms_get_last_hash_ids(struct settings_zms *cf)
 			return rc;
 		}
 
+		if (settings_element.previous_hash != previous_ll_hash_id) {
+			/* This is a special case that can happen when a power down occurred
+			 * when deleting a linked list node.
+			 * If the power down occurred after updating the previous linked list node,
+			 * then we would end up with a state where the previous_hash of the linked
+			 * list is broken. Let's recover from this
+			 */
+			rc = zms_delete(&cf->cf_zms, settings_element.previous_hash);
+			if (rc < 0) {
+				return rc;
+			}
+			/* Now recover the linked list */
+			settings_element.previous_hash = previous_ll_hash_id;
+			zms_write(&cf->cf_zms, ll_last_hash_id, &settings_element,
+				  sizeof(struct settings_hash_linked_list));
+			if (rc < 0) {
+				return rc;
+			}
+		}
+		previous_ll_hash_id = ll_last_hash_id;
+
 #ifdef CONFIG_SETTINGS_ZMS_LL_CACHE
 		if (cf->ll_cache_next <= CONFIG_SETTINGS_ZMS_LL_CACHE_SIZE) {
 			cf->ll_cache[cf->ll_cache_next++] = settings_element;