DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

liw · 2023-07-21T07:36:35Z

pool_glance should only free map_buf if ds_pool_svc_load returns zero. The segfault might be triggered because ds_pool_svc_load returned DER_UNINIT; it is unknown that what actually happened. This patch fixes the D_FREE logic.

ds_pool_svc_load does not need to free map_buf in any case. It is likely a merge error. This patch removes the unnecessary D_FREE in ds_pool_svc_load.

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:

pool_glance should only free map_buf if ds_pool_svc_load returns zero. The segfault might be triggered because ds_pool_svc_load returned DER_UNINIT; it is unknown that what actually happened. This patch fixes the D_FREE logic. ds_pool_svc_load does not need to free map_buf in any case. It is likely a merge error. This patch removes the unnecessary D_FREE in ds_pool_svc_load. Signed-off-by: Li Wei <[email protected]> Required-githooks: true

github-actions · 2023-07-21T07:36:53Z

Bug-tracker data:
Ticket title is 'DRAM corruption inside pool_glance'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-13996

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-21T13:54:55Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12683/1/execution/node/1259/log

DAOS-13931

daosbuild1 · 2023-07-21T14:05:53Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12683/1/execution/node/1286/log

DAOS-13998

liw · 2023-07-24T04:26:15Z

src/pool/srv_pool.c

@@ -1540,8 +1540,6 @@ ds_pool_svc_load(struct rdb_tx *tx, uuid_t uuid, rdb_path_t *root, uint32_t *glo
 	*global_version_out = global_version;
 	*map_buf_out = map_buf;
 	*map_version_out = map_version;
-	if (rc != 0)
-		D_FREE(map_buf);


[Note] This is probably caused by previous merged with master.

or set "*map_version_out = NULL" at above L1519 (rc != 0) case, seems in that case the map_buf ptr in stack possibly not init.

@liuxuezhao, it is intentional that ds_pool_svc_load changes *map_version_out only when returning zero. Callers shall only expect an output in *map_version_out if ds_pool_svc_load returns zero.

Nasf-Fan · 2023-07-24T04:36:58Z

src/pool/srv_pool_check.c

-	if (rc == DER_UNINIT) {
+	if (rc == 0) {
+		D_FREE(map_buf);
+	} else if (rc == DER_UNINIT) {


Just want to know why ds_pool_svc_load() return positive number for empty pool map instead of "-DER_UNINIT"?

Because that is a important, non-error scenario that will almost 100% happen during the creation of a PS, and I was afraid that functions called by ds_pool_svc_load may return -DER_UNINIT for other unrelated scenarios. Hence, I chose a positive return value many years ago.

If I were to make the same choice again today, I might prefer defining an enum with a positive value.

Because that is a important, non-error scenario that will almost 100% happen during the creation of a PS, and I was afraid that functions called by ds_pool_svc_load may return -DER_UNINIT for other unrelated scenarios. Hence, I chose a positive return value many years ago.

If I were to make the same choice again today, I might prefer defining an enum with a positive value.

I think you can do that in master independently.

liuxuezhao · 2023-07-24T05:59:28Z

src/pool/srv_pool.c

@@ -1540,8 +1540,6 @@ ds_pool_svc_load(struct rdb_tx *tx, uuid_t uuid, rdb_path_t *root, uint32_t *glo
 	*global_version_out = global_version;
 	*map_buf_out = map_buf;
 	*map_version_out = map_version;
-	if (rc != 0)
-		D_FREE(map_buf);


or set "*map_version_out = NULL" at above L1519 (rc != 0) case, seems in that case the map_buf ptr in stack possibly not init.

liw · 2023-07-24T06:10:58Z

Thanks to the reviewers for the quick responses!

Required-githooks: true

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-25T12:47:15Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12683/2/testReport/

daosbuild1 · 2023-07-25T13:32:14Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12683/2/execution/node/1283/log

Nasf-Fan · 2023-07-28T02:46:06Z

@liw , what is the next step for this patch? Do you need more reviewer or any further investigation? The patch will be helpful for CR tests on CI. Thanks!

liw · 2023-07-28T02:48:46Z

@Nasf-Fan, this PR is hitting CR and non-CR test failures. If those test failures don't go away, how does this PR proceed?

Nasf-Fan · 2023-07-28T04:09:15Z

@Nasf-Fan, this PR is hitting CR and non-CR test failures. If those test failures don't go away, how does this PR proceed?

I did not want to push the landing with break regular land process.
I hit DAOS-13996 corruption many times during both CR CI test and manual test, so just to make sure the gatekeeper can be aware of the land requirement in time since quite busy recently.

liw · 2023-07-31T01:51:02Z

The rebuild_simple failure is unrelated to this PR; the pool_list_consolidation failure involves multiple problems that are likely unrelated to this PR. We need to investigate the latter.

liw · 2023-07-31T02:28:58Z

Discussed with Fan Yong the situation. We think the pool_list_consolidation is first a test issue (a ticket will be created in a minute) and unrelated to this PR.

daosbuild1 reviewed Jul 21, 2023

View reviewed changes

liw marked this pull request as ready for review July 24, 2023 04:22

liw requested review from liuxuezhao and Nasf-Fan July 24, 2023 04:25

liw commented Jul 24, 2023

View reviewed changes

Nasf-Fan reviewed Jul 24, 2023

View reviewed changes

Nasf-Fan self-requested a review July 24, 2023 04:37

Nasf-Fan approved these changes Jul 24, 2023

View reviewed changes

liuxuezhao approved these changes Jul 24, 2023

View reviewed changes

Merge branch 'feature/cat_recovery' into liw/fix-map_buf-free

3aea445

Required-githooks: true

daosbuild1 reviewed Jul 25, 2023

View reviewed changes

liw requested a review from a team July 31, 2023 02:29

liw added forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. CR Catastrophic Recovery Feature labels Jul 31, 2023

gnailzenh approved these changes Jul 31, 2023

View reviewed changes

gnailzenh merged commit 02a43c1 into feature/cat_recovery Jul 31, 2023

gnailzenh deleted the liw/fix-map_buf-free branch July 31, 2023 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

liw commented Jul 21, 2023

github-actions bot commented Jul 21, 2023 •

edited

Loading

daosbuild1 left a comment

daosbuild1 commented Jul 21, 2023 •

edited by liw

Loading

daosbuild1 commented Jul 21, 2023 •

edited by liw

Loading

liw Jul 24, 2023

liuxuezhao Jul 24, 2023

liw Jul 24, 2023

Nasf-Fan Jul 24, 2023

liw Jul 24, 2023

Nasf-Fan Jul 24, 2023

liuxuezhao Jul 24, 2023

liw commented Jul 24, 2023

daosbuild1 left a comment

daosbuild1 commented Jul 25, 2023

daosbuild1 commented Jul 25, 2023

Nasf-Fan commented Jul 28, 2023

liw commented Jul 28, 2023

Nasf-Fan commented Jul 28, 2023

liw commented Jul 31, 2023

liw commented Jul 31, 2023

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

DAOS-13996 pool: Fix invalid D_FREE in pool_glance #12683

Conversation

liw commented Jul 21, 2023

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jul 21, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jul 21, 2023 • edited by liw Loading

daosbuild1 commented Jul 21, 2023 • edited by liw Loading

liw Jul 24, 2023

Choose a reason for hiding this comment

liuxuezhao Jul 24, 2023

Choose a reason for hiding this comment

liw Jul 24, 2023

Choose a reason for hiding this comment

Nasf-Fan Jul 24, 2023

Choose a reason for hiding this comment

liw Jul 24, 2023

Choose a reason for hiding this comment

Nasf-Fan Jul 24, 2023

Choose a reason for hiding this comment

liuxuezhao Jul 24, 2023

Choose a reason for hiding this comment

liw commented Jul 24, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jul 25, 2023

daosbuild1 commented Jul 25, 2023

Nasf-Fan commented Jul 28, 2023

liw commented Jul 28, 2023

Nasf-Fan commented Jul 28, 2023

liw commented Jul 31, 2023

liw commented Jul 31, 2023

github-actions bot commented Jul 21, 2023 •

edited

Loading

daosbuild1 commented Jul 21, 2023 •

edited by liw

Loading

daosbuild1 commented Jul 21, 2023 •

edited by liw

Loading