DAOS-16982 csum: recalculate checksum on retrying

[jxiong]

This PR fixes retry logic by actually recalculating the checksum; also it removes the code that incorrectly records nvme error.

Change-Id: Ib0287851fea4d125eecda48c5ccb3c73ed85b8f8
Signed-off-by: Jinshan Xiong [email protected]

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 owners.
• 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]

Ticket title is 'We should not report checksum errors against the nmve device for key verification'
Status is 'Open'
Labels: 'google-cloud-daos'
https://daosio.atlassian.net/browse/DAOS-16982

[jxiong]

I have already tested it by manually injecting failure, and I'm working on turning that into a unit test.

diff --git a/src/object/cli_obj.c b/src/object/cli_obj.c
index fdd9528a0..700abe77e 100644
--- a/src/object/cli_obj.c
+++ b/src/object/cli_obj.c
@@ -5141,6 +5141,7 @@ obj_csum_update(struct dc_object *obj, daos_obj_update_t *args, struct obj_auxi_
 		return 0;

 	if (obj_auxi->csum_retry) {
+	  	D_ERROR("recalculate csum error\n");
 		/* release old checksum result and prepare for new calculation */
 		daos_csummer_free_ic(obj->cob_co->dc_csummer, &obj_auxi->rw_args.iod_csums);
 	}
@@ -5156,6 +5157,7 @@ obj_csum_fetch(const struct dc_object *obj, daos_obj_fetch_t *args,
 	       struct obj_auxi_args *obj_auxi)
 {
 	if (obj_auxi->csum_retry) {
+	  	D_ERROR("recalculate csum error\n");
 		/* release old checksum result and prepare for new calculation */
 		daos_csummer_free_ic(obj->cob_co->dc_csummer, &obj_auxi->rw_args.iod_csums);
 	}
diff --git a/src/object/srv_obj.c b/src/object/srv_obj.c
index 26240540b..aae06ef61 100644
--- a/src/object/srv_obj.c
+++ b/src/object/srv_obj.c
@@ -1366,10 +1366,16 @@ obj_local_rw_internal(crt_rpc_t *rpc, struct obj_io_context *ioc, daos_iod_t *io
 		D_GOTO(out, rc = 0);
 	}

+	static int fail_count = 5;
+
 	rc = csum_verify_keys(ioc->ioc_coc->sc_csummer, &orw->orw_dkey,
 			      orw->orw_dkey_csum, &orw->orw_iod_array,
 			      &orw->orw_oid);
-	if (rc != 0) {
+	if (fail_count > 0) {
+	  	fail_count--;
+		rc = -DER_CSUM;
+	}
+	if (rc != 0) {
 		D_ERROR(DF_C_UOID_DKEY"verify_keys error: "DF_RC"\n",
 			DP_C_UOID_DKEY(orw->orw_oid, &orw->orw_dkey),
 			DP_RC(rc));

[jolivier23]

I think we probably want to do this after a couple of retries

[jxiong]

It's really easy to add but I wonder if that is indeed necessary, because cksum error is a rare event by itself.

How about revising it to:

if (obj_auxi->csum_retry && obj_auxi->csum_retry_cnt > 2) { ... }

would that work for you?

[jolivier23]

In the case of the actual issue we saw, it was the dkey_csum that needs to be recalculated, is that happening here?

[jxiong]

Yes if I read the code correctly because we release the previous calculation above.