RFC/WIP: Revert Consolidate arc_buf allocation checks #12227
Merged
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 revert should be safe and I think doing so would be a good idea. This isn't a change we need, and nothing has really fundamentally changed in the way this area of the code was intended to work. The only wrinkle with reverting this you've already addressed and that was the zstd conflict. Thanks for keeping those patches separate at least for the moment since it made it easier to verify it was a clean revert. The minor cstyle error will need to be sorted but aside from that this functionally looks good. |
behlendorf
added
Status: Revision Needed
behlendorf
approved these changes
Jun 14, 2021
|
Great! Do you want me to squash these, or just add more discussion in the commits (and rebase)? |
|
@aerusso why don't we keep them split up for now. Can you just update your top commit to resolve the csyle warning it introduced and update the PR. I'll see about rounding up some more reviewers. When we merge this I can squash everything. |
In preparation for reverting 13fac09, we partially revert 10b3c7f, within dbuf_alloc_arcbuf_from_arcbuf, which otherwise would conflict. Signed-off-by: Antonio Russo <[email protected]>
This reverts commit 13fac09. Per the discussion in openzfs#11531, the reverted commit---which intended only to be a cleanup commit---introduced a subtle, unintended change in behavior. Suggested-by: @chrisrd Suggested-by: [email protected] Signed-off-by: Antonio Russo <[email protected]>
aerusso
force-pushed
the
pulls/revert-consolidate-arcbuf-2
branch
from
eba0e14 to
a87bc85
Compare
This commit reverts the partial reversion of 10b3c7f, transposed to account for the un-consolidation of dbuf_alloc_arcbuf_from_arcbuf. Signed-off-by: Antonio Russo <[email protected]>
aerusso
force-pushed
the
pulls/revert-consolidate-arcbuf-2
branch
from
a87bc85 to
ad96131
Compare
behlendorf
added
Status: Accepted
behlendorf
pushed a commit
that referenced
this pull request
Jun 24, 2021
This reverts commit 13fac09. Per the discussion in #11531, the reverted commit---which intended only to be a cleanup commit---introduced a subtle, unintended change in behavior. Care was taken to partially revert and then reapply 10b3c7f which would otherwise have caused a conflict. These changes were squashed in to this commit. Reviewed-by: Brian Behlendorf <[email protected]> Suggested-by: @chrisrd Suggested-by: [email protected] Signed-off-by: Antonio Russo <[email protected]> Closes #11531 Closes #12227
aerusso
added a commit
to aerusso/zfs
that referenced
this pull request
Jul 10, 2021
This reverts commit 13fac09. Per the discussion in openzfs#11531, the reverted commit---which intended only to be a cleanup commit---introduced a subtle, unintended change in behavior. Care was taken to partially revert and then reapply 10b3c7f which would otherwise have caused a conflict. These changes were squashed in to this commit. Reviewed-by: Brian Behlendorf <[email protected]> Suggested-by: @chrisrd Suggested-by: [email protected] Signed-off-by: Antonio Russo <[email protected]> Closes openzfs#11531 Closes openzfs#12227
tonyhutter
pushed a commit
that referenced
this pull request
Aug 31, 2021
This reverts commit 13fac09. Per the discussion in #11531, the reverted commit---which intended only to be a cleanup commit---introduced a subtle, unintended change in behavior. Care was taken to partially revert and then reapply 10b3c7f which would otherwise have caused a conflict. These changes were squashed in to this commit. Reviewed-by: Brian Behlendorf <[email protected]> Suggested-by: @chrisrd Suggested-by: [email protected] Signed-off-by: Antonio Russo <[email protected]> Closes #11531 Closes #12227
tonyhutter
pushed a commit
that referenced
this pull request
Sep 22, 2021
This reverts commit 13fac09. Per the discussion in #11531, the reverted commit---which intended only to be a cleanup commit---introduced a subtle, unintended change in behavior. Care was taken to partially revert and then reapply 10b3c7f which would otherwise have caused a conflict. These changes were squashed in to this commit. Reviewed-by: Brian Behlendorf <[email protected]> Suggested-by: @chrisrd Suggested-by: [email protected] Signed-off-by: Antonio Russo <[email protected]> Closes #11531 Closes #12227
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.
Motivation and Context
The discussion in #11531 suggests that 13fac09 may be implicated in that bug.
I would like comments on the safety of revert the aforementioned commit.
Description
Because 10b3c7f also touches the same code, this patch set carefully reverts 10b3c7f (limited to
module/zfs/dbuf.c), then actually reverts 13fac09, and then finally reverts the first commit.How Has This Been Tested?
It passes the ZTS when rebased on 2.0.4 and the MR for 2.0.5.
Types of changes
Checklist:
Signed-off-by.