DAOS-17106 swim: Never change self_id #15929
Conversation
In the current implementation of DAOS, when it fails to select a dping target, it will set its self_id to SWIM_ID_INVALID and from now on, the node is not fully functional because only swim_updates_short() will be invoked to respond to PING and IPING request. Once the node enters into this state, it can not be restored. This PR fixes the problem by setting the self_id to SWIM_ID_INVALID if it won't be able to find a dping target. Change-Id: I4a2dad9aeb0571f938a84a97a9e29868a33b01a1 Signed-off-by: Jinshan Xiong <[email protected]>
src/cart/swim/swim.c
Outdated
| @@ -924,8 +924,7 @@ swim_progress(struct swim_context *ctx, int64_t timeout_us) | |||
| ctx->sc_next_event = ctx->sc_deadline; | |||
| ctx_state = SCS_PINGED; | |||
| } else { | |||
| if (ctx->sc_next_tick_time < ctx->sc_next_event) | |||
| ctx->sc_next_event = ctx->sc_next_tick_time; | |||
| ctx->sc_next_event = MIN(ctx->sc_next_event, ctx->sc_next_tick_time); | |||
There was a problem hiding this comment.
@liw I don't really understand the usage of sc_next_tick_time, can you please explain it for me?
|
Ticket title is 'SWIM enters into a weird state and never restores' |
| if (ctx->sc_next_tick_time < ctx->sc_next_event) | ||
| ctx->sc_next_event = ctx->sc_next_tick_time; | ||
| ctx->sc_next_event = | ||
| MIN(ctx->sc_next_event, ctx->sc_next_tick_time); |
There was a problem hiding this comment.
[Suggestion] Shall we refrain from making not-strictly-necessary changes, please? I find my brain converts this MIN into the original if structure before it understands what it is happening---this surprises me.
There was a problem hiding this comment.
This is just better code, IMO.
For the original code, you have to look into what is being compared and what is being assigned to understand what it is doing. I found it sometime difficult to understand.
Therefore, I take this change as an improvement but not 'not-strictly-necessary'; because it will help the other folks understand the code easier.
| } | ||
| break; | ||
| case SCS_SELECT: | ||
| ctx->sc_target = ctx->sc_ops->get_dping_target(ctx); | ||
| if (ctx->sc_target == SWIM_ID_INVALID) { | ||
| swim_ctx_unlock(ctx); | ||
| D_GOTO(out, rc = -DER_SHUTDOWN); |
There was a problem hiding this comment.
[Suggestion] Since this is the only place generating DER_SHUTDOWN in src/cart, I wonder if removing it allows us to remove some of the code that handles this DER_SHUTDOWN in src/cart?
There was a problem hiding this comment.
that's a good idea. Will fix it.
| } | ||
| break; | ||
| case SCS_SELECT: | ||
| ctx->sc_target = ctx->sc_ops->get_dping_target(ctx); | ||
| if (ctx->sc_target == SWIM_ID_INVALID) { | ||
| swim_ctx_unlock(ctx); | ||
| D_GOTO(out, rc = -DER_SHUTDOWN); | ||
| ctx->sc_next_event = now + swim_period_get(); |
There was a problem hiding this comment.
Although I'm not totally clear about how sc_next_event and sc_next_tick_time work, most other places in swim carefully prevent pushing sc_next_event into a future timestamp. This makes me wonder:
- If we go to
outin this case, there's a risk that next timeswim_progressis called, we might be (hopefully) a bit passsc_next_event(which might not be a big deal?). - If we leave
sc_next_eventas it is and continue the loop, will there be any problem?
There was a problem hiding this comment.
sc_next_event is clearly defined which is the next time the swim should be scheduled. sc_next_tick_time seems to be just a copy of it but it's not well maintained or just a variable with bad name.
I don't see an issue to change sc_next_event though. And if we don't break, it will just have loop until the previous deadline comes. I tend to think the current fix is fine regarding this issue.
In the current implementation of DAOS, when it fails to select a dping target, it will set its self_id to SWIM_ID_INVALID and from now on, the node is not fully functional because only swim_updates_short() will be invoked to respond to PING and IPING request. Once the node enters into this state, it can not be restored.
This PR fixes the problem by not setting the self_id to SWIM_ID_INVALID if it won't be able to find a dping target. It's simply not necessary.
Change-Id: I4a2dad9aeb0571f938a84a97a9e29868a33b01a1
Before requesting gatekeeper:
Features:(orTest-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.Gatekeeper: