DAOS-3114 vos: Consolidate timestamp cache APIs #3200

jolivier23 · 2020-08-04T22:58:04Z

Simplify the timestamp cache some by consolidating APIs. There is now one
API for checking for conflicts and one API for updating the read timestamps.
This is preparation for adding more read timestamp updates for iteration
and query APIs and for adding epoch uncertainty checks. More simplification
is probably needed but this is a step in that direction.

Update the mvcc test to enable same transaction tests. A few changes were
required for this to work

Change the code so that it only calls vts_dtx_begin/end/commit/abort once
per transaction and uses sequence number.
Disable 5 cases that require the punch model change patch that is
blocked by a rebuild bug.
Fixes a bug that was clearing the uuids in the timestamp cache
Disable cases where the first operation is a failing conditional update
or punch in the same transaction. These cases require more work in
VOS to support
Encapsulate vos_publish/cancel inside of vos_tx_end
Defer free for same transaction overwrites
Add full dtx_id to timestamps for comparisons

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

Extents covered by a minor epoch punch should not be visible. This modifies the handling of prior punches so that the minor epoch is accounted for. Modifies replay punches so that they use max minor epoch - 1 and non-transactional updates to use max minor epoch. This way rebuild can replay the punch using only the major epoch and the existing flag. Signed-off-by: Jeff Olivier <[email protected]>

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

a punch/update to execute in non-transactional mode with separate minor epochs Signed-off-by: Jeff Olivier <[email protected]>

Simplify the timestamp cache some by consolidating APIs. There is now one API for checking for conflicts and one API for updating the read timestamps. This is preparation for adding more read timestamp updates for iteration and query APIs and for adding epoch uncertainty checks. More simplification is probably needed but this is a step in that direction. Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

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

daosbuild1 · 2020-08-05T19:58:44Z

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/1/execution/node/803/log

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

daosbuild1

LGTM. No errors found by checkpatch.

liw · 2020-08-06T08:20:44Z

src/vos/vos_ts.h

+	/** Write level for the set */
+	uint16_t		 ts_wr_level;
+	/** Max type */
+	uint16_t		 ts_max_type;
 	/** Transaction that owns the set */
 	uuid_t			 ts_tx_id;


By the way, we'll need to change this field, as well as te_tx_rl and te_tx_rh, from uuid_t to dtx_id, because TXs opened by the same thread share the same UUID.

I don't think we do because the hlc guarantees that two transactions from the same thread will have different epoch, no?

I'm afraid that'll not always be the case, at least not without adding more restrictions. E.g., imagine the application uses Argobots, like daos_io_server, and opens more than TXs from one pthread. One of these TXs may choose its epoch on server x, getting 100, and read value v with 100; another may choose its epoch on server y, in parallel with the former TX, getting also 100, and write value v with 100. The latter should be rejected.

why would they use the same uuid ? Is the uuid not chosen on the leader that chooses the epoch?

Their UUIDs are generated by the same client thread.

liw · 2020-08-07T03:51:07Z

src/vos/vos_ts.h

+			if (ts_set->ts_max_type > VOS_TS_TYPE_OBJ)
+				break;
+		case VOS_TS_TYPE_DKEY:
+			high_mask |= VOS_TS_READ_DKEY_CHILD;


The fall-through and the _CHILD are redundant, are they not? Just want to make sure it's intentional.

yeah, they are but if we go straight to DKEY, it needs to be set

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

daosbuild1 · 2020-08-18T13:55:17Z

Test stage Build on Leap 15 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/21/execution/node/263/log

daosbuild1 · 2020-08-18T13:56:54Z

Test stage Build on CentOS 7 debug completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/21/execution/node/275/log

daosbuild1 · 2020-08-18T13:58:31Z

Test stage Build on CentOS 7 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/21/execution/node/292/log

daosbuild1 · 2020-08-18T13:58:48Z

Test stage Build on CentOS 7 release completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/21/execution/node/295/log

daosbuild1 · 2020-08-18T13:59:46Z

Test stage Build on Leap 15 with Intel-C and TARGET_PREFIX completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/21/execution/node/265/log

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

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2020-08-18T18:22:13Z

Test stage Functional_Hardware_Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-3200/22/execution/node/844/log

daosbuild1

LGTM. No errors found by checkpatch.

Add dedup to ignore list Signed-off-by: Jeff Olivier <[email protected]>

toward making negative entries part of the corresponding positive entry rather than their own timestamp cache. Signed-off-by: Jeff Olivier <[email protected]>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

Nasf-Fan

Some logic can be improved, but not fatal.

Nasf-Fan · 2020-08-19T10:15:14Z

src/vos/vos_io.c

+	if (!dtx_is_valid_handle(dth) || dth->dth_deferred == NULL)
+		return 0;
+
+	/** Reserve enough space for any deferred actions */


Some confused, my understand is that the "dth_deferred[i]" will be used as output parameter for pmemobj_defer_free(), one defer_free() only need one "pobj_action". Since we have already created N (N is the 'dth_modification_cnt') "vos_rsrvd_scm" slots when DTX handle init, then for each slot, we only need to create one "pobj_action", right? Do we really need so large size to hold multiple "pobj_action"? Or I missed anything?

Nasf-Fan · 2020-08-19T10:18:26Z

src/vos/vos_tree.c


-		rsrvd_scm = dru->dru_scm;
 		if (rsrvd_scm->rs_actv_at >= rsrvd_scm->rs_actv_cnt)
 			continue; /* Can't really be > but keep it simple */


The check is redundant, because each sub modification will call defer_free() at most once, "rs_actv_at" will be always zero.

if it modifies multiple akeys, it can happen more than once, no?

You are right. I ignored such case.
On the other hand, if some SCM area is not allocated by current DTX, defer_free() it, whether will it cause issue or not? I will assume that it can work since I am not familiar with the semantics for defer_free().

It will not cause an issue to defer free something not allocated by (or reserved for) the transaction. However, this case only happens if the reservation has the same epoch and is in the same tx.

Nasf-Fan · 2020-08-19T10:35:13Z

src/vos/vos_tree.c

-		if (dru->dru_scm == NULL)
-			continue;
+	for (i = 0; i < dth->dth_deferred_cnt; i++) {
+		rsrvd_scm = dth->dth_deferred[i];


We do not need to find out the free slot one by one. Because we have already created N "vos_rsrvd_scm" slots when init the DTX handle, so just directly use "rsrvd_scm = dth->dth_deferred[dth_op_seq];", nobody will share using such slot with current sub-modification.

Good point. I guess you can never have more undo's than the number of submodifications.

Simplify the timestamp cache some by consolidating APIs. There is now one API for checking for conflicts and one API for updating the read timestamps. This is preparation for adding more read timestamp updates for iteration and query APIs and for adding epoch uncertainty checks. More simplification is probably needed but this is a step in that direction. Update the mvcc test to enable same transaction tests. A few changes were required for this to work *Change the code so that it only calls vts_dtx_begin/end/commit/abort once per transaction and uses sequence number. *Disable 5 cases that require the punch model change patch that is blocked by a rebuild bug. *Fixes a bug that was clearing the uuids in the timestamp cache *Disable cases where the first operation is a failing conditional update or punch in the same transaction. These cases require more work in *VOS to support *Encapsulate vos_publish/cancel inside of vos_tx_end *Defer free for same transaction overwrites *Add full dtx_id to timestamps for comparisons Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 added 8 commits July 31, 2020 14:47

Address review comments

712495b

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

Merge branch 'master' into jvolivie/fix_minor_epc

da090c8

Merge branch 'master' into jvolivie/fix_minor_epc

2515749

Merge branch 'master' into jvolivie/fix_minor_epc

4a5ae4d

Fix one more issue

35355d5

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

Add a test for the rebuild use case. Fix a related issue allowing

d91ebf2

a punch/update to execute in non-transactional mode with separate minor epochs Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 requested review from NiuYawei, Nasf-Fan, gnailzenh and liw August 4, 2020 22:58

daosbuild1 reviewed Aug 4, 2020

View reviewed changes

fix a checkpatch warning

48f1131

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

jolivier23 requested a review from ryon-jensen August 5, 2020 14:03

jolivier23 added 4 commits August 6, 2020 08:21

Merge branch 'master' into jvolivie/fix_minor_epc

8ad8db7

Address review comments

f95b0f3

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

Add a limit in dtx_handle_init

4d9dc38

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

Merge branch 'master' into jvolivie/consolidate

142adad

daosbuild1 reviewed Aug 6, 2020

View reviewed changes

NiuYawei previously approved these changes Aug 7, 2020

View reviewed changes

liw previously approved these changes Aug 7, 2020

View reviewed changes

ryon-jensen previously approved these changes Aug 7, 2020

View reviewed changes

Merge branch 'master' into jvolivie/consolidate

f6b02c6

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

jolivier23 dismissed stale reviews from ryon-jensen, liw, and NiuYawei via f6b02c6 August 7, 2020 14:00

jolivier23 requested review from liw and NiuYawei August 7, 2020 14:00

Remove a typo

3b81348

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

jolivier23 requested review from liw and Nasf-Fan August 18, 2020 14:27

daosbuild1 reviewed Aug 18, 2020

View reviewed changes

Nasf-Fan previously approved these changes Aug 18, 2020

View reviewed changes

ryon-jensen previously approved these changes Aug 18, 2020

View reviewed changes

Merge branch 'master' into jvolivie/consolidate

68dc1d4

daosbuild1 reviewed Aug 18, 2020

View reviewed changes

jolivier23 added 2 commits August 18, 2020 14:23

Fix a bug with not cancelling reservations

e730549

Add dedup to ignore list Signed-off-by: Jeff Olivier <[email protected]>

Add new struct to encapsulate read timestamps and ids. This is a step

5b8cd98

toward making negative entries part of the corresponding positive entry rather than their own timestamp cache. Signed-off-by: Jeff Olivier <[email protected]>

jolivier23 dismissed stale reviews from ryon-jensen and Nasf-Fan via 5b8cd98 August 18, 2020 21:59

jolivier23 requested review from Nasf-Fan and ryon-jensen August 18, 2020 22:00

daosbuild1 reviewed Aug 18, 2020

View reviewed changes

ryon-jensen approved these changes Aug 18, 2020

View reviewed changes

Merge branch 'master' into jvolivie/consolidate

d1fd7cb

daosbuild1 reviewed Aug 18, 2020

View reviewed changes

liw approved these changes Aug 19, 2020

View reviewed changes

Nasf-Fan approved these changes Aug 19, 2020

View reviewed changes

jolivier23 merged commit c645966 into master Aug 19, 2020

jolivier23 deleted the jvolivie/consolidate branch August 19, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-3114 vos: Consolidate timestamp cache APIs #3200

DAOS-3114 vos: Consolidate timestamp cache APIs #3200

jolivier23 commented Aug 4, 2020 •

edited

Loading

daosbuild1 left a comment

daosbuild1 commented Aug 5, 2020

daosbuild1 left a comment

liw Aug 6, 2020

jolivier23 Aug 7, 2020

liw Aug 7, 2020

jolivier23 Aug 7, 2020

liw Aug 8, 2020

liw Aug 7, 2020

jolivier23 Aug 7, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 left a comment

daosbuild1 commented Aug 18, 2020

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 left a comment

Nasf-Fan left a comment

Nasf-Fan Aug 19, 2020

Nasf-Fan Aug 19, 2020

jolivier23 Aug 19, 2020

Nasf-Fan Aug 19, 2020 •

edited

Loading

jolivier23 Aug 19, 2020

Nasf-Fan Aug 19, 2020

jolivier23 Aug 19, 2020

DAOS-3114 vos: Consolidate timestamp cache APIs #3200

DAOS-3114 vos: Consolidate timestamp cache APIs #3200

Conversation

jolivier23 commented Aug 4, 2020 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Aug 5, 2020

daosbuild1 left a 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

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 commented Aug 18, 2020

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Aug 18, 2020

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

Nasf-Fan left a 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 Aug 19, 2020 • 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

jolivier23 commented Aug 4, 2020 •

edited

Loading

Nasf-Fan Aug 19, 2020 •

edited

Loading