Avoid extra memory copies during mirror scrub. #13593
Closed
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
When several mirror scrub reads complete successfully, we do not need to copy data from each of them into the parent ZIO buffer, the first one is enough. This also moves the memory copy out of the ZIO locks, that I expected to cause contention, though on my system they almost didn't, had incorrect lock order and was partially not needed. When scrubbing 4-way mirror with 1.5TB of 128KB ZVOL blocks, this reduces CPU time spent in memcpy() by 75% and total CPU time by 31%. Signed-off-by: Alexander Motin <[email protected]> Sponsored-By: iXsystems, Inc.
amotin
added
Type: Performance
behlendorf
added
Status: Accepted
mmaybee
reviewed
Jun 28, 2022
| while ((pio = zio_walk_parents(zio, &zl)) != NULL) { | ||
| mutex_enter(&pio->io_lock); | ||
| ASSERT3U(zio->io_size, >=, pio->io_size); | ||
| zio_t *pio = zio_unique_parent(zio); |
There was a problem hiding this comment.
The original algorithm walked all parents to do the copy... while it seems reasonable that there should generally be only a single parent here, do you have a reason to believe that is an invariant? Why did the original algorithm assume the possibility of multiple parents?
There was a problem hiding this comment.
I have no idea what the original author was thinking about. zio_unique_parent() actually verifies there are not other parents, and the assertion didn't fire on my tests. My best guess is that it was some paranoia, same as unneeded locking.
This was referenced Jun 29, 2022
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.
When several mirror scrub reads complete successfully, we do not need
to copy data from each of them into the parent ZIO buffer, the first
one is enough. This also moves the memory copy out of the ZIO locks,
that I expected to cause contention, though on my system they almost
didn't, had incorrect lock order and was partially not needed.
When scrubbing 4-way mirror with 1.5TB of 128KB ZVOL blocks, this
reduces CPU time spent in memcpy() by 75% and total CPU time by 31%.
Types of changes
Checklist:
Signed-off-by.