DAOS-19451 cont: enable checksum by default on non-v0 pools - #18885
Draft
ryon-jensen wants to merge 6 commits into
Draft
DAOS-19451 cont: enable checksum by default on non-v0 pools#18885ryon-jensen wants to merge 6 commits into
ryon-jensen wants to merge 6 commits into
Conversation
DAOS-17946 changed the default container properties to crc32 checksum and server-side checksum verification, but only updated the v0 default property table in src/container/srv_layout.c. cont_prop_entries_default, which is the table used for every pool with a global version >= 1, was left untouched, so containers created on any modern pool still defaulted to checksum off. Apply the same defaults to cont_prop_entries_default: crc32 checksum, server-side verification on and RF3. Note that the container redundancy factor is always inherited from the pool when it is not explicitly specified, so the RF3 entry is only there to keep the two tables consistent. The regression went unnoticed because the test harnesses hid it: daos_test forced checksum off on every container it created and the ftest container utility hardcoded "cksum:off,srv_cksum:off", both added by DAOS-17946. Drop those overrides and update the co_properties assertions so the defaults are actually exercised. Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
Collaborator
|
Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18885/1/testReport/ |
|
Ticket title is 'container checksum is off by default' |
cont_open_complete() adds the container to the pool container list before calling dc_cont_props_init(), but its error path returns without unlinking it. The last reference is then dropped on the way out and dc_cont_free() trips D_ASSERT(d_list_empty(&dc->dc_po_list)), aborting the client. The path was unreachable until now: with the checksum default off, dc_cont_props_init() returned 0 without allocating anything and could not fail. Now that containers default to crc32 it calls daos_csummer_init_with_type(), which can fail with -DER_NOMEM, and NLT fault injection hits it in dnt_fi_cont-create. Unlink the container before dropping the pool list lock. Note that dc_cont_global2local() already initializes the properties before adding the container to the list, so it is not affected. Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
crt_proc_struct_dcs_iod_csums_adv() allocates ic_data and then decodes each dcs_csum_info into it, with proc_struct_dcs_csum_info_adv() allocating cs_csum per entry. If decoding an entry fails partway through the loop, or if the trailing ic_akey decode fails, only the ic_data array was released, leaking every cs_csum buffer decoded so far. Add iod_csums_free_decoded() and use it on both decode error paths so the per-entry checksum buffers are freed along with the array. This path was previously unreachable in practice because checksums were off by default, so NLT fault injection never exercised it. Enabling checksums by default exposes it as a 'memory not freed' finding. Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
Collaborator
|
Test stage Functional Cluster Box Medium 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-18885/2/execution/node/1522/log |
Collaborator
|
Test stage Functional Cluster Box Medium 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-18885/3/execution/node/1525/log |
Collaborator
|
Test stage Functional Cluster Box Medium 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-18885/4/execution/node/1524/log |
ds_cpd_handle_one() keeps per-sub-request arrays of pointers to iods, offsets, skips and checksums. When dcde_write_cnt exceeds LOCAL_STACK_NUM those arrays are heap allocated, but unlike the on-stack case the individual element pointers are never pointed at backing storage, so they stay NULL. obj_get_iods_offs() then dereferenced *p_csums unconditionally when the request carried checksums, segfaulting the engine on any EC transaction with more than LOCAL_STACK_NUM writes. A NULL slot is in fact fine: obj_get_iods_offs_by_oid() allocates the csums array and fills in ic_data per entry when *iods is NULL, so only skip seeding ic_data in that case. That in turn makes pcsums[i] heap allocated on this path, which exposed a typo in the corresponding cleanup loop: it incremented i instead of j, corrupting the enclosing loop index and walking off the csum array. Free the per-entry checksums with the correct index. Both defects were unreachable while container checksums defaulted to off. Features: checksum Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
…EC singv Three defects in the client-side fetch checksum verification path, all unreachable while container checksums defaulted to off. dc_rw_cb_csum_verify() asserted maps_nr == iod_nr before the early return that skips echo object classes. The server short-circuits echo RPCs in obj_rw_handler() before obj_fetch_create_maps() runs, so no iod maps come back and the assert aborted the client. Move the assert below the echo check, which is where it was always meant to apply. iod_sgl_copy() asserted that the shard offset lies strictly inside the first iov of the copied sgl. A zero length leading iov is legal - a fetch that only queries record sizes leaves iov_len at 0 - and with a zero shard offset daos_sgl_processor() consumes nothing, so the assert compared 0 < 0 and aborted. Only trim the leading iov when there is an offset to trim; the trim was a no-op otherwise, apart from needlessly clobbering iov_buf_len. dc_rw_cb_singv_lo_get() derived the EC single-value cell size from the caller supplied iod_size. A fetch may deliberately over-estimate it, in which case the computed layout no longer matched the one used when the record was written and verification failed with -DER_CSUM. Use the record size replied by the server, which is what the rest of the verify path already uses via shard_iod.iod_size. Features: checksum Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
ryon-jensen
force-pushed
the
ryon-jensen/DAOS-19451
branch
from
August 21, 2026 01:45
d2e8bf5 to
e842977
Compare
Collaborator
|
Test stage Functional Cluster Box Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18885/5/testReport/ |
daos_sgl_get_bytes() asserted that idx->iov_offset was strictly less than the current iov length. A zero-length iov is legal - the basic_byte_array test deliberately builds an sgl whose leading iov is set with d_iov_set(&sg_iov[0], NULL, 0) - so with a zero offset the assert compares 0 < 0 and aborts the client. This was unreachable while container checksums defaulted to off. With them on, checksum verification drives the sgl processor over the fetch sgl and trips the assert. Step over exhausted iovs instead of indexing into them, keeping the corruption check as an equality assert on the offset. Features: checksum Test-tag: daos_test pr test_daos_container checksum Signed-off-by: Ryon Jensen <ryon.jensen@hpe.com>
Collaborator
|
Test stage Functional Cluster Box Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18885/6/testReport/ |
Collaborator
|
Test stage Functional Hardware Medium Verbs Provider 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-18885/6/execution/node/1632/log |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DAOS-17946 changed the default container properties to crc32 checksum and server-side checksum verification, but only updated the v0 default property table in src/container/srv_layout.c. cont_prop_entries_default, which is the table used for every pool with a global version >= 1, was left untouched, so containers created on any modern pool still defaulted to checksum off.
Apply the same defaults to cont_prop_entries_default: crc32 checksum, server-side verification on and RF3. Note that the container redundancy factor is always inherited from the pool when it is not explicitly specified, so the RF3 entry is only there to keep the two tables consistent.
The regression went unnoticed because the test harnesses hid it: daos_test forced checksum off on every container it created and the ftest container utility hardcoded "cksum:off,srv_cksum:off", both added by DAOS-17946. Drop those overrides and update the co_properties assertions so the defaults are actually exercised.
Steps for the author:
After all prior steps are complete: