DAOS-14903 container: refine task process in pmap_refresh_cb (#13491)

[liuxuezhao]

should register completion callback before task reinit, or the complete cb possibly cannot be triggered.

and a few other backports:
6d4e549 - DAOS-14788 pool: Fix some reinit usages (#13518)
91b93c8 - DAOS-13252 tests: set svcn for multiple_failure test (#13619)
d30e842 - DAOS-14903 object: fix bug in peer status check (#13585)

Required-githooks: true

Before requesting gatekeeper:

• Two review approvals and any prior change requests have been resolved.
• Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
• Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
• Commit messages follows the guidelines outlined here.
• Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

• You are the appropriate gatekeeper to be landing the patch.
• The PR has 2 reviews by people familiar with the code, including appropriate watchers.
• Githooks were used. If not, request that user install them and check copyright dates.
• Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
• All builds have passed. Check non-required builds for any new compiler warnings.
• Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
• If applicable, the PR has addressed any potential version compatibility issues.
• Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
• Extra checks if forced landing is requested
  • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
  • No new NLT or valgrind warnings. Check the classic view.
  • Quick-build or Quick-functional is not used.
• Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

[github-actions]

Bug-tracker data:
Ticket title is 'fix bug in EC aggregation's peer update'
Status is 'Awaiting backport'
Job should run at elevated priority (1)
https://daosio.atlassian.net/browse/DAOS-14903

[daosbuild1]

LGTM. No errors found by checkpatch.

[daltonbohning]

This should at least run the modified functional test and possibly some other features.

Features: EcodOnlineMultFail

[daosbuild1]

LGTM. No errors found by checkpatch.

tags used

[daltonbohning]

ftest LGTM

[Nasf-Fan]

Can we hold the reference during the RPC process on server side, and then ask map_refresh_cb() to release the reference? Then map_refresh_cb() do not need to get reference again. Otherwise, if it is possible that the pool may be freed before map_refresh_cb() triggered, then dc_pool_get() in map_refresh_cb() may access invalid memory?

[liuxuezhao]

leave it to @liw reply as this part is backport from one of his PR, thx.

[liw]

@Nasf-Fan, arg->mra_pool is the reference that prevents the dc_pool from being freed before map_refresh_cb accesses it. The extra references taken in this PR are for the tricky reinit cases, where anther thread could execute the reinitialized task and (in theory) free the arg->mra_pool reference after the reinit call but before the function that calls reinit returns.

[Nasf-Fan]

Consider the following scenario: map_refresh() hold the reference at the beginning of the function, then registers map_refresh_cb for the task. Before RPC sending, it puts the reference on the pool. At that time, map_refresh_cb is not called yet. It is possible that someone free the pool before map_refresh_cb() being triggered, right? if yes, then calling dc_pool_get(pool) in map_refresh_cb() will access invalid memory?

[liuxuezhao]

Consider the following scenario: map_refresh() hold the reference at the beginning of the function, then registers map_refresh_cb for the task. Before RPC sending, it puts the reference on the pool. At that time, map_refresh_cb is not called yet. It is possible that someone free the pool before map_refresh_cb() being triggered, right? if yes, then calling dc_pool_get(pool) in map_refresh_cb() will access invalid memory?

as my understanding, before map_refresh() called, in dc_pool_create_map_refresh_task() it take one extra reference "dc_pool_get(pool); a->mra_pool = pool;", and that reference will be released at map_refresh_cb()'s "!reinit" case. that would aovid the case you worried? @Nasf-Fan

[Nasf-Fan]

Consider the following scenario: map_refresh() hold the reference at the beginning of the function, then registers map_refresh_cb for the task. Before RPC sending, it puts the reference on the pool. At that time, map_refresh_cb is not called yet. It is possible that someone free the pool before map_refresh_cb() being triggered, right? if yes, then calling dc_pool_get(pool) in map_refresh_cb() will access invalid memory?

as my understanding, before map_refresh() called, in dc_pool_create_map_refresh_task() it take one extra reference "dc_pool_get(pool); a->mra_pool = pool;", and that reference will be released at map_refresh_cb()'s "!reinit" case. that would aovid the case you worried? @Nasf-Fan

Thanks for the explain. I am wondering whether map_refresh_cb() can properly distinguish "reinit" or not, for example, if someone triggered reinit, then as shown following:

tse_task_reinit_with_delay(tse_task_t *task, uint64_t delay)
{               
...
        task->dt_result = 0;

        /** Move back to init list */
        if (delay == 0) {
                dtp->dtp_wakeup_time = 0;
                d_list_move_tail(&dtp->dtp_list, &dsp->dsp_init_list);
        } else {
                dtp->dtp_wakeup_time = daos_getutime() + delay;
                d_list_del_init(&dtp->dtp_list);
                tse_task_insert_sleeping(dtp, dsp);
        }

At that time, dt_result is reset as zero, then whether set "reinit" flag depends on pool_tgt_query_map_out::po_rc, but related (re-sent) RPC may be not handled by server, then can be zero also?

The issue is not directly related with the back port patch. But may be worth for us to consider more.

[liw]

At that time, dt_result is reset as zero, then whether set "reinit" flag depends on pool_tgt_query_map_out::po_rc, but related (re-sent) RPC may be not handled by server, then can be zero also?

@Nasf-Fan, the reinit variable in map_refresh_cb means "shall reinitialize the current task", not "the current task has been reinitialized before". Does that answer your concern? If not, what is your concern (sorry, I'm not confident that I get what you mean)?

[Nasf-Fan]

At that time, dt_result is reset as zero, then whether set "reinit" flag depends on pool_tgt_query_map_out::po_rc, but related (re-sent) RPC may be not handled by server, then can be zero also?

@Nasf-Fan, the reinit variable in map_refresh_cb means "shall reinitialize the current task", not "the current task has been reinitialized before". Does that answer your concern? If not, what is your concern (sorry, I'm not confident that I get what you mean)?

I see, thanks