From 74cb258973baf0d39a1565555f327e4496fd41f3 Mon Sep 17 00:00:00 2001 From: Xuezhao Liu Date: Fri, 10 Jul 2026 08:51:13 +0000 Subject: [PATCH] DAOS-19265 pool: refine pool_discard() target validation pool_discard() previously sent the POOL_TGT_DISCARD RPC to target engines using the caller-supplied pool_target_addr array as-is, without validating it against the pool map. On the target engine side, ds_pool_tgt_discard_ult() then relied on its own (possibly stale) local pool map, using ex_status = PO_COMP_ST_UP | PO_COMP_ST_UPIN | PO_COMP_ST_DRAIN passed to ds_pool_thread_collective(), to exclude targets from the discard. When the target engine did not yet have the latest pool map, this could lead to targets holding useful data being discarded, or targets needing a discard being skipped. Add pool_discard_filter_tgts() to validate/filter the input pool_target_addr array against the pool service leader's authoritative pool map before building the POOL_TGT_DISCARD RPC: - for REINT, only keep targets currently in PO_COMP_ST_DOWN or PO_COMP_ST_DOWNOUT. - for EXTEND, only keep targets currently in PO_COMP_ST_NEW, or targets not yet present in the pool map at all (brand new ranks/targets being added can't hold pool data yet, so they are always safe to discard). On the target engine side, ds_pool_tgt_discard_ult() no longer needs to exclude targets based on its local pool map status, since the RPC input list has already been validated and filtered by the leader. pool_child_discard() already restricts the discard to targets present in the RPC's target list. Signed-off-by: Xuezhao Liu --- src/pool/srv_pool.c | 116 ++++++++++++++++++++++++++++++++++++++---- src/pool/srv_target.c | 13 +++-- 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/src/pool/srv_pool.c b/src/pool/srv_pool.c index fad6ef2ea25..967f3739429 100644 --- a/src/pool/srv_pool.c +++ b/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; } diff --git a/src/pool/srv_target.c b/src/pool/srv_target.c index 740931088ba..179d3e0a558 100644 --- a/src/pool/srv_target.c +++ b/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;