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-16486 object: return proper error on stale pool map #15064

Merged
merged 2 commits into from
Sep 6, 2024
Merged

DAOS-16486 object: return proper error on stale pool map #15064

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efbe469
DAOS-16486 object: return proper error on stale pool map
NiuYawei Sep 4, 2024
30fd195
Merge remote-tracking branch 'origin/master' into niu/DAOS-16486
NiuYawei Sep 5, 2024
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
7 changes: 7 additions & 0 deletions src/dtx/tests/srv_mock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ ds_pool_child_put(struct ds_pool_child *child)
assert_true(false);
}

struct ds_pool_child *
ds_pool_child_find(const uuid_t uuid)
{
assert_true(false);
return NULL;
}

struct ds_pool_child *
ds_pool_child_lookup(const uuid_t uuid)
{
Expand Down
2 changes: 2 additions & 0 deletions src/include/daos_srv/pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,8 @@ ds_pool_svc_ops_save(struct rdb_tx *tx, void *pool_svc, uuid_t pool_uuid, uuid_t
uint64_t cli_time, bool dup_op, int rc_in, struct ds_pool_svc_op_val *op_valp);

/* Find ds_pool_child in cache, hold one reference */
struct ds_pool_child *ds_pool_child_find(const uuid_t uuid);
/* Find ds_pool_child in STARTING or STARTED state, hold one reference */
struct ds_pool_child *ds_pool_child_lookup(const uuid_t uuid);
/* Put the reference held by ds_pool_child_lookup() */
void ds_pool_child_put(struct ds_pool_child *child);
Expand Down
31 changes: 30 additions & 1 deletion src/object/srv_obj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2161,8 +2161,37 @@ obj_ioc_begin_lite(uint32_t rpc_map_ver, uuid_t pool_uuid,
int rc;

rc = obj_ioc_init(pool_uuid, coh_uuid, cont_uuid, rpc, ioc);
if (rc)
if (rc) {
DL_ERROR(rc, "Failed to initialize object I/O context.");

/*
* Client with stale pool map may try to send RPC to a DOWN target, if the
* target was brought DOWN due to faulty NVMe device, the ds_pool_child could
* have been stopped on the NVMe faulty reaction, then above obj_io_init()
* will fail with -DER_NO_HDL.
*
* We'd ensure proper error code is returned for such case.
*/
poc = ds_pool_child_find(pool_uuid);
Copy link
Contributor

Choose a reason for hiding this comment

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

just confirm why not just use ds_pool_child_lookup()?
the difference with ds_pool_child_find() is ds_pool_child_find() will not return NULL for POOL_CHILD_STOPPING, so you want it continue to serve IO request when POOL_CHILD_STOPPING?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment explained, If the ds_pool_child is already stopped, it's state will be NEW or STOPPING, then ds_pool_child_lookup() will return NULL.

We'll return error instead of continue server I/O request, but we need to ensure -DER_STALE is returned when pool map is stale (instead of -DER_NO_HDL).

if (poc == NULL) {
D_ERROR("Failed to find pool:"DF_UUID"\n", DP_UUID(pool_uuid));
return rc;
}

if (rpc_map_ver < poc->spc_pool->sp_map_version) {
D_ERROR("Stale pool map version %u < %u from client.\n",
rpc_map_ver, poc->spc_pool->sp_map_version);

/* Restart the DTX if using stale pool map */
if (opc_get(rpc->cr_opc) == DAOS_OBJ_RPC_CPD)
rc = -DER_TX_RESTART;
else
rc = -DER_STALE;
}

ds_pool_child_put(poc);
return rc;
}

poc = ioc->ioc_coc->sc_pool;
D_ASSERT(poc != NULL);
Expand Down
15 changes: 15 additions & 0 deletions src/pool/srv_target.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,21 @@ pool_child_lookup_noref(const uuid_t uuid)
return NULL;
}

struct ds_pool_child *
ds_pool_child_find(const uuid_t uuid)
Copy link
Contributor

Choose a reason for hiding this comment

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

given the logic for find and lookup is mostly the same why not just add a bool opt to lookup to return all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would cause more code changes. (every caller of ds_pool_child_lookup() needs be changed).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a improper name, I'd rather call it ds_pool_child_lookup_force(), it's confusing to have both find() and lookup()

{
struct ds_pool_child *child;

child = pool_child_lookup_noref(uuid);
if (child == NULL) {
D_ERROR(DF_UUID": Pool child isn't found.\n", DP_UUID(uuid));
return child;
}

child->spc_ref++;
return child;
}

struct ds_pool_child *
ds_pool_child_lookup(const uuid_t uuid)
{
Expand Down
Loading