DAOS-2790 evtree: Fix some issues with evtree distance sort by jolivier23 · Pull Request #736 · daos-stack/daos

jolivier23 · 2019-07-08T23:47:30Z

When middle node is equal distant from both sides, just
split evenly
Modify the minor field for the weight to be distance from
new rectangle to start epoch of mbr. Use the distance
between the original mbr and this value as the weight
Fix some off by one issues with boundary calculations

1. When middle node is equal distant from both sides, just split evenly 2. Modify the minor field for the weight to be distance from new rectangle to start epoch of mbr. Use the distance between the original mbr and this value as the weight 3. Fix some off by one issues with boundary calculations Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

jolivier23 · 2019-07-08T23:54:43Z


-	entry_src = evt_node_entry_at(tcx, nd_src, nr - 1);
-	dist = evt_mbr_dist(evt_node_mbr_get(tcx, nd_src), &entry_src->ne_rect);
+	entry_src = evt_node_entry_at(tcx, nd_src, nr);


Note that using nr is safe here because this is only called on split (full node) and the minimum order is 4. So tn_nr / 2 + 1 will always be < order. The + 1 only happens for odd numbers so worst case is order of 5 in which case nr would be 3.

jolivier23 · 2019-07-09T00:04:55Z

@gnailzenh I have been unable to reproduce the aggregation bug but this addresses a few issues with the algorithm. Item #2 in the description is unrelated but something I noticed while playing with overwrites in same epoch. In existing code, if you insert the same entry in [order] consecutive epochs and then insert another one, it would always insert the new entry with the lower half of epochs. By taking into account the distance from the mbr to the new epoch, we can ensure that for overwrite, we end up grouping the new entry with the higher epochs.

daosbuild1 · 2019-07-09T04:36:34Z

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

daosbuild1 · 2019-07-09T17:58:55Z

Test stage Functional completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-736/3/execution/node/547/log

daosbuild1 · 2019-07-10T02:03:10Z

Test stage Functional completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-736/4/execution/node/170/log

NiuYawei · 2019-07-10T03:11:07Z

-evt_common_rect_weight(struct evt_context *tcx, const struct evt_rect *rect,
-		       struct evt_weight *weight)
+evt_common_rect_weight(struct evt_context *tcx, daos_epoch_t epoch,
+		       const struct evt_rect *rect, struct evt_weight *weight)


I don't quite understand this change, this function is to calculate 'weight' of a specified MBR, why do we need to pass a 'eopch' in and comparing it with the MBR epoch?

Instead of changing all these weight calculating/diff functions, I think we'd just improve the evt_select_node() to further compare the epoch distance (new rect with two MBRs) when the two MBR weight diffs are same, that looks more straightforward to me, what do you think?

yeah, I could do that as well. The "minor" field is kind of useless without a high epoch. It only shows a weight difference if the new rectangle underwrites the mbr. If we go that direction, perhaps we should just have one uint64 for "weight" and handle the epoch externally

Ok, it doesn't work very well the way I have it. Don't review this yet

Ok, updated again. I favored "disabling" minor. Ideally, we'd add upper bound for mbr and get an actual weight difference. But in the absence of such, the only case that really matters is overwrite (non-overwrites get sorted by the extent). And in such cases, the distance to the start epoch of the MBR is a good heuristic for where to put the rectangle.

NiuYawei · 2019-07-10T03:24:57Z


 	if (ent_array.ea_ent_nr == 0)
-		return -DER_ENOENT;
+		return -DER_NONEXIST;


Not a big deal, but why it's changed from ENOENT to NONEXIST?

just because this is the only place in vos we return ENOENT

we should remove one of those error code...

NiuYawei · 2019-07-10T09:41:46Z

@gnailzenh I have been unable to reproduce the aggregation bug but this addresses a few issues with the algorithm. Item #2 in the description is unrelated but something I noticed while playing with overwrites in same epoch. In existing code, if you insert the same entry in [order] consecutive epochs and then insert another one, it would always insert the new entry with the lower half of epochs. By taking into account the distance from the mbr to the new epoch, we can ensure that for overwrite, we end up grouping the new entry with the higher epochs.

FYI, I tried to reproduce the aggregation error on current master in following way: disable all other aggregation tests and leave VOS412 only, then run the "vos_test -a" in a loop, with EVT_FEAT_SORT_DIST, the evt_delete() -DER_NOENT error is reproduced on 9th iteration, with EVT_FEAT_SORT_SOFF, the error can't be reproduced (30 iterations in total).

If you can reproduce it in same way, it could be helpful for debugging.

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

daosbuild1 · 2019-07-10T19:28:49Z

Test stage Functional completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-736/6/execution/node/547/log

NiuYawei · 2019-07-11T00:41:03Z

 	/* NB: they can be negative */
 	wt_diff->wt_major = wt1->wt_major - wt2->wt_major;
-	wt_diff->wt_minor = wt1->wt_minor - wt2->wt_minor;
+	/** wt2 is the difference to the original mbr */


Minor: this function is just a common function comparing two weights, this comment should be put somewhere else.

NiuYawei · 2019-07-11T00:41:49Z

 	wt_diff->wt_major = wt1->wt_major - wt2->wt_major;
-	wt_diff->wt_minor = wt1->wt_minor - wt2->wt_minor;
+	/** wt2 is the difference to the original mbr */
+	wt_diff->wt_minor = wt1->wt_major - wt2->wt_minor;


Is the 'wt_major' a typo?

Good catch :-). Will fix that

NiuYawei · 2019-07-11T00:54:27Z

+			return nd1;
+		else
+			return nd2;
+	}


If there are two full overwrite nodes, node1 have epoch 1, 2, 3, 4, node2 have epoch 6, 7, 8, 9 10, 11, then an epoch 5 overwrite comes, I suppose node1 should be the better choice but we'll chose node2 according to the epoch distance?

yes, that is true. Without a high epoch in the mbr, there is no perfect heuristic

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

jolivier23 · 2019-07-11T07:20:54Z

Thinking about it and looking at the structs, if we add an upper bound the to mbr, it takes the size of the evt_node without entries to 64 bytes from 56...this means better alignment for entries so may be worth doing to get a more accurate split and insertion heuristic.

daosbuild1 · 2019-07-11T08:57:14Z

Test stage Functional completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-736/7/execution/node/546/log

jolivier23 · 2019-07-11T14:40:22Z

Thinking about it and looking at the structs, if we add an upper bound the to mbr, it takes the size of the evt_node without entries to 64 bytes from 56...this means better alignment for entries so may be worth doing to get a more accurate split and insertion heuristic.

I'm gonna defer this change for now so I can fully concentrate on incarnation log...I'll file a ticket for it.

gnailzenh · 2019-07-12T14:24:57Z


 	if (ent_array.ea_ent_nr == 0)
-		return -DER_ENOENT;
+		return -DER_NONEXIST;


we should remove one of those error code...

jolivier23 commented Jul 8, 2019

View reviewed changes

jolivier23 requested review from Nasf-Fan, NiuYawei, gnailzenh and ryon-jensen July 8, 2019 23:56

Merge branch 'master' into jvolivie/fix_evtree

9dd6c01

NiuYawei reviewed Jul 10, 2019

View reviewed changes

jolivier23 added 3 commits July 10, 2019 07:50

Address review comment

60d1122

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

Merge branch 'master' into jvolivie/fix_evtree

e7e9e3a

Disable minor weight in favor of distance

cdb98a5

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

jolivier23 requested review from NiuYawei and rpadma2 July 10, 2019 22:04

NiuYawei reviewed Jul 11, 2019

View reviewed changes

jolivier23 added 2 commits July 11, 2019 00:34

Merge branch 'master' into jvolivie/fix_evtree

163bec0

Fix a bug from review

00e4767

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

jolivier23 requested a review from NiuYawei July 11, 2019 07:17

Merge branch 'master' into jvolivie/fix_evtree

be173cc

NiuYawei approved these changes Jul 12, 2019

View reviewed changes

gnailzenh approved these changes Jul 12, 2019

View reviewed changes

gnailzenh merged commit a46c552 into master Jul 12, 2019

gnailzenh deleted the jvolivie/fix_evtree branch July 12, 2019 14:33

Conversation

jolivier23 commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolivier23 commented Jul 9, 2019

Uh oh!

daosbuild1 commented Jul 9, 2019

Uh oh!

daosbuild1 commented Jul 9, 2019

Uh oh!

daosbuild1 commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NiuYawei commented Jul 10, 2019

Uh oh!

daosbuild1 commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolivier23 commented Jul 11, 2019

Uh oh!

daosbuild1 commented Jul 11, 2019

Uh oh!

jolivier23 commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants