DAOS-6967 vos: Data loss on updates after uncommitted punch #4899

jolivier23 · 2021-03-06T03:56:40Z

The main issue we are trying to prevent is removal of a subtree that isn't empty.
This typically means that we have a bug in aggregation or in the incarnation log

This patch does the following:

Modifies the btree code so that it returns aborted entries during aggregation.
Modify ilog_aggregate so it takes a minor epoch into account. Previously, if we had a punch of a parent, it would remove the child at the same major epoch even if it had updates afterward.
Modify the incarnation log to ensure we take the punch of a parent into account when doing an update
4.Modify aggregation so that we skip aggregation of parent objects if a child iterator hits -DER_TX_BUSY. This avoids removing orphaned subtrees.
Aggregation of the incarnation log was returning -DER_TX_BUSY for entries that are after the aggregated range. Moved the check to after the range check.
Modify evtree aggregation to restart the current tree in case of removing an aborted entry rather than returning -DER_TX_BUSY which would cause upper layer to abort too.
Fixed a few other issues with aggregation.

Signed-off-by: Jeff Olivier jeffrey.v.olivier@intel.com

If a punch of a parent object/key is uncommited, a new incarnation log entry needs to be inserted. This reguressed with the addition of minor epochs. Added a regression test that exercises the problematic case. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

src/vos/vos_io.c

src/vos/vos_obj.c

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

* Modify aggregate to bypass aggregation if child tree was aborted * Fix an issue with incarnation log. We don't need to abort aggregation if the uncommitted entry is after the aggregation period so move the check. Note, there is an issue with single value still because it doesn't abort the aggregation under such cases so this is a WIP Skip-test: true Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

btr_check_availability for single value but I think this is best left for 2.0 Modify the reset to also reset the ta_reprobe flags Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

jolivier23 · 2021-03-11T18:33:14Z

Note, I have ideas for lots more VOS level testing....but it doesn't seem practical for a 1.2 patch at this point

jolivier23 · 2021-03-14T07:02:50Z

Failure is due to DAOS-7016

NiuYawei · 2021-03-15T06:03:11Z

src/common/btree.c

@@ -3658,6 +3665,10 @@ dbtree_iter_delete(daos_handle_t ih, void *args)
 	if (rc != 0)
 		return rc;

+	/** The probed entry is not committed so return -DER_TX_BUSY */


I don't see why an aborted entry can't be removed?

You are right. This needs to be for in-progress entries. We need another way to determine if an entry is aborted. I don't think the dtx_status stuff is working for single value

NiuYawei · 2021-03-15T06:09:08Z

src/vos/vos_aggregate.c

@@ -409,6 +413,16 @@ vos_agg_sv(daos_handle_t ih, vos_iter_entry_t *entry,

 delete:
 	rc = agg_del_entry(ih, agg_param->ap_umm, entry, acts);
+	if (rc == -DER_TX_BUSY) {


Isn't the uncommitted check done above (by checking the entry->ie_dtx_state)?

NiuYawei · 2021-03-15T06:16:18Z

src/vos/vos_aggregate.c

@@ -180,7 +183,8 @@ agg_del_entry(daos_handle_t ih, struct umem_instance *umm,
 		rc = umem_tx_commit(umm);

 	if (rc) {
-		D_ERROR("Failed to delete entry: "DF_RC"\n", DP_RC(rc));
+		D_CDEBUG(rc == -DER_TX_BUSY, DB_TRACE, DLOG_ERR,
+			 "Failed to delete entry: "DF_RC"\n", DP_RC(rc));


I think current aggregation can ensure not deleting prepared entries, no?

NiuYawei · 2021-03-15T06:41:33Z

src/vos/vos_aggregate.c

-		 * moving forward.
+		 * moving forward.   We do, however, need to ensure we do not
+		 * aggregate anything in the parent path.  Otherwise, we could
+		 * orphan the current entry due to incarnation log semantics.


I don't quite see the "incarnation log semantics" here, could you explain a bit?

I don't see how this "ap_skip_obj/dkey/akey" works as well, the intention is to set the skip flag for current epoch range aggregation? Or for all epoch ranges aggregation?

I discussed this with you offline but for everyone's benefit, the issue is that the incarnation log doesn't contain enough information to know at the key level that there are in-progress updates that we may orphan by aggregating the ilog. The example I used is this one

committed update at 1

in progress update at 2 - this will not generate an incarnation log entry

punch at 3

From the ilog perspective, we can delete everything but the subtree will not be empty due to the inprogress update.

may need re-probe. Evtree reprobe does the sort over again anyway. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 · 2021-03-15T21:44:49Z

Test stage NLT completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-4899/7/display/redirect

daosbuild1

LGTM. No errors found by checkpatch.

NiuYawei · 2021-03-16T02:09:13Z

src/vos/vos_aggregate.c

-			break;
+			D_DEBUG(DB_EPC, "Hit uncommitted single value at epoch:"
+				DF_X64"\n", entry->ie_epoch);
+			return -DER_TX_BUSY;


The comments above need be revised, the original design is to skip the uncommitted epoch and continue on the next lower epoch.

Ok, I will fix that.

NiuYawei · 2021-03-16T02:16:15Z

src/vos/vos_iterator.c

+			if (acts & VOS_ITER_CB_RESTART) {
+				daos_anchor_set_zero(anchor);
+				probe_anchor = NULL;
+				goto probe;


Simply re-probe won't work for sorted evtree iteration, you need to re-prepare the sorted entries on restart, since the deletion before restart have invalidated the visibility of evtree entries.

I think a simpler way is to rely on next round of aggregation to aggregate the same epoch range again. (we just need to skip updating HAE when aggregation aborted on hitting aborted or uncommitted entries.)

You don't need to re-prepare. For sorted iterator, probe is what re-reads all the entries and sorts them so it works to simply re-probe in this case. See evt_iter_probe_sorted. I suppose it may be a bit weird for upper levels but it's only really used by evtree so maybe it's fine. I originally did re-prepare but this actually requires a reprobe of the parent iterator and is more complicated.

We could do as you say and just abort but what is the advantage of doing that?

Ok, I was thinking reprobe won't re-read all entries.

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Nasf-Fan · 2021-03-16T15:43:08Z

src/vos/ilog.c

+		return true;
+
+	return minor_epc <= agg_arg->aa_punched_minor;
+


if (entry->ie_id.id_punch_minor_eph > entry->ie_id.id_update_minor_eph &&
entry->ie_id.id_punch_minor_eph > agg_arg->aa_punched_minor &&
entry->ie_id.id_epoch == agg_arg->aa_punched)

Then it is a punched entry or not?

If the entry is punched (e.g. punch_minor > update_minor) then we will determine later that the entry is punched. But this check is only about whether it is punched entirely by the parent.

Nasf-Fan · 2021-03-16T17:55:00Z

src/vos/vos_iterator.c

@@ -683,11 +681,18 @@ vos_iterate_internal(vos_iter_param_t *param, vos_iter_type_t type,
 			if (acts & VOS_ITER_CB_ABORT)
 				break;

+			if (acts & VOS_ITER_CB_RESTART) {
+				daos_anchor_set_zero(anchor);
+				probe_anchor = NULL;


Set "probe_anchor" as NULL may cause some potential trouble in our current implementation, please check evt_iter_probe_sorted(), it assumes that the input parameter "rect" and "anchor" will not be both NULL. But that is not true in your patch. I am not quite sure for that yet, but need some detailed check/test.

If anchor is NULL, it passes EVT_ITER_FIRST to evt_iter_probe. In this case, it doesn't reach the scenario your speak of.

Nasf-Fan · 2021-03-17T02:22:30Z

src/vos/vos_aggregate.c

-			*acts |= VOS_ITER_CB_ABORT;
-			if (rc == -DER_CSUM)
-				agg_param->ap_csum_err = true;
+			if (rc == -DER_TX_RESTART)


This check will be always false since you have checked "if (rc == -DER_CSUM || rc == -DER_TX_BUSY)" at L1834.

You are right. I guess I never actually hit this case because the case that was causing issues was that aborted entries were getting ignored entirely. I'll fix.

I reworked this a bit

Nasf-Fan · 2021-03-17T02:41:48Z

src/vos/tests/vts_pm.c

+	assert_memory_equal(buf, "xxxxx", 5);
+
+	memset(buf, 'x', sizeof(buf));
+	epoch += 10;


It is unnecessary to bump the epoch again for another fetch.

True but it also doesn't hurt anything

Not hurt anything, just redundant.

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Nasf-Fan · 2021-03-17T04:06:34Z

src/vos/tests/vts_pm.c

+	assert_memory_equal(buf, "xxxxx", 5);
+
+	memset(buf, 'x', sizeof(buf));
+	epoch += 10;


Not hurt anything, just redundant.

Nasf-Fan · 2021-03-17T04:13:57Z

src/vos/vos_iterator.c

@@ -683,11 +681,18 @@ vos_iterate_internal(vos_iter_param_t *param, vos_iter_type_t type,
 			if (acts & VOS_ITER_CB_ABORT)
 				break;

+			if (acts & VOS_ITER_CB_RESTART) {
+				daos_anchor_set_zero(anchor);
+				probe_anchor = NULL;


The main issue we are trying to prevent is removal of a subtree that isn't empty. This typically means that we have a bug in aggregation or in the incarnation log This patch does the following: Modifies the btree code so that it returns aborted entries during aggregation. Modify ilog_aggregate so it takes a minor epoch into account. Previously, if we had a punch of a parent, it would remove the child at the same major epoch even if it had updates afterward. Modify the incarnation log to ensure we take the punch of a parent into account when doing an update 4.Modify aggregation so that we skip aggregation of parent objects if a child iterator hits -DER_TX_BUSY. This avoids removing orphaned subtrees. Aggregation of the incarnation log was returning -DER_TX_BUSY for entries that are after the aggregated range. Moved the check to after the range check. Modify evtree aggregation to restart the current tree in case of removing an aborted entry rather than returning -DER_TX_BUSY which would cause upper layer to abort too. Fixed a few other issues with aggregation. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

…5059) The main issue we are trying to prevent is removal of a subtree that isn't empty. This typically means that we have a bug in aggregation or in the incarnation log This patch does the following: Modifies the btree code so that it returns aborted entries during aggregation. Modify ilog_aggregate so it takes a minor epoch into account. Previously, if we had a punch of a parent, it would remove the child at the same major epoch even if it had updates afterward. Modify the incarnation log to ensure we take the punch of a parent into account when doing an update 4.Modify aggregation so that we skip aggregation of parent objects if a child iterator hits -DER_TX_BUSY. This avoids removing orphaned subtrees. Aggregation of the incarnation log was returning -DER_TX_BUSY for entries that are after the aggregated range. Moved the check to after the range check. Modify evtree aggregation to restart the current tree in case of removing an aborted entry rather than returning -DER_TX_BUSY which would cause upper layer to abort too. Fixed a few other issues with aggregation. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

jolivier23 requested a review from Nasf-Fan March 6, 2021 03:56

daosbuild1 reviewed Mar 6, 2021

View reviewed changes

jolivier23 requested review from gnailzenh, NiuYawei and ryon-jensen March 6, 2021 04:00

ryon-jensen reviewed Mar 8, 2021

View reviewed changes

src/vos/vos_io.c Outdated Show resolved Hide resolved

src/vos/vos_obj.c Outdated Show resolved Hide resolved

jolivier23 added 2 commits March 8, 2021 10:27

Address review comment

575ab34

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

Merge branch 'master' into jvolivie/punch

e4bff3d

jolivier23 requested a review from ryon-jensen March 8, 2021 17:31

Oops, removed a print

4ab6751

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 8, 2021

View reviewed changes

ryon-jensen previously approved these changes Mar 8, 2021

View reviewed changes

NiuYawei previously approved these changes Mar 9, 2021

View reviewed changes

Merge branch 'master' into jvolivie/punch

8ed74f0

daosbuild1 reviewed Mar 9, 2021

View reviewed changes

jolivier23 added 2 commits March 10, 2021 15:11

Merge branch 'master' into jvolivie/punch

fa892ce

jolivier23 dismissed stale reviews from NiuYawei and ryon-jensen via 318aaf1 March 11, 2021 00:43

daosbuild1 reviewed Mar 11, 2021

View reviewed changes

jolivier23 added 2 commits March 11, 2021 15:06

Merge branch 'master' into jvolivie/punch

8baa945

Add tc_aborted flag. Ideally, we would refactor to remove

23f1258

btr_check_availability for single value but I think this is best left for 2.0 Modify the reset to also reset the ta_reprobe flags Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 11, 2021

View reviewed changes

jolivier23 requested review from NiuYawei and ryon-jensen March 11, 2021 18:17

ryon-jensen previously approved these changes Mar 12, 2021

View reviewed changes

NiuYawei reviewed Mar 15, 2021

View reviewed changes

Restart without re-prepare because parent iterator entry

011ba45

may need re-probe. Evtree reprobe does the sort over again anyway. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 15, 2021

View reviewed changes

jolivier23 requested review from NiuYawei and ryon-jensen March 15, 2021 21:47

NiuYawei reviewed Mar 16, 2021

View reviewed changes

jolivier23 requested a review from NiuYawei March 16, 2021 02:42

Address a review comment

ea5015c

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 16, 2021

View reviewed changes

NiuYawei previously approved these changes Mar 16, 2021

View reviewed changes

ryon-jensen previously approved these changes Mar 16, 2021

View reviewed changes

Nasf-Fan requested changes Mar 17, 2021

View reviewed changes

jolivier23 added 2 commits March 16, 2021 21:18

Merge branch 'master' into jvolivie/punch

8d2fa51

Fix an issue caught in review

78edf74

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

jolivier23 dismissed stale reviews from ryon-jensen and NiuYawei via 78edf74 March 17, 2021 03:37

jolivier23 requested review from Nasf-Fan, NiuYawei and ryon-jensen March 17, 2021 03:37

daosbuild1 reviewed Mar 17, 2021

View reviewed changes

Fix stupid compiler warning

0b4464a

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 17, 2021

View reviewed changes

Nasf-Fan approved these changes Mar 17, 2021

View reviewed changes

NiuYawei approved these changes Mar 17, 2021

View reviewed changes

jolivier23 merged commit e8f5784 into master Mar 17, 2021

jolivier23 deleted the jvolivie/punch branch March 17, 2021 21:32

ashleypittman mentioned this pull request Apr 28, 2021

1.2 dfuse single #5582

Closed

ashleypittman mentioned this pull request May 20, 2021

1.2 proc #5753

Closed

DAOS-6967 vos: Data loss on updates after uncommitted punch #4899

DAOS-6967 vos: Data loss on updates after uncommitted punch #4899

Conversation

jolivier23 commented Mar 6, 2021 • edited Loading

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

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

jolivier23 commented Mar 11, 2021

jolivier23 commented Mar 14, 2021

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 Mar 15, 2021

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

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

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

Choose a reason for hiding this comment

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

jolivier23 commented Mar 6, 2021 •

edited

Loading