DAOS-16954 Fix the tree->embedded case + simplify the logic

In the previous commit, I made a mistake when fixing the embedded value
case. Because it remained unnoticed, I think it is fair to say the current
`btr_root_del_rec()` logic is a little confusing. With this commit, I
hope to both fix my mistake and simplify the logic so this kind of
mistake won't happen in the future.

What is simplified:

- if it is an embedded value destroy it straight away,
- mostly restore the pre-embedded value logic without consideration for
  embedded values with one exception,
  - if the root is a leaf and there is more than one key remaining:
    - if there are exactly two keys and the embedded value is supported,
      the tree should be replaced with an embedded value.

Since both:
- deleting the embedded value and
- deleting the last key from a regular tree,
are the same a `btr_root_del_rec_last()` helper function has been
introduced.

Signed-off-by: Jan Michalski <[email protected]>
Co-authored-by: Tomasz Gromadzki <[email protected]>

src/common/btree.c

@@ -3217,9 +3217,47 @@ btr_node_del_rec(struct btr_context *tcx, struct btr_trace *par_tr,
 				  sib_on_right, args);
 }
 
+/**
+ * Delete the last record from the tree as pointed by @trace.
+ *
+ * It handles both the embedded record and a record stored in a regular tree.
+ */
+static inline int
+btr_root_del_rec_last(struct btr_context *tcx, struct btr_trace *trace, void *args)
+{
+	struct btr_root *root = tcx->tc_tins.ti_root;
+	int              rc;
+
+	rc = btr_node_destroy(tcx, trace->tr_node, args, NULL);
+	if (rc != 0)
+		return rc;
+
+	if (btr_has_tx(tcx)) {
+		rc = btr_root_tx_add(tcx);
+		if (rc != 0)
+			return rc;
+	}
+
+	root->tr_depth = 0;
+	root->tr_node  = BTR_NODE_NULL;
+
+	if (btr_has_embedded_value(tcx)) {
+		root->tr_feats ^= BTR_FEAT_EMBEDDED;
+		tcx->tc_feats = root->tr_feats;
+	}
+
+	btr_context_set_depth(tcx, 0);
+	D_DEBUG(DB_TRACE, "Tree is empty now.\n");
+
+	return 0;
+}
+
 /**
  * Deleted the record/child pointed by @trace from the root node.
  *
+ * - If the remaining value is embedded it will be freed.
+ * - If only one record is left in the root and the embedded value is supported, it will become an
+ * embedded value.
  * - If the root node is also a leaf, and the root is empty after the deletion,
  *   then the root node will be freed as well.
  * - If the root node is a non-leaf node, then the corresponding child node
@@ -3229,75 +3267,42 @@ btr_node_del_rec(struct btr_context *tcx, struct btr_trace *par_tr,
 static int
 btr_root_del_rec(struct btr_context *tcx, struct btr_trace *trace, void *args)
 {
-	struct btr_node *node      = NULL;
-	struct btr_root *root      = tcx->tc_tins.ti_root;
-	int              rc        = 0;
-	int              threshold = 1;
-
-	if ((tcx->tc_trace.ti_embedded_info & BTR_EMBEDDED_SET) == 0) {
-		node = btr_off2ptr(tcx, trace->tr_node);
-	} else {
-		D_ASSERT(btr_has_embedded_value(tcx));
-	}
+	struct btr_node *node = NULL;
+	struct btr_root *root = tcx->tc_tins.ti_root;
+	int              rc   = 0;
 
 	D_DEBUG(DB_TRACE, "Delete record/child from tree root, depth=%d\n",
 		root->tr_depth);
 
-	if (btr_has_embedded_value(tcx) || btr_node_is_leaf(tcx, trace->tr_node)) {
-		if (D_LOG_ENABLED(DB_TRACE)) {
-			if (btr_has_embedded_value(tcx)) {
-				D_DEBUG(DB_TRACE, "Delete embedded record from the root\n");
-			} else {
-				D_DEBUG(DB_TRACE, "Delete leaf from the root, key_nr=%d.\n",
-					node->tn_keyn);
-			}
-		}
+	if (btr_has_embedded_value(tcx)) {
+		D_DEBUG(DB_TRACE, "Delete the embedded record\n");
+		return btr_root_del_rec_last(tcx, trace, args);
+	}
 
-		if (btr_supports_embedded_value(tcx)) {
-			/* If a tree supports embedded values, the minimal number of keys that must
-			 * exist in an actual tree is 2. Below this threshold, the tree is deleted,
-			 * and the remaining key is stored as embedded. */
-			threshold = 2;
-		}
+	D_ASSERT((tcx->tc_trace.ti_embedded_info & BTR_EMBEDDED_SET) == 0);
+	node = btr_off2ptr(tcx, trace->tr_node);
 
-		if (node != NULL) {
-			/* the root is also a leaf node */
-			if (node->tn_keyn > threshold) {
-				/* have more than one record, simply remove the record
-				 * to be deleted.
-				 */
-				if (btr_has_tx(tcx)) {
-					rc = btr_node_tx_add(tcx, trace->tr_node);
-					if (rc != 0)
-						return rc;
-				}
-
-				rc = btr_node_del_leaf_only(tcx, trace, true, args);
-			} else if (node->tn_keyn == threshold) {
-				rc = btr_node_del_embed(tcx, trace, root, args);
-			}
-		}
+	if (btr_node_is_leaf(tcx, trace->tr_node)) {
+		/* the root is also a leaf node */
+		D_DEBUG(DB_TRACE, "Delete leaf from the root, key_nr=%d.\n", node->tn_keyn);
 
-		if (node == NULL || node->tn_keyn < threshold) {
-			rc = btr_node_destroy(tcx, trace->tr_node, args, NULL);
-			if (rc != 0)
-				return rc;
+		if (node->tn_keyn > 1) {
+			/* When removing the second to last record and embedding value is supported
+			 * the remaining record will become an embedded one. */
+			if (node->tn_keyn == 2 && btr_supports_embedded_value(tcx)) {
+				return btr_node_del_embed(tcx, trace, root, args);
+			}
 
+			/* have more than one record, simply remove the record to be deleted. */
 			if (btr_has_tx(tcx)) {
-				rc = btr_root_tx_add(tcx);
+				rc = btr_node_tx_add(tcx, trace->tr_node);
 				if (rc != 0)
 					return rc;
 			}
 
-			root->tr_depth	= 0;
-			root->tr_node	= BTR_NODE_NULL;
-			if (btr_has_embedded_value(tcx)) {
-				root->tr_feats ^= BTR_FEAT_EMBEDDED;
-				tcx->tc_feats = root->tr_feats;
-			}
-
-			btr_context_set_depth(tcx, 0);
-			D_DEBUG(DB_TRACE, "Tree is empty now.\n");
+			rc = btr_node_del_leaf_only(tcx, trace, true, args);
+		} else {
+			return btr_root_del_rec_last(tcx, trace, args);
 		}
 	} else {
 		/* non-leaf node */