fix dsl_scan_ds_clone_swapped logic #9163

avg-I · 2019-08-14T14:48:45Z

The was incorrect with respect to swapping dataset IDs both in the
on-disk ZAP object and the in-memory queue.

In both cases, if ds1 was already present, then it would be first
replaced with ds2 and then ds would be replaced back with ds1.
Also, both cases did not properly handle a situation where both ds1 and
ds2 are already queued. A duplicate insertion would be attempted and
its failure would result in a panic.

This change should fix #9140.

Signed-off-by: Andriy Gapon avg@FreeBSD.org

avg-I · 2019-08-14T15:43:24Z

Note that I was not sure if mintxg1 and mintxg2 are required to be equal (as they are for the ZAP).
If that's the case, then the code could be made simpler.

avg-I · 2019-08-14T15:44:02Z

CC @tcaputi

tcaputi · 2019-08-14T15:52:27Z

The first part of the fix (dealing with scan_ds_queue_contains()) looks good to me. For the second part, don't we need similar handling for when both the clone and origin are in the zap?

avg-I · 2019-08-14T15:56:17Z

I think that that is completly handled in the first branch. It handles two sub-cases: (a) only ds1 is in the ZAP; (b) both ds1 and ds2 are in the ZAP. So, the second branch needs to deal only with the case where only ds2 is in the ZAP.

tcaputi · 2019-08-14T15:59:57Z

Ah, I see. I think it would be good to break that up similarly to how you did with scan_ds_queue_contains(). This way its easy to see that these code paths are doing very similar things.

avg-I · 2019-08-14T16:35:01Z

Okay, I will do that!
Just want to clarify first if mintxg-s can ever be different between the origin and the clone or if they must always be the same.
Thanks!

tcaputi · 2019-08-16T17:33:13Z

I'm not sure about the mintxgs. I would assume they can be different.

avg-I · 2019-08-17T09:30:12Z

It's just that the code dealing with the ZAP asserts that ds1's mintxg is equal to ds1's and ds2's ds_prev_snap_txg (and ds_prev_snap_txg -s must obviously be equal to each other).
And also the same is asserted for ds2's mintxg.

So, I guessed that mintxg-s recorded in the in-memory queue must obey the same rule.
Maybe @ahrens can help us here.

ahrens · 2019-08-21T16:50:00Z

@avg-I That's right, since we are doing a clone-swap, ds1 and ds2 have the same prev snap, and their mintxgs in the on-disk queue (scn_queue_obj) should be the same as that prev snap's birth. I'm not as familiar with the in-memory queue.

module/zfs/dsl_scan.c

ahrens · 2019-08-26T17:55:12Z

module/zfs/dsl_scan.c

@@ -2184,52 +2184,57 @@ dsl_scan_ds_clone_swapped(dsl_dataset_t *ds1, dsl_dataset_t *ds2, dmu_tx_t *tx)
 {


I think this could use some comments explaining that we need to apply this transformation to both the in-memory and on-disk queues, and since the state of the in-memory and on-disk queues might be different, we need to apply the transformation independently.

I agree. But I would prefer to let someone who is more familiar with the code do that.

@ahrens, I am just not entirely sure if the in-memory and on-disk queues can indeed be different in this respect.
If they can be different, then I need to understand why / how.
I need some help with writing the requested comment.
Maybe @tcaputi could provide it.

ahrens · 2019-08-26T17:58:40Z

module/zfs/dsl_scan.c

-		ASSERT3U(mintxg, ==, dsl_dataset_phys(ds2)->ds_prev_snap_txg);
-		VERIFY3U(0, ==, zap_remove_int(dp->dp_meta_objset,
+	if (ds1_queued && ds2_queued) {
+		/* Nothing to do if both are queued (or both are not queued). */


I think we can use the same algorithm for the on-disk and in-memory queues. I.e. make both of them:

if (ds1_queued) { remove ds1, add ds2 } if (ds2_queued) { remove ds2, add ds1 }

or make both of them

if (ds1_queued && ds2_queued) { // nothing } else if (ds1_queued) { remove ds1, add ds2 } else if (ds2_queued) { remove ds2, add ds1 }

To me, the first way is easier to understand, but either works.

The only reason I* structured them differently is that I was not sure if mintxg values obey the same rules for both queues. I will change them both to the second form (because of the issue with the first form).

Sounds good. It would be nice to augment that comment explaining why the "both queued" case is needed (so that we don't try to add something that's already there).

@ahrens, I am not sure how to fit the requested comment into the new control flow.
I mean that removing "both queued" case would not only result in the duplicate insertion, but it would also result in obviously incorrect logic where we swap just one ds but not the other (because of else-if).
In other words, I am not sure how to word a comment that would explain why the code does not do an obviously wrong thing.

codecov · 2019-08-30T02:30:27Z

Codecov Report

Merging #9163 into master will decrease coverage by 0.18%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #9163      +/-   ##
==========================================
- Coverage   79.22%   79.04%   -0.19%     
==========================================
  Files         400      400              
  Lines      122041   122054      +13     
==========================================
- Hits        96687    96472     -215     
- Misses      25354    25582     +228

Flag	Coverage Δ
#kernel	`79.72% <0%> (-0.1%)`	⬇️
#user	`66.66% <0%> (-0.27%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a49dbbb...7b8781a. Read the comment docs.

The was incorrect with respect to swapping dataset IDs both in the on-disk ZAP object and the in-memory queue. In both cases, if ds1 was already present, then it would be first replaced with ds2 and then ds would be replaced back with ds1. Also, both cases did not properly handle a situation where both ds1 and ds2 are already queued. A duplicate insertion would be attempted and its failure would result in a panic. This change should fix openzfs#9140. Signed-off-by: Andriy Gapon <avg@FreeBSD.org>

behlendorf · 2019-09-11T00:11:07Z

@tcaputi would you mind reviewing this.

tcaputi

LGTM if you can explain (and possibly comment in the code) as to how those sanity checks work.

tcaputi · 2019-09-11T14:46:18Z

module/zfs/dsl_scan.c

+	if (ds2_queued) {
+		ASSERT3U(mintxg2, ==, dsl_dataset_phys(ds1)->ds_prev_snap_txg);
+		ASSERT3U(mintxg2, ==, dsl_dataset_phys(ds2)->ds_prev_snap_txg);
+	}


Can you explain the sanity check here (and below?). If both are queued this looks impossible to me unless mintxg1 == mintxg2.

ds1 and ds2 have the same ds_prev (see assertion in the beginning of dsl_dataset_clone_swap_sync_impl()). So if mintxgX is the prev snap txg (as asserted here), then mintxg1==mintxg2.

Yes, I think that all mintxg-s must be equal and the asserts are implicitly asserting that.

tcaputi · 2019-09-11T14:48:54Z

module/zfs/dsl_scan.c

@@ -2184,52 +2184,75 @@ dsl_scan_ds_clone_swapped(dsl_dataset_t *ds1, dsl_dataset_t *ds2, dmu_tx_t *tx)
 {


while we're here can you change the comment above the function to say origin instead of parent?

I think that a simple s/parent/origin/ would not produce a good result.
Also, it seems that there is such a term as "clone parent-child dependency relationship", so "parent" is not completely out of place. And an unqualified "origin" seems to mean an origin snapshot.
Anyway, "parent" is more ambiguous than "origin" in this context, so I'll try to improve that comment.

ahrens

I'd still like to see the comments I mentioned added, but I'm OK with the change as-is.

behlendorf · 2019-09-12T20:45:05Z

@avg-I can I twist your arm to add the requested additional comments. Then we can get this integrated.

avg-I · 2019-09-14T20:57:25Z

@behlendorf , I can add the comments requested by Matt, but I genuinely cannot understand what they should say. (And in one case why it's needed at all).
So, if someone else can write them or can help me write them, then I can add them.

ahrens

I added some specific suggestions for the comments.

ahrens · 2019-09-16T22:16:34Z

module/zfs/dsl_scan.c

+	}
+
+	if (ds1_queued && ds2_queued) {
+		/* Nothing to do if both are queued (or both are not queued). */


How about changing this comment to:
If both are queued, we don't need to do anything. The swapping code below would not handle this case correctly, since we can't insert ds2 if it is already there.

Thank you for the suggestion! I will take it.
At the same time it's not the only problem with the hypothetical alternative mentioned in the comment. It would attempt to replace ds1 with ds2 but not in the opposite direction because of else if. And without the else if there would be another problem: replacing ds1 with ds2 and then ds2 back with ds1.
So, in my opinion, even if the queue silently ignored duplicate inserts, it would be very hard (if possible) to handle the case where both ds1 and ds2 are queued without explicitly handling that case in one way or another.
E.g., the proposed code shows one way; the existing code for the on-disk queue shows another: having the enqueue operation return an error code, checking for an error, and if the error is EEXIST, then backing out the original removal. Which is more work than just doing nothing in the first place.

ahrens · 2019-09-16T22:21:40Z

module/zfs/dsl_scan.c

-		ASSERT3U(mintxg, ==, dsl_dataset_phys(ds2)->ds_prev_snap_txg);
-		VERIFY3U(0, ==, zap_remove_int(dp->dp_meta_objset,
+	/*
+	 * Handle the on-disk scan queue.


How about adding this to the comment:
The on-disk state is an out-of-date version of the in-memory state, so the in-memory and on-disk values for ds1_queuedandds2_queued may be different. Therefore we need to apply the swap logic to the on-disk state independently of the in-memory state.

Signed-off-by: Andriy Gapon <avg@FreeBSD.org>

The was incorrect with respect to swapping dataset IDs both in the on-disk ZAP object and the in-memory queue. In both cases, if ds1 was already present, then it would be first replaced with ds2 and then ds would be replaced back with ds1. Also, both cases did not properly handle a situation where both ds1 and ds2 are already queued. A duplicate insertion would be attempted and its failure would result in a panic. Reviewed-by: Matt Ahrens <matt@delphix.com> Reviewed-by: Tom Caputi <tcaputi@datto.com> Signed-off-by: Andriy Gapon <avg@FreeBSD.org> Closes openzfs#9140 Closes openzfs#9163

The was incorrect with respect to swapping dataset IDs both in the on-disk ZAP object and the in-memory queue. In both cases, if ds1 was already present, then it would be first replaced with ds2 and then ds would be replaced back with ds1. Also, both cases did not properly handle a situation where both ds1 and ds2 are already queued. A duplicate insertion would be attempted and its failure would result in a panic. Reviewed-by: Matt Ahrens <matt@delphix.com> Reviewed-by: Tom Caputi <tcaputi@datto.com> Signed-off-by: Andriy Gapon <avg@FreeBSD.org> Closes #9140 Closes #9163

behlendorf requested a review from tcaputi August 15, 2019 23:28

behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 15, 2019

avg-I force-pushed the 9140-dsl_scan_ds_clone_swapped branch from a0978af to 2f66a09 Compare August 26, 2019 13:11

ahrens reviewed Aug 26, 2019

View reviewed changes

ahrens added the Type: Defect Incorrect behavior (e.g. crash, hang) label Aug 26, 2019

avg-I force-pushed the 9140-dsl_scan_ds_clone_swapped branch from 2f66a09 to 5bd8f78 Compare August 29, 2019 09:52

avg-I force-pushed the 9140-dsl_scan_ds_clone_swapped branch from 5bd8f78 to 1501c95 Compare September 4, 2019 07:34

behlendorf requested a review from ahrens September 11, 2019 00:10

tcaputi approved these changes Sep 11, 2019

View reviewed changes

ahrens approved these changes Sep 11, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 12, 2019

ahrens approved these changes Sep 16, 2019

View reviewed changes

update / expand comments

7b8781a

Signed-off-by: Andriy Gapon <avg@FreeBSD.org>

behlendorf merged commit dd262c9 into openzfs:master Sep 18, 2019

avg-I deleted the 9140-dsl_scan_ds_clone_swapped branch September 19, 2019 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix dsl_scan_ds_clone_swapped logic #9163

fix dsl_scan_ds_clone_swapped logic #9163

avg-I commented Aug 14, 2019

avg-I commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 16, 2019

avg-I commented Aug 17, 2019

ahrens commented Aug 21, 2019

ahrens Aug 26, 2019

avg-I Aug 26, 2019

avg-I Sep 14, 2019

ahrens Aug 26, 2019

avg-I Aug 26, 2019

ahrens Aug 26, 2019

avg-I Sep 14, 2019

codecov bot commented Aug 30, 2019 •

edited

Loading

behlendorf commented Sep 11, 2019

tcaputi left a comment

tcaputi Sep 11, 2019

ahrens Sep 11, 2019

avg-I Sep 14, 2019

tcaputi Sep 11, 2019

avg-I Sep 14, 2019

ahrens left a comment

behlendorf commented Sep 12, 2019

avg-I commented Sep 14, 2019

ahrens left a comment

ahrens Sep 16, 2019

avg-I Sep 17, 2019

ahrens Sep 16, 2019

avg-I Sep 17, 2019

		@@ -2184,52 +2184,57 @@ dsl_scan_ds_clone_swapped(dsl_dataset_t ds1, dsl_dataset_t ds2, dmu_tx_t *tx)
		{

		@@ -2184,52 +2184,75 @@ dsl_scan_ds_clone_swapped(dsl_dataset_t ds1, dsl_dataset_t ds2, dmu_tx_t *tx)
		{

fix dsl_scan_ds_clone_swapped logic #9163

fix dsl_scan_ds_clone_swapped logic #9163

Conversation

avg-I commented Aug 14, 2019

avg-I commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 14, 2019

avg-I commented Aug 14, 2019

tcaputi commented Aug 16, 2019

avg-I commented Aug 17, 2019

ahrens commented Aug 21, 2019

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

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

behlendorf commented Sep 11, 2019

tcaputi 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

ahrens left a comment

Choose a reason for hiding this comment

behlendorf commented Sep 12, 2019

avg-I commented Sep 14, 2019

ahrens 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

codecov bot commented Aug 30, 2019 •

edited

Loading