Repository navigation
DAOS-17322 ddb: DTX aggregation with ddb (Part 1) - #16745
Conversation
Cleanup of SCons file. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Miscellaneous fixes detected by clangd. Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Add the following new command to ddb: - dtx_stat: Print statistics on DTX entries Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
Ticket title is 'DTX aggregation with ddb' |
| Name: "dtx_stat", | ||
| Aliases: nil, | ||
| Help: "Stat on DTX entries", | ||
| LongHelp: "Print statistic on the DTX entries", |
There was a problem hiding this comment.
Can we print the DTX statistics information like that:
if container UUID is specified in the path, then for the container; otherwise, if only pool UUID is available, then for the DTX summary of the pool or every container's DTX statistics information in the pool.
There was a problem hiding this comment.
- Print stat for all containers.
There was a problem hiding this comment.
- Print stat for all containers.
Fixed with commit 2b72e15
| if (strftime(buf, buf_size, "%F %T", &date) == 0) | ||
| return -DER_INVAL; | ||
|
|
||
| buf_len = strnlen(buf, buf_size); |
There was a problem hiding this comment.
buflen = strftime();
if (buflen == 0)
return -DER_INVAL;
There was a problem hiding this comment.
- Remove useless strlen
There was a problem hiding this comment.
- Remove useless strlen
Fixed with commit 983d9d4
| if (!SUCCESS(rc)) | ||
| goto done; | ||
|
|
||
| if (!itp_has_cont(&itp)) { |
There was a problem hiding this comment.
It is better to support that: if only specify pool UUID, then iterate the containers in the pool and print each container's DTX statistics information. One benefit for such ability is that we can verify whether the count of the total DTX entries for pool can match the summary of all containers' or not.
There was a problem hiding this comment.
- Print stat for all containers.
| if (!SUCCESS(rc)) | ||
| goto done; | ||
|
|
||
| rc = vos_dtx_cmt_reindex(coh); |
There was a problem hiding this comment.
If we just want to obtain the committed DTX entries statistics information, it is unnecessary to reindex them. vos_dtx_cmt_reindex() is time consuming operation. Instead, we can iterate each vos_dtx_blob_df and summary the vos_dtx_blob_df::dbd_count. That is more efficient.
There was a problem hiding this comment.
- Replace
vos_dtx_cmt_reindex()with iterate onvos_dtx_blob_df
There was a problem hiding this comment.
- Replace
vos_dtx_cmt_reindex()with iterate onvos_dtx_blob_df
Fixed with commit 3ff2b3a
| int | ||
| ddb_date2epoch(const char *date, uint64_t *epoch) | ||
| { | ||
| struct tm date_tm; |
There was a problem hiding this comment.
struct tm date_tm = {0}; then no need memset();
There was a problem hiding this comment.
- Use static initialisation instead of explicit initialisation.
There was a problem hiding this comment.
- Use static initialisation instead of explicit initialisation.
Fixed with commit 983d9d4
| D_ALLOC_ARRAY(_buf, _buf_size); \ | ||
| assert_non_null(_buf); \ | ||
| regerror(_rc, &_preg, _buf, _buf_size); \ | ||
| if (_buf) \ |
There was a problem hiding this comment.
You have assert assert_non_null(_buf);
There was a problem hiding this comment.
- Remove useless test
There was a problem hiding this comment.
@Nasf-Fan , at the end this test is needed to manage gcc false positive warning.
[2025-09-11T18:30:29.037Z] In file included from src/utils/ddb/tests/ddb_commands_tests.c:21:
[2025-09-11T18:30:29.037Z] src/utils/ddb/tests/ddb_commands_tests.c: In function ‘dtx_stat_tests’:
[2025-09-11T18:30:29.037Z] src/utils/ddb/tests/ddb_cmocka.h:89:25: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
[2025-09-11T18:30:29.038Z] 89 | print_error("ERROR: invalid regex '%s': %s\n", regex, _buf); \
[2025-09-11T18:30:29.038Z] | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[2025-09-11T18:30:29.038Z] src/utils/ddb/tests/ddb_commands_tests.c:422:9: note: in expansion of macro ‘assert_regex_match’
[2025-09-11T18:30:29.038Z] 422 | assert_regex_match(
[2025-09-11T18:30:29.038Z] | ^~~~~~~~~~~~~~~~~~
[2025-09-11T18:30:29.038Z] src/utils/ddb/tests/ddb_cmocka.h:89:65: note: format string is defined here
[2025-09-11T18:30:29.038Z] 89 | print_error("ERROR: invalid regex '%s': %s\n", regex, _buf); \
More details could be found in the log of the PR build #5
This test was reintroduced with the commit 7c96b6a
| if (_buf) \ | ||
| print_error("ERROR: invalid regex '%s': %s\n", regex, _buf); \ | ||
| D_FREE(_buf); \ | ||
| regfree(&_preg); \ |
There was a problem hiding this comment.
If regcomp() return failure, then _preg maybe uninitialized?
There was a problem hiding this comment.
- fix invalid
regfree()
There was a problem hiding this comment.
- fix invalid
regfree()
Fixed with commit 77e3c21
kjacque
left a comment
There was a problem hiding this comment.
Only looked at the Go side in detail. LGTM.
…/daos-17322-part1
Integrate reviewers comments: - Remove useless test - Remove useless strlen - Use static initialisation instead of explicit initialisation. Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
Test stage Build RPM on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/2/execution/node/350/log |
|
Test stage Build on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/2/execution/node/418/log |
|
Test stage Build RPM on EL 8 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/2/execution/node/386/log |
|
Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/2/execution/node/385/log |
Integrate reviewers comments: - fix invalid regfree() Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
|
Test stage Build on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/3/execution/node/404/log |
|
Test stage Build RPM on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/3/execution/node/416/log |
|
Test stage Build RPM on EL 8 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/3/execution/node/347/log |
|
Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/3/execution/node/421/log |
|
Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/9/testReport/ |
|
Test stage Unit Test bdev with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/9/testReport/ |
…/daos-17322-part1 Features: aggregation
|
Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/10/testReport/ |
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/10/testReport/ |
|
Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/10/testReport/ |
|
Test stage Unit Test bdev with memcheck on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/10/testReport/ |
…/daos-17322-part1 Features: aggregation
| dbd = umem_off2ptr(umm, cont->vc_cont_df->cd_dtx_committed_head); | ||
| while (dbd != NULL) { | ||
| D_ASSERTF(dbd->dbd_magic == DTX_CMT_BLOB_MAGIC, | ||
| "Corrupted committed DTX blob (2) %x\n", dbd->dbd_magic); |
There was a problem hiding this comment.
Different from regular vos logic, ddb is usually to handle system corruption, then it is better not assertion the on-disk data, instead, we can report some error.
There was a problem hiding this comment.
- Change assert into error code.
There was a problem hiding this comment.
- Change assert into error code.
Fixed with commit 49be495
| ctx = args->ctx; | ||
|
|
||
| itp_set_cont(&itp, entry->ie_couuid, args->cont_seen); | ||
| ++args->cont_seen; |
There was a problem hiding this comment.
Better ++(args->cont_seen); to avoid confusion.
There was a problem hiding this comment.
- fix C coding standard
There was a problem hiding this comment.
- fix C coding standard
Fixed with commit 49be495
| param.ip_hdl = ctx->dc_poh; | ||
| param.ip_epr.epr_hi = DAOS_EPOCH_MAX; | ||
| do { | ||
| struct vos_iter_anchors anchors = {0}; |
There was a problem hiding this comment.
Move anchors outside the while() block; otherwise, you will reset the iteration repeatedly and fall into dead loop.
There was a problem hiding this comment.
Arghhhh, not yet used to manage this kind of iterator.
- Fix iterator management.
There was a problem hiding this comment.
Arghhhh, not yet used to manage this kind of iterator.
- Fix iterator management.
Fixed with commit 49be495
| Dbds[i].dbd_cap = DBD_BLOB_DF_CAP; | ||
| DbdsOff[i] = umem_ptr2off(&Pool.vp_umm, &Dbds[i]); | ||
| Dbds[i].dbd_next = | ||
| (i == DBD_COUNT - 1) ? UMOFF_NULL : umem_ptr2off(&Pool.vp_umm, &Dbds[i + 1]); |
There was a problem hiding this comment.
The real committed DTX table is something like that: for each vos_dtx_blob_df, the vos_dtx_cmt_ent_df array will be contiguous with its vos_dtx_blob_df header. But in your test_setup, all the headers of vos_dtx_blob_df are together, that does not match the real case.
Since the test logic does not use any committed DTX entry, then maybe you can only allocate array for vos_dtx_cmt_ent_df headers without vos_dtx_cmt_ent_df array. Otherwise, the position for &Dbds[i + 1]) may belong to some vos_dtx_cmt_ent_df.
BTW, in DAOS engine, the C logic variable name is usually lowercase. I would suggest the same for the test logic. Thanks!
On the other hand, there is not strict requirement for adding unit test for the new VOS API vos_dtx_get_cmt_cnt that is quite simple and only used for ddb.
There was a problem hiding this comment.
BTW, in DAOS engine, the C logic variable name is usually lowercase. I would suggest the same for the test logic. Thanks!
I have to confess that I have used some existing test file as example and do not take attention of the case variable name issue.
- Fix coding standard issue
There was a problem hiding this comment.
The real committed DTX table is something like that: for each vos_dtx_blob_df, the vos_dtx_cmt_ent_df array will be contiguous with its vos_dtx_blob_df header. But in your test_setup, all the headers of vos_dtx_blob_df are together, that does not match the real case.
Since the test logic does not use any committed DTX entry, then maybe you can only allocate array for vos_dtx_cmt_ent_df headers without vos_dtx_cmt_ent_df array. Otherwise, the position for &Dbds[i + 1]) may belong to some vos_dtx_cmt_ent_df.
- Fix DTX blob offset
There was a problem hiding this comment.
On the other hand, there is not strict requirement for adding unit test for the new VOS API
vos_dtx_get_cmt_cntthat is quite simple and only used forddb.
I agree that this test is over kill considering the complexity of the tested function. From my side, it allowed me to better understand this part of the DAOS code.
I have no issue to remove it if you think that it could make some confusion. If yes, before removing it, could it be possible to push the fix of the invalid DTX blob offset for being sure that I properly understood the issue.
There was a problem hiding this comment.
BTW, in DAOS engine, the C logic variable name is usually lowercase. I would suggest the same for the test logic. Thanks!
I have to confess that I have used some existing test file as example and do not take attention of the case variable name issue.
- Fix coding standard issue
Fixed with commit 49be495
There was a problem hiding this comment.
The real committed DTX table is something like that: for each vos_dtx_blob_df, the vos_dtx_cmt_ent_df array will be contiguous with its vos_dtx_blob_df header. But in your test_setup, all the headers of vos_dtx_blob_df are together, that does not match the real case.
Since the test logic does not use any committed DTX entry, then maybe you can only allocate array for vos_dtx_cmt_ent_df headers without vos_dtx_cmt_ent_df array. Otherwise, the position for &Dbds[i + 1]) may belong to some vos_dtx_cmt_ent_df.
- Fix DTX blob offset
Fixed with commit 49be495
| if (!SUCCESS(rc)) | ||
| goto done; | ||
|
|
||
| cnt_tmp = vos_dtx_get_cmt_cnt(coh); |
There was a problem hiding this comment.
I would suggest vos_dtx_get_cmt_cnt() can return failure if hit corruption instead of triggering assertion.
There was a problem hiding this comment.
- Change assert into error code.
There was a problem hiding this comment.
- Change assert into error code.
Fixed with commit 49be495
…/daos-17322-part1
Integrate reviewers comments: - Change assert into error code. - Fix C coding standard. - Fix iterator management. - Fix coding standard issue - Fix DTX blob offset Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
| assert_true(Dbds[i].dbd_count <= DBD_BLOB_DF_CAP); | ||
| struct vos_dtx_blob_df *dbd; | ||
|
|
||
| dbd = (struct vos_dtx_blob_df *)((uint64_t)(&mock_dbds[0]) + i * CELL_SIZE); |
There was a problem hiding this comment.
Is it correct? (uint64_t)(&mock_dbds[0]) seems to be (void *)(&mock_dbds[0]). Honestly, we can simplify the logic to initialize each mock_dbds[i] independently, something like that:
for (i = 0; i < DBD_COUNT; i++) {
D_ALLOC(mock_dbds[i], CELL_SIZE);
if (mock_dbds[i] == NULL)
return -DER_NOMEM;
...
mock_dbds_off[i] = xxx;
if (i > 0) {
mock_dbds[i].dbd_prev = mock_dbds_off[i - 1];
mock_dbds[i - 1].dbd_next = mock_dbds_off[i];
} else {
mock_dbds[i].dbd_prev = UMOFF_NULL;
}
}
mock_dbds[0].dbd_next = mock_dbds_off[1];
Correspondingly, destroy them one by one in test_teardown:
for (i = 0; i < DBD_COUNT; i--) {
D_FREE(mock_dbds[i]);
}
Then the logic is more easy to be understood and avoid bugs during calculate next and prev.
There was a problem hiding this comment.
Perfectly agree, using one allocation is over complicated for no good reason (i.e. just avoid to have two loop: one for memory allocation and one for initialisation).
Going to fix this as you suggested.
- Use multiple allocation.
There was a problem hiding this comment.
Perfectly agree, using one allocation is over complicated for no good reason (i.e. just avoid to have two loop: one for memory allocation and one for initialisation). Going to fix this as you suggested.
- Use multiple allocation.
Fixed with commit 9d81546
|
|
||
| return cnt; | ||
| *cnt = tmp; | ||
| rc = -DER_SUCCESS; |
There was a problem hiding this comment.
[suggestion] I am wondering why return -DER_SUCCESS instead of 0? That seems confused although we usually define DER_SUCCESS as 0. You have already described \return 0 on success, error otherwise in the header. So please just return 0 and adjust related test cases. Thanks!
There was a problem hiding this comment.
- Replace
-DER_SUCCESSwith0
There was a problem hiding this comment.
- Replace
-DER_SUCCESSwith0
Fixed with commit 9d81546
…/daos-17322-part1
Integrate reviewers comments: - Replace -DER_SUCCESS with 0 - Use multiple allocation Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
Fix clang format issue Features: aggregation Signed-off-by: Cedric Koch-Hofer <cedric.koch-hofer@hpe.com>
| for (i = 0; i < DBD_COUNT; i++) { | ||
| struct vos_dtx_blob_df *dbd; | ||
| D_ALLOC(mock_dbds[i], CELL_SIZE); | ||
| assert_non_null(mock_dbds[i]); |
There was a problem hiding this comment.
mock_dbds_off[i] = umem_ptr2off(&mock_pool.vp_umm, mock_dbds[i]);
Add above line in this for section. Then in subsequent for section, we can directly use them, for example:
if (i == DBD_COUNT - 1)
mock_dbds[i]->dbd_next = UMOFF_NULL;
else
mock_dbds[i]->dbd_next = mock_dbds_off[i + 1];
if (i == 0)
mock_dbds[i]->dbd_prev = UMOFF_NULL;
else
mock_dbds[i]->dbd_prev = mock_dbds_off[i - 1];
Anyway, just some code optimization, not fatal. If you have other chance to refresh the patch, you can consider.
There was a problem hiding this comment.
If I have no mandatory change requested, I will integrate your remarks in the following up PR introducing the ddb aggregation command.
There was a problem hiding this comment.
- Simplify linked list management
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/14/testReport/ |
kjacque
left a comment
There was a problem hiding this comment.
Go changes LGTM. Didn't look closely at the rest.
…/daos-17322-part1 Features: aggregation
|
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16745/15/testReport/ |
|
Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16745/15/execution/node/1300/log |
…/daos-17322-part1 Features: aggregation
|
@daos-stack/daos-gatekeeper please could you lend this PR. |
Description
First PR to add DTX entries aggregation sub commands to DDB.
This preliminary PR introduce the following modifications:
src/utils/ddb/tests/SConscriptdtx_statallowing to print statistics on DTX entries.This PR will be followed by the PR #16746 introducing a new DDB sub command
dtx_aggrallowing to aggregate DTX entries with DDB.Steps for the author:
After all prior steps are complete: