Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-13812 container: fix destroy vs lookup #12757

Merged
merged 4 commits into from
Dec 21, 2023
Merged

DAOS-13812 container: fix destroy vs lookup #12757

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54159bd
DAOS-13812 container: fix destroy vs lookup
wangshilong Aug 2, 2023
a58ec79
address comments
wangshilong Dec 14, 2023
2bbc8a4
remove unneeded check
wangshilong Dec 18, 2023
9a8570d
Merge branch 'master' of github.com:daos-stack/daos into shilongw/DAO…
wangshilong Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions src/container/srv_target.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,13 +629,18 @@ cont_child_alloc_ref(void *co_uuid, unsigned int ksize, void *po_uuid,
rc = ABT_cond_create(&cont->sc_scrub_cond);
if (rc != ABT_SUCCESS) {
rc = dss_abterr2der(rc);
goto out_mutex;
goto out_resync_cond;
}
rc = ABT_cond_create(&cont->sc_rebuild_cond);
if (rc != ABT_SUCCESS) {
rc = dss_abterr2der(rc);
goto out_scrub_cond;
}

cont->sc_pool = ds_pool_child_lookup(po_uuid);
if (cont->sc_pool == NULL) {
rc = -DER_NO_HDL;
goto out_cond;
goto out_rebuild_cond;
}

rc = vos_cont_open(cont->sc_pool->spc_hdl, co_uuid, &cont->sc_hdl);
Expand All @@ -661,7 +666,11 @@ cont_child_alloc_ref(void *co_uuid, unsigned int ksize, void *po_uuid,

out_pool:
ds_pool_child_put(cont->sc_pool);
out_cond:
out_rebuild_cond:
ABT_cond_free(&cont->sc_rebuild_cond);
out_scrub_cond:
ABT_cond_free(&cont->sc_scrub_cond);
out_resync_cond:
ABT_cond_free(&cont->sc_dtx_resync_cond);
out_mutex:
ABT_mutex_free(&cont->sc_mutex);
Expand All @@ -688,6 +697,7 @@ cont_child_free_ref(struct daos_llink *llink)
D_FREE(cont->sc_snapshots);
ABT_cond_free(&cont->sc_dtx_resync_cond);
ABT_cond_free(&cont->sc_scrub_cond);
ABT_cond_free(&cont->sc_rebuild_cond);
ABT_mutex_free(&cont->sc_mutex);
D_FREE(cont);
}
Expand Down Expand Up @@ -742,6 +752,13 @@ ds_cont_child_cache_destroy(struct daos_lru_cache *cache)
daos_lru_cache_destroy(cache);
}

static void
cont_child_put(struct daos_lru_cache *cache, struct ds_cont_child *cont)
{
daos_lru_ref_release(cache, &cont->sc_list);
}


/*
* If create == false, then this is assumed to be a pure lookup. In this case,
* -DER_NONEXIST is returned if the ds_cont_child object does not exist.
Expand All @@ -754,6 +771,7 @@ cont_child_lookup(struct daos_lru_cache *cache, const uuid_t co_uuid,
struct daos_llink *llink;
uuid_t key[2]; /* HT key is cuuid+puuid */
int rc;
struct ds_cont_child *ds_cont;

uuid_copy(key[0], co_uuid);
uuid_copy(key[1], po_uuid);
Expand All @@ -772,16 +790,15 @@ cont_child_lookup(struct daos_lru_cache *cache, const uuid_t co_uuid,
return rc;
}

*cont = cont_child_obj(llink);
ds_cont = cont_child_obj(llink);
if (ds_cont->sc_stopping) {
cont_child_put(cache, ds_cont);
return -DER_SHUTDOWN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking "sc_stopping" in this internal lookup function could unintentional errors (for example, in cont_child_start() or cont_child_destroy_one()) , should we only check "sc_stopping" in the ds_cont_child_lookup()?

}
*cont = ds_cont;
wangshilong marked this conversation as resolved.
Show resolved Hide resolved
return 0;
}

static void
cont_child_put(struct daos_lru_cache *cache, struct ds_cont_child *cont)
{
daos_lru_ref_release(cache, &cont->sc_list);
}

static inline bool
cont_child_started(struct ds_cont_child *cont_child)
{
Expand All @@ -807,13 +824,13 @@ cont_child_stop(struct ds_cont_child *cont_child)
/* Some ds_cont_child will only created by ds_cont_child_lookup().
* never be started at all
*/
cont_child->sc_stopping = 1;
if (cont_child_started(cont_child)) {
D_DEBUG(DB_MD, DF_CONT"[%d]: Stopping container\n",
DP_CONT(cont_child->sc_pool->spc_uuid,
cont_child->sc_uuid),
dss_get_module_info()->dmi_tgt_id);

cont_child->sc_stopping = 1;
d_list_del_init(&cont_child->sc_link);

dtx_cont_deregister(cont_child);
Expand Down Expand Up @@ -1178,7 +1195,7 @@ cont_child_destroy_one(void *vin)
ABT_mutex_unlock(cont->sc_mutex);

/* Give chance to DTX reindex ULT for exit. */
if (unlikely(cont->sc_dtx_reindex))
while (unlikely(cont->sc_dtx_reindex))
ABT_thread_yield();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nasf-Fan , reindex is supposed to be very quick? Is this busy yield loop ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reindex may take long time. But when close the container, reindex ULT will exit soon when detects the "stopping" flag.


/* Make sure checksum scrubbing has stopped */
Expand All @@ -1189,6 +1206,12 @@ cont_child_destroy_one(void *vin)
}
ABT_mutex_unlock(cont->sc_mutex);

/* Make sure rebuild has stopped */
ABT_mutex_lock(cont->sc_mutex);
if (cont->sc_rebuilding)
ABT_cond_wait(cont->sc_rebuild_cond, cont->sc_mutex);
ABT_mutex_unlock(cont->sc_mutex);

retry_cnt++;
if (retry_cnt > 1) {
D_ERROR("container is still in-use: open %u, resync %s, reindex %s\n",
Expand Down
4 changes: 3 additions & 1 deletion src/include/daos_srv/container.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ struct ds_cont_child {
ABT_mutex sc_mutex;
ABT_cond sc_dtx_resync_cond;
ABT_cond sc_scrub_cond;
ABT_cond sc_rebuild_cond;
uint32_t sc_dtx_resyncing:1,
sc_dtx_reindex:1,
sc_dtx_reindex_abort:1,
Expand All @@ -71,7 +72,8 @@ struct ds_cont_child {
sc_stopping:1,
sc_vos_agg_active:1,
sc_ec_agg_active:1,
sc_scrubbing:1;
sc_scrubbing:1,
sc_rebuilding:1;
uint32_t sc_dtx_batched_gen;
/* Tracks the schedule request for aggregation ULT */
struct sched_request *sc_agg_req;
Expand Down
43 changes: 29 additions & 14 deletions src/rebuild/scan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,7 @@ struct rebuild_scan_arg {
int snapshot_cnt;
uint32_t yield_freq;
int32_t obj_yield_cnt;
struct ds_cont_child *cont_child;
};

/**
Expand Down Expand Up @@ -696,7 +697,7 @@ rebuild_obj_scan_cb(daos_handle_t ch, vos_iter_entry_t *ent,
int i;
int rc = 0;

if (rpt->rt_abort) {
if (rpt->rt_abort || arg->cont_child->sc_stopping) {
D_DEBUG(DB_REBUILD, "rebuild is aborted\n");
return 1;
}
Expand Down Expand Up @@ -839,35 +840,45 @@ rebuild_container_scan_cb(daos_handle_t ih, vos_iter_entry_t *entry,
}

rc = vos_cont_open(iter_param->ip_hdl, entry->ie_couuid, &coh);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not from this patch, but I don't see why we need to call vos_cont_open() here, won't following ds_cont_child_lookup() open vos container? @wangdi1 ?

if (rc == -DER_NONEXIST) {
D_DEBUG(DB_REBUILD, DF_UUID" already destroyed\n", DP_UUID(arg->co_uuid));
return 0;
}

if (rc != 0) {
D_ERROR("Open container "DF_UUID" failed: "DF_RC"\n",
DP_UUID(entry->ie_couuid), DP_RC(rc));
return rc;
}

rc = ds_cont_child_lookup(rpt->rt_pool_uuid, entry->ie_couuid, &cont_child);
if (rc == -DER_NONEXIST || rc == -DER_SHUTDOWN) {
D_DEBUG(DB_REBUILD, DF_UUID" already destroyed or destroying\n",
DP_UUID(arg->co_uuid));
rc = 0;
D_GOTO(close, rc);
}

if (rc != 0) {
D_ERROR("Container "DF_UUID", ds_cont_child_lookup failed: "DF_RC"\n",
DP_UUID(entry->ie_couuid), DP_RC(rc));
D_GOTO(close, rc);
}
cont_child->sc_rebuilding = 1;

rc = ds_cont_fetch_snaps(rpt->rt_pool->sp_iv_ns, entry->ie_couuid, NULL,
&snapshot_cnt);
if (rc) {
D_ERROR("Container "DF_UUID", ds_cont_fetch_snaps failed: "DF_RC"\n",
DP_UUID(entry->ie_couuid), DP_RC(rc));
vos_cont_close(coh);
return rc;
D_GOTO(close, rc);
}

rc = ds_cont_get_props(&arg->co_props, rpt->rt_pool->sp_uuid, entry->ie_couuid);
if (rc) {
D_ERROR("Container "DF_UUID", ds_cont_get_props failed: "DF_RC"\n",
DP_UUID(entry->ie_couuid), DP_RC(rc));
vos_cont_close(coh);
return rc;
}

rc = ds_cont_child_lookup(rpt->rt_pool_uuid, entry->ie_couuid, &cont_child);
if (rc != 0) {
D_ERROR("Container "DF_UUID", ds_cont_child_lookup failed: "DF_RC"\n",
DP_UUID(entry->ie_couuid), DP_RC(rc));
vos_cont_close(coh);
return rc;
D_GOTO(close, rc);
}

/* Wait for EC aggregation to finish. NB: migrate needs to wait for EC aggregation to finish */
Expand Down Expand Up @@ -899,6 +910,7 @@ rebuild_container_scan_cb(daos_handle_t ih, vos_iter_entry_t *entry,
param.ip_flags = VOS_IT_FOR_MIGRATION;
uuid_copy(arg->co_uuid, entry->ie_couuid);
arg->snapshot_cnt = snapshot_cnt;
arg->cont_child = cont_child;

/* If there is no snapshots, then rebuild does not need to migrate
* punched objects at all. Ideally, it should ignore any objects
Expand All @@ -914,8 +926,11 @@ rebuild_container_scan_cb(daos_handle_t ih, vos_iter_entry_t *entry,
close:
vos_cont_close(coh);

if (cont_child != NULL)
if (cont_child != NULL) {
cont_child->sc_rebuilding = 0;
ABT_cond_broadcast(cont_child->sc_rebuild_cond);
ds_cont_child_put(cont_child);
}

D_DEBUG(DB_REBUILD, DF_UUID"/"DF_UUID" iterate cont done: "DF_RC"\n",
DP_UUID(rpt->rt_pool_uuid), DP_UUID(entry->ie_couuid),
Expand Down