vdev_open_child() race on vdev_parent->vdev_nonrot update? #5162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is clearer. LGTM. |
inkdot7
force-pushed
the
vdev_open_children_nonrot
branch
3 times, most recently
from
b0fd8c8 to
7c0f2e3
Compare
behlendorf
reviewed
Oct 3, 2016
There was a problem hiding this comment.
You're absolutely right about this race. In order to resolve the conflict with #4637 I've updated the patch and applied it on top of the other proposed change. Since you're familiar with this code it would be great if you could review and test both fixes so they could be finalized and merged.
| @@ -1124,7 +1124,7 @@ vdev_open_child(void *arg) | |||
| vd->vdev_open_thread = curthread; | |||
| vd->vdev_open_error = vdev_open(vd); | |||
| vd->vdev_open_thread = NULL; | |||
| vd->vdev_parent->vdev_nonrot &= vd->vdev_nonrot; | |||
| /* vd->vdev_parent->vdev_nonrot is updated in vdev_open_children() */ | |||
There was a problem hiding this comment.
This should just be removed.
There was a problem hiding this comment.
I updated this patch, such that it could be applied if the CentOS issue of #4637 takes time to resolve. Reason being that condensing the nonrot logic to the end of vdev_open_children() simplifies some other work.
Updating vd->vdev_parent->vdev_nonrot in vdev_open_child() is a race when vdev_open_child is called for many children from a task queue. vdev_open_child() is only called by vdev_open_children(), let the latter update the parent vdev_nonrot member. The update was already there, so done twice previously. Thus using the same logic at the end in vdev_open_children() to update vdev_nonrot, either we are vdev_uses_zvols() or not.
inkdot7
force-pushed
the
vdev_open_children_nonrot
branch
from
7c0f2e3 to
70c35e9
Compare
behlendorf
approved these changes
Oct 6, 2016
DeHackEd
pushed a commit
to DeHackEd/zfs
that referenced
this pull request
Oct 19, 2016
Updating vd->vdev_parent->vdev_nonrot in vdev_open_child() is a race when vdev_open_child is called for many children from a task queue. vdev_open_child() is only called by vdev_open_children(), let the latter update the parent vdev_nonrot member. The update was already there, so done twice previously. Thus using the same logic at the end in vdev_open_children() to update vdev_nonrot, either we are vdev_uses_zvols() or not. Reviewed-by: Richard Elling <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Haakan T Johansson <[email protected]> Closes openzfs#5162
DeHackEd
pushed a commit
to DeHackEd/zfs
that referenced
this pull request
Oct 29, 2016
Updating vd->vdev_parent->vdev_nonrot in vdev_open_child() is a race when vdev_open_child is called for many children from a task queue. vdev_open_child() is only called by vdev_open_children(), let the latter update the parent vdev_nonrot member. The update was already there, so done twice previously. Thus using the same logic at the end in vdev_open_children() to update vdev_nonrot, either we are vdev_uses_zvols() or not. Reviewed-by: Richard Elling <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Haakan T Johansson <[email protected]> Closes openzfs#5162
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looking at the vdev_nonrot logic, I think I found a small race condition.
This pull request will conflict with #5117, I'll update either if any is merged. (But please comment on the open questions for #5117 meanwhile).