DAOS-19265 pool: refine pool_discard() target validation

src/pool/srv_pool.c

@@ -7964,6 +7964,93 @@ pool_svc_update_map(struct pool_svc *svc, crt_opcode_t opc, bool exclude_rank,
 	return rc;
 }
 
+/*
+ * Validate/filter the pool_target_addr array against the current pool map, so that
+ * ds_pool_tgt_discard_ult() on the target engines no longer needs to rely on their own
+ * (possibly stale) pool map to decide which targets to exclude from the discard.
+ *
+ * - For REINT, only targets currently in PO_COMP_ST_DOWN/PO_COMP_ST_DOWNOUT are kept,
+ *   those are the ones about to be reintegrated and whose stale data needs discarding.
+ * - For EXTEND, only targets currently in PO_COMP_ST_NEW are kept (or targets that are
+ *   not part of the pool map yet at all, i.e. brand new ranks/targets being added, which
+ *   can't hold any pool data yet and are therefore always safe to discard).
+ *
+ * \a valid_list is allocated by this function and must be freed by the caller with
+ * pool_target_addr_list_free(), regardless of the return value.
+ */
+static int
+pool_discard_filter_tgts(struct pool_svc *svc, struct pool_target_addr_list *list, bool reint,
+			 struct pool_target_addr_list *valid_list)
+{
+	struct pool_map *map;
+	uint32_t         wanted_status, disallow_status;
+	int              i;
+	int              rc = 0;
+
+	memset(valid_list, 0, sizeof(*valid_list));
+	wanted_status   = reint ? (PO_COMP_ST_DOWN | PO_COMP_ST_DOWNOUT) : PO_COMP_ST_NEW;
+	disallow_status = reint ? (PO_COMP_ST_NEW | PO_COMP_ST_DRAIN)
+				: (PO_COMP_ST_DOWN | PO_COMP_ST_DRAIN | PO_COMP_ST_DOWNOUT);
+
+	ABT_rwlock_rdlock(svc->ps_pool->sp_lock);
+	map = svc->ps_pool->sp_map;
+	for (i = 0; i < list->pta_number; i++) {
+		struct pool_target_addr *addr = &list->pta_addrs[i];
+		struct pool_target      *tgts = NULL;
+		int                      tgt_nr;
+		int                      j;
+
+		tgt_nr =
+		    pool_map_find_target_by_rank_idx(map, addr->pta_rank, addr->pta_target, &tgts);
+		if (tgt_nr <= 0) {
+			if (reint) {
+				D_INFO(DF_UUID ": discard skip rank %u target %u: not in pool map.",
+				       DP_UUID(svc->ps_pool->sp_uuid), addr->pta_rank,
+				       addr->pta_target);
+				continue;
+			}
+
+			/* Brand new rank/target not part of the pool map yet, i.e. this
+			 * is the initial EXTEND for it, nothing to protect, keep as is.
+			 */
+			rc = pool_target_addr_list_append(valid_list, addr);
+			if (rc != 0)
+				goto out;
+			continue;
+		}
+
+		for (j = 0; j < tgt_nr; j++) {
+			struct pool_target_addr valid_addr;
+
+			if (tgts[j].ta_comp.co_status & disallow_status) {
+				rc = -DER_BUSY;
+				DL_ERROR(rc, DF_UUID ": Can't %s rank %u target %u: status %u.",
+					 reint ? "REINT" : "EXTEND", DP_UUID(svc->ps_pool->sp_uuid),
+					 addr->pta_rank, tgts[j].ta_comp.co_id,
+					 tgts[j].ta_comp.co_status);
+				goto out;
+			}
+
+			if (!(tgts[j].ta_comp.co_status & wanted_status)) {
+				D_INFO(DF_UUID ": discard skip rank %u target %u: status %u.",
+				       DP_UUID(svc->ps_pool->sp_uuid), addr->pta_rank,
+				       tgts[j].ta_comp.co_id, tgts[j].ta_comp.co_status);
+				continue;
+			}
+
+			valid_addr.pta_rank = addr->pta_rank;
+			valid_addr.pta_target =
+			    (addr->pta_target == (uint32_t)-1) ? j : addr->pta_target;
+			rc = pool_target_addr_list_append(valid_list, &valid_addr);
+			if (rc != 0)
+				goto out;
+		}
+	}
+out:
+	ABT_rwlock_unlock(svc->ps_pool->sp_lock);
+	return rc;
+}
+
 static int
 pool_discard(crt_context_t ctx, struct pool_svc *svc, struct pool_target_addr_list *list,
 	     bool reint)
@@ -7972,6 +8059,7 @@ pool_discard(crt_context_t ctx, struct pool_svc *svc, struct pool_target_addr_li
 	struct pool_tgt_discard_out	*ptdi_out;
 	crt_rpc_t			*rpc;
 	d_rank_list_t			*rank_list = NULL;
+	struct pool_target_addr_list     valid_list = {0};
 	crt_opcode_t                     opc       = POOL_TGT_DISCARD;
 	int				i;
 	int				rc;
@@ -7984,23 +8072,32 @@ pool_discard(crt_context_t ctx, struct pool_svc *svc, struct pool_target_addr_li
 		D_ASSERTF(svc->ps_pool->sp_incr_reint == 0,
 			  "incremental reint should not get here\n");
 
-	rank_list = d_rank_list_alloc(list->pta_number);
+	rc = pool_discard_filter_tgts(svc, list, reint, &valid_list);
+	if (rc != 0)
+		D_GOTO(out, rc);
+
+	if (valid_list.pta_number == 0) {
+		D_INFO(DF_UUID " discard 0 valid target.", DP_UUID(svc->ps_pool->sp_uuid));
+		D_GOTO(out, rc = 0);
+	}
+
+	rank_list = d_rank_list_alloc(valid_list.pta_number);
 	if (rank_list == NULL)
 		D_GOTO(out, rc = -DER_NOMEM);
 
 	rank_list->rl_nr = 0;
 	/* remove the duplicate ranks from list, see reintegrate target case */
-	for (i = 0; i < list->pta_number; i++) {
-		if (daos_rank_in_rank_list(rank_list, list->pta_addrs[i].pta_rank))
+	for (i = 0; i < valid_list.pta_number; i++) {
+		if (daos_rank_in_rank_list(rank_list, valid_list.pta_addrs[i].pta_rank))
 			continue;
 
-		rank_list->rl_ranks[rank_list->rl_nr++] = list->pta_addrs[i].pta_rank;
-		D_DEBUG(DB_MD, DF_UUID": discard rank %u\n",
-			DP_UUID(svc->ps_pool->sp_uuid), list->pta_addrs[i].pta_rank);
+		rank_list->rl_ranks[rank_list->rl_nr++] = valid_list.pta_addrs[i].pta_rank;
+		D_DEBUG(DB_MD, DF_UUID ": discard rank %u\n", DP_UUID(svc->ps_pool->sp_uuid),
+			valid_list.pta_addrs[i].pta_rank);
 	}
 
 	if (rank_list->rl_nr == 0) {
-		D_DEBUG(DB_MD, DF_UUID" discard 0 rank.\n", DP_UUID(svc->ps_pool->sp_uuid));
+		D_INFO(DF_UUID " discard 0 rank.", DP_UUID(svc->ps_pool->sp_uuid));
 		D_GOTO(out, rc = 0);
 	}
 
@@ -8011,8 +8108,8 @@ pool_discard(crt_context_t ctx, struct pool_svc *svc, struct pool_target_addr_li
 		D_GOTO(out, rc);
 
 	ptdi_in = crt_req_get(rpc);
-	ptdi_in->ptdi_addrs.ca_arrays = list->pta_addrs;
-	ptdi_in->ptdi_addrs.ca_count = list->pta_number;
+	ptdi_in->ptdi_addrs.ca_arrays = valid_list.pta_addrs;
+	ptdi_in->ptdi_addrs.ca_count  = valid_list.pta_number;
 	uuid_copy(ptdi_in->ptdi_uuid, svc->ps_pool->sp_uuid);
 	rc = dss_rpc_send(rpc);
 
@@ -8028,6 +8125,7 @@ pool_discard(crt_context_t ctx, struct pool_svc *svc, struct pool_target_addr_li
 out:
 	if (rank_list)
 		d_rank_list_free(rank_list);
+	pool_target_addr_list_free(&valid_list);
 	return rc;
 }
 

src/pool/srv_target.c

@@ -2802,7 +2802,6 @@ ds_pool_tgt_discard_ult(void *data)
 {
 	struct ds_pool         *pool;
 	struct tgt_discard_arg *arg = data;
-	uint32_t                ex_status;
 	int                     discarding;
 	int                     rc;
 
@@ -2816,9 +2815,15 @@ ds_pool_tgt_discard_ult(void *data)
 		D_GOTO(free, rc = 0);
 	}
 
-	ex_status = PO_COMP_ST_UP | PO_COMP_ST_UPIN | PO_COMP_ST_DRAIN;
-	rc        = ds_pool_thread_collective(arg->pool_uuid, ex_status, pool_child_discard, arg,
-					      DSS_ULT_DEEP_STACK);
+	/*
+	 * arg->tgt_list has already been validated and filtered by the pool service
+	 * leader against the authoritative pool map (see pool_discard() in srv_pool.c),
+	 * so there is no need to further exclude targets based on this engine's own
+	 * (possibly stale) pool map here. pool_child_discard() only discards targets
+	 * that are present in arg->tgt_list.
+	 */
+	rc = ds_pool_thread_collective(arg->pool_uuid, 0 /* exclude_status */, pool_child_discard,
+				       arg, DSS_ULT_DEEP_STACK);
 
 	ABT_mutex_lock(pool->sp_mutex);
 	pool->sp_discard_status = rc;