DAOS-16876 vos: Improve error reporting on corruption crash #15747

jolivier23 · 2025-01-20T04:26:50Z

If the records referenced by a DTX have been removed from the tree, the data will no longer match.
We can't detect this in all cases but we can avoid overwriting when we do detect it.

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:

If the records referenced by a DTX have been removed from the tree, the data will no longer match. We can't detect this in all cases but we can avoid overwriting when we do detect it. Signed-off-by: Jeff Olivier <[email protected]>

github-actions · 2025-01-20T04:27:07Z

Ticket title is 'LRZ: m02r01s07dao engine coredumps with vos EMRG src/vos/ilog.c:411 ilog_open() Assertion'
Status is 'Awaiting Verification'
Labels: 'ALCF,LRZ,alcf_track,google-cloud-daos'
Job should run at elevated priority (1)
https://daosio.atlassian.net/browse/DAOS-16876

Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1 · 2025-01-20T05:20:15Z

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/3/testReport/

daosbuild1 · 2025-01-20T05:43:43Z

Test stage Unit Test bdev on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/3/testReport/

daosbuild1 · 2025-01-20T05:53:43Z

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/3/testReport/

daosbuild1 · 2025-01-20T07:18:33Z

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/3/testReport/

daosbuild1 · 2025-01-20T09:06:00Z

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/3/testReport/

Nasf-Fan · 2025-01-20T14:47:05Z

src/vos/vos_dtx.c

+		       DP_DTI(&DAE_XID(dae)), dtx_umoff_flag2type(rec));
+		d_tm_inc_counter(vos_tls_get(false)->vtl_dtx_rec_missing, 1);
+		rc = 0;
+	}


The user may miss the warning message. As we discussed, there is risk for silent data corruption. So when we hit recognisable corruption, I would suggest to trigger assertion that will make the user to be aware of potential data corruption in stead of make the system to go ahead without explicitly blocking the system.

On the other hand, in your current patch, if we do not access some corrupted DTX entry, the corruption will be always there, longer time then more dangerous since related area maybe reused by others as time going.

We can move your patch logic into some new DDB functionality to repair the dangling DTX records via removing them. Since it is inconvenient to directly access "epoch" via DTX record reference, then we can do that as your patch does with checking lid+magic (if have). That is some kind of "quick mode" or "quick scan".

In our case, we would monitor the metric and alert someone that it happened. I still don't think quick scan buys us anything. A full scan would be able to find all of the missing entries but a quick scan can only find entries like the ones in this patch.

I think maybe having this behavior be configurable would be better. Until we have full scan, this patch is really all we can really do

One of the main concern for this patch is that we passively handle the corrupted DTX entries when we access them, that may leave more windows as to more risks of data corruption. @janekmi is working on the "quick scan" via ddb as offline mode. Maybe we need both (part).

It seem since we can't be 100% sure the root cause of the missing records we can't actually tell the user everything is ok. TBH "some corruption" sounds really bad and this, in my understanding, is the state of the system since this discrepancies are being discovered. It seems not fit to continue normal operation.

IMHO we should not automatically decide to continue despite the DTX transaction is obviously incomplete.

Indeed, I tend to agree with @janekmi and @Nasf-Fan .

If we don't know exactly the root case, It's not safe to keep engine running, otherwise, more severe data corruption could be introduced;

If we know the root case for sure, we should fix the bug;

If the original assert can only be triggered by bug (it was a proper assert), it looks to me there is no way of getting round of it (I don't think software is able to gracefully handle it's own bug).

Nasf-Fan · 2025-01-20T14:48:11Z

src/vos/vos_obj_index.c

@@ -321,6 +322,7 @@ vos_oi_find_alloc(struct vos_container *cont, daos_unit_oid_t oid,
 	if (log) {
 		vos_ilog_desc_cbs_init(&cbs, vos_cont2hdl(cont));
 		rc = ilog_open(vos_cont2umm(cont), &obj->vo_ilog, &cbs, dth == NULL, &loh);
+		D_ASSERTF(rc == -DER_NONEXIST, "Uncorrectable incarnation log corruption detected");


rc != -DER_NONEXIST

Ah, right, I fixed the other one when a unit test failed

Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1 · 2025-01-20T19:51:47Z

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/4/testReport/

Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1 · 2025-01-21T04:55:10Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15747/5/execution/node/1199/log

daltonbohning · 2025-01-21T16:05:50Z

src/tests/ftest/util/telemetry_utils.py

@@ -207,6 +208,7 @@ class TelemetryUtils():
        _gen_stats_metrics("engine_io_dtx_committable")
    ENGINE_IO_DTX_COMMITTED_METRICS = \
        _gen_stats_metrics("engine_io_dtx_committed")
+    ENGINE_IO_DTX_RECORD_MISSING_METRICS=["engine_io_dtx_record_missing"]


E225 missing whitespace around operator

Suggested change

ENGINE_IO_DTX_RECORD_MISSING_METRICS=["engine_io_dtx_record_missing"]

ENGINE_IO_DTX_RECORD_MISSING_METRICS = ["engine_io_dtx_record_missing"]

daltonbohning · 2025-01-21T16:06:17Z

src/tests/ftest/util/telemetry_utils.py

@@ -310,6 +312,7 @@ class TelemetryUtils():
    ENGINE_IO_METRICS = ENGINE_IO_DTX_ASYNC_CMT_LAT_METRICS +\
        ENGINE_IO_DTX_COMMITTABLE_METRICS +\
        ENGINE_IO_DTX_COMMITTED_METRICS +\
+        ENGINE_IO_DTX_RECORD_MISSING_METRIC +\


undefined-variable, Undefined variable 'ENGINE_IO_DTX_RECORD_MISSING_METRIC'

Missing a list definition?

missed the S at the end

Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1 · 2025-01-22T02:34:03Z

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-15747/6/execution/node/1477/log

janekmi · 2025-01-22T11:02:09Z

src/vos/vos_dtx.c

@@ -1,5 +1,6 @@
-/**
+/**RY


Please clean it up.

janekmi · 2025-01-22T11:09:23Z

src/vos/vos_dtx.c

+		       DP_DTI(&DAE_XID(dae)), dtx_umoff_flag2type(rec));
+		d_tm_inc_counter(vos_tls_get(false)->vtl_dtx_rec_missing, 1);
+		rc = 0;
+	}


It seem since we can't be 100% sure the root cause of the missing records we can't actually tell the user everything is ok. TBH "some corruption" sounds really bad and this, in my understanding, is the state of the system since this discrepancies are being discovered. It seems not fit to continue normal operation.

IMHO we should not automatically decide to continue despite the DTX transaction is obviously incomplete.

janekmi · 2025-01-22T11:13:07Z

src/vos/vos_dtx.c

+		D_WARN("record no longer exists, may indicate some corruption: " DF_DTI
+		       " type %u\n",
+		       DP_DTI(&DAE_XID(dae)), dtx_umoff_flag2type(rec));
+		d_tm_inc_counter(vos_tls_get(false)->vtl_dtx_rec_missing, 1);


Is there anything the user can do with this counter? How many potential corruptions the user can accept before felt not comfortable storing data in the system?

It seems pointless for the end user. Did you envision it as a feature for developers to track the issue when the system is online?

Will probably remove the metric since we decided abort is better but, in general, metrics are for admins. We use them all the time to try to diagnose issues with running systems.

janekmi · 2025-01-22T11:16:40Z

src/vos/ilog.c

@@ -1213,7 +1221,7 @@ ilog_fetch(struct umem_instance *umm, struct ilog_df *root_df,
 	int			 rc = 0;
 	bool			 retry;

-	ILOG_ASSERT_VALID(root_df);
+	ILOG_CHECK_VALID(root_df);


Why just a warning in this case?

yeah, I think we have to assert.

It will assert in the caller

nevermind, not sure why I did that.

janekmi · 2025-01-22T11:19:17Z

src/vos/vos_dtx.c

@@ -571,7 +572,7 @@ dtx_ilog_rec_release(struct umem_instance *umm, struct vos_container *cont,

 	ilog_close(loh);

-	if (rc != 0)
+	if (rc != 0 && rc != -DER_NONEXIST)


I understand the DTX transaction still can't be committed nor aborted in this case, right?

That is good point. Considering the scenarios of generating the dangling DTX reference: the modification was made against some akey, but before its committed/aborted, related dkey was removed; or the modification was made against some dkey, but related object was removed. So under these scenarios, the subsequent modifications under related {d,a}key became meaningless, but we may cannot detect all dangling DTX records if there is no magic information (such as for SV) or "lid" maybe reused. So once we detect and discard the dangling DTX record for ilog, then all the subsequent modification(s) after such ilog DTX record should be discarded (until another valid ilog DTX record was found for distributed transaction case in the future).

Isn't it too late to check DER_NONEXIST here? It should have returned error on ilog_open() above, right?

I don't see why we want to avoid error messages on DER_NONEXIST errors.

Isn't it too late to check DER_NONEXIST here? It should have returned error on ilog_open() above, right?

The caller - do_dtx_rec_release() will print the log message.

daosbuild1 · 2025-02-12T02:24:57Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/12/testReport/

Signed-off-by: Jeff Olivier <[email protected]>

Allow-unstable-test: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 · 2025-02-13T00:23:28Z

@Nasf-Fan @janekmi can you have another look?

When commit or abort a DTX, we will check whether it is a valid entry or not. For invalid case, we will discard it with warning message and increase related metrics counter. It may be not perfect solution, but it is efficient to help the user to cleanup system efficiently. Signed-off-by: Jeff Olivier <[email protected]> Signed-off-by: Fan Yong <[email protected]>

Signed-off-by: Jeff Olivier <[email protected]> Signed-off-by: Fan Yong <[email protected]>

daosbuild1 · 2025-02-14T00:52:51Z

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15747/18/testReport/

Signed-off-by: Fan Yong <[email protected]>

Signed-off-by: Jeff Olivier <[email protected]> Signed-off-by: Fan Yong <[email protected]>

daosbuild1 · 2025-02-14T02:50:08Z

Test stage Unit Test with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/20/display/redirect

daosbuild1 · 2025-02-14T02:50:09Z

Test stage Unit Test bdev on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/20/display/redirect

daosbuild1 · 2025-02-14T02:50:11Z

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/20/display/redirect

daosbuild1 · 2025-02-14T02:50:12Z

Test stage Unit Test on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/20/display/redirect

daosbuild1 · 2025-02-14T02:50:15Z

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/20/display/redirect

daosbuild1 · 2025-02-14T02:59:32Z

Test stage Unit Test with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/21/display/redirect

daosbuild1 · 2025-02-14T02:59:33Z

Test stage Unit Test bdev with memcheck on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/21/display/redirect

daosbuild1 · 2025-02-14T02:59:36Z

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15747/21/display/redirect

Nasf-Fan · 2025-02-14T14:57:05Z

Ping reviewers, Thanks!

Nasf-Fan · 2025-02-16T02:18:32Z

Ping @NiuYawei , thanks!

NiuYawei · 2025-02-16T08:52:25Z

src/vos/ilog.c

@@ -363,7 +366,8 @@ ilog_open(struct umem_instance *umm, struct ilog_df *root,
 	struct ilog_context	*lctx;
 	int			 rc;

-	ILOG_ASSERT_VALID(root);
+	if (!ILOG_CHECK_VALID(root))
+		return -DER_NONEXIST;


Better to add an error message.

NiuYawei · 2025-02-16T08:52:44Z

src/vos/ilog.c

-	    (opc == ILOG_OP_PERSIST || opc == ILOG_OP_ABORT)) {
+	if (rc == 0 && opc != ILOG_OP_UPDATE) {
+		if (version == ilog_mag2ver(lctx->ic_root->lr_magic)) {
+			D_WARN("ilog entry on %s doesn't exist\n", opc_str[opc]);


D_ERROR() instead of D_WARN().

NiuYawei · 2025-02-16T08:53:30Z

src/vos/vos_dtx.c

@@ -571,7 +572,7 @@ dtx_ilog_rec_release(struct umem_instance *umm, struct vos_container *cont,

 	ilog_close(loh);

-	if (rc != 0)
+	if (rc != 0 && rc != -DER_NONEXIST)


I don't see why we want to avoid error messages on DER_NONEXIST errors.

old review

DAOS-16876 vos: Avoid crash on missing records

0d76a1e

If the records referenced by a DTX have been removed from the tree, the data will no longer match. We can't detect this in all cases but we can avoid overwriting when we do detect it. Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from a team as code owners January 20, 2025 04:26

github-actions bot added the priority Ticket has high priority (automatically managed) label Jan 20, 2025

jolivier23 requested review from janekmi, Nasf-Fan, gnailzenh, wangdi1 and NiuYawei January 20, 2025 04:28

jolivier23 added 2 commits January 19, 2025 21:37

Need to get clang format working locally

94d3b70

Signed-off-by: Jeff Olivier <[email protected]>

Another try

5a767b5

Signed-off-by: Jeff Olivier <[email protected]>

Nasf-Fan requested changes Jan 20, 2025

View reviewed changes

jolivier23 added 2 commits January 20, 2025 08:24

Update vos_obj_index.c

6bb5e3a

Signed-off-by: Jeff Olivier <[email protected]>

Merge branch 'master' into jvolivie/corrupt

51a2461

fix functional test

2e55c78

Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from a team as code owners January 20, 2025 23:48

Nasf-Fan mentioned this pull request Jan 21, 2025

DAOS-16951 dtx: add invalid records discard capability #15752

Merged

18 tasks

daltonbohning reviewed Jan 21, 2025

View reviewed changes

Fixes typo

4a80eac

Signed-off-by: Jeff Olivier <[email protected]>

janekmi previously requested changes Jan 22, 2025

View reviewed changes

jolivier23 added 2 commits February 11, 2025 21:54

Allow-unstable-test: true

cabf59e

Signed-off-by: Jeff Olivier <[email protected]>

clang-format

09293bf

Allow-unstable-test: true Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested a review from NiuYawei February 12, 2025 15:54

Nasf-Fan added 2 commits February 14, 2025 01:29

DAOS-16876 vos: resolve some copyright warning

1cde1b6

Signed-off-by: Jeff Olivier <[email protected]> Signed-off-by: Fan Yong <[email protected]>

Nasf-Fan added 2 commits February 14, 2025 10:25

DAOS-16876 tests: for new added metrics io_dtx_invalid

0c88a2f

Signed-off-by: Fan Yong <[email protected]>

DAOS-16876 tests: resolve copyright warning

7df3970

Signed-off-by: Jeff Olivier <[email protected]> Signed-off-by: Fan Yong <[email protected]>

gnailzenh approved these changes Feb 14, 2025

View reviewed changes

NiuYawei approved these changes Feb 16, 2025

View reviewed changes

Nasf-Fan requested review from a team and Nasf-Fan and removed request for Nasf-Fan February 17, 2025 02:21

Nasf-Fan approved these changes Feb 17, 2025

View reviewed changes

jolivier23 merged commit e98c5ac into master Feb 19, 2025
56 of 57 checks passed

jolivier23 deleted the jvolivie/corrupt branch February 19, 2025 00:44

	ENGINE_IO_DTX_RECORD_MISSING_METRICS=["engine_io_dtx_record_missing"]
	ENGINE_IO_DTX_RECORD_MISSING_METRICS = ["engine_io_dtx_record_missing"]

		@@ -1,5 +1,6 @@
		/**
		/**RY

DAOS-16876 vos: Improve error reporting on corruption crash #15747

DAOS-16876 vos: Improve error reporting on corruption crash #15747

Conversation

jolivier23 commented Jan 20, 2025 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 20, 2025 • edited Loading

daosbuild1 commented Jan 20, 2025

daosbuild1 commented Jan 20, 2025

daosbuild1 commented Jan 20, 2025

daosbuild1 commented Jan 20, 2025

daosbuild1 commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nasf-Fan Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 20, 2025

daosbuild1 commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nasf-Fan Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Feb 12, 2025

jolivier23 commented Feb 13, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

daosbuild1 commented Feb 14, 2025

Nasf-Fan commented Feb 14, 2025

Nasf-Fan commented Feb 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolivier23 commented Jan 20, 2025 •

edited

Loading

github-actions bot commented Jan 20, 2025 •

edited

Loading

Nasf-Fan Jan 21, 2025 •

edited

Loading

Nasf-Fan Jan 24, 2025 •

edited

Loading