OpenZFS 7744 - Holes can lose birth time info if a block has a mix of… #5675
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
… birth times Reviewed by: George Wilson <[email protected]> Reviewed by: Matthew Ahrens <[email protected]> The problem here is that when you truncate and write a file in the same transaction group, the dbuf for the indirect block will be zeroed out to deal with the truncation, and then written for the write. During this process, we will lose hole birth time information for any holes in the range. This is partly fixed by: 6513 partially filled holes lose birth time which introduced dbuf_write_children_ready and would be the correct time to do this processing; however, in the case where a dnode is being freed, the old code includes necessary space accounting logic. We need a hybrid approach between determining whether the block should be converted to a higher-level hole in the zio pipeline, and doing it when the dnode is being synced out. Upstream bugs: DLPX-46861 Porting Notes: - This commit also adds tests for various hole_birth issues; these were compiled, corrected, and added by @rincebrain. These test cases should be pushed upstream to OpenZFS.
|
@behlendorf, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ahrens, @nedbass and @tuxoko to be potential reviewers. |
This was referenced Jan 27, 2017
prakashsurya
pushed a commit
to prakashsurya/openzfs
that referenced
this pull request
Jan 27, 2017
This is intended to be a follow on for the openzfs PR openzfs#273, and pulls in the new test cases added in the zfsonlinux PR #5675. I haven't built or tested this patch yet, so there's a good chance it's broken, but I wanted to open a PR so I can run this through the automated system. I generated this patch by configuring git to allow cherry-picking the zfsonlinx commit directly; i.e. $ git config merge.renameLimit 999999 $ git cherry-pick -X ours 29031d3660991b446c6fd7dfbf49c7c58ee211ae and then I had to manually fix up a few files due to differences in the build system for two repositories (e.g. remove Makefile.am, etc). Here's a link to the zfsonlinx PR: openzfs/zfs#5675
prakashsurya
pushed a commit
to prakashsurya/openzfs
that referenced
this pull request
Jan 27, 2017
This is intended to be a follow on for the openzfs PR openzfs#273, and pulls in the new test cases added in the zfsonlinux PR #5675. I haven't built or tested this patch yet, so there's a good chance it's broken, but I wanted to open a PR so I can run this through the automated system. I generated this patch by configuring git to allow cherry-picking the zfsonlinx commit directly; i.e. $ git config merge.renameLimit 999999 $ git cherry-pick -X ours 29031d3660991b446c6fd7dfbf49c7c58ee211ae and then I had to manually fix up a few files due to differences in the build system for two repositories (e.g. remove Makefile.am, etc). Here's a link to the zfsonlinx PR: openzfs/zfs#5675
|
@behlendorf Unless I'm blind, I think you didn't actually merge the tests from this into master. Was that intentional, and if so, why? |
behlendorf
pushed a commit
to behlendorf/zfs
that referenced
this pull request
Jul 26, 2018
… birth times As reported by openzfs#4996, there is yet another hole birth issue. In this one, if a block is entirely holes, but the birth times are not all the same, we lose that information by creating one hole with the current txg as its birth time. The ZoL PR's fix approach is incorrect. Ultimately, the problem here is that when you truncate and write a file in the same transaction group, the dbuf for the indirect block will be zeroed out to deal with the truncation, and then written for the write. During this process, we will lose hole birth time information for any holes in the range. In the case where a dnode is being freed, we need to determine whether the block should be converted to a higher-level hole in the zio pipeline, and if so do it when the dnode is being synced out. Porting Notes: * The DMU_OBJECT_END change in zfs_znode.c was already applied. * Added test cases from openzfs#5675 provided by @rincebrain for hole_birth issues. These test cases should be pushed upstream to OpenZFS. * Updated mk_files which is used by several rsend tests so the files created are a little more interesting and may contain holes. Authored by: Paul Dagnelie <[email protected]> Reviewed by: Matt Ahrens <[email protected]> Reviewed by: George Wilson <[email protected]> Approved by: Robert Mustacchi <[email protected]> Ported-by: Brian Behlendorf <[email protected]> OpenZFS-issue: https://www.illumos.org/issues/9438 OpenZFS-commit: openzfs/openzfs@738e2a3c External-issue: DLPX-46861
13 tasks
|
@rincebrain your not blind, in fact neither the original fix nor tests were merged. Instead a mitigation was put in place, 9ea9e0b, until the proper fix was finalized. That turned out to take a little longer than expected, but it's done now. I've opened #7746 with the complete fix from OpenZFS and added your test cases to it, with a few simplifications. I know it's been a while but if you have the time it would be great if you could review and test it. Thanks! |
behlendorf
pushed a commit
that referenced
this pull request
Jul 30, 2018
… birth times As reported by #4996, there is yet another hole birth issue. In this one, if a block is entirely holes, but the birth times are not all the same, we lose that information by creating one hole with the current txg as its birth time. The ZoL PR's fix approach is incorrect. Ultimately, the problem here is that when you truncate and write a file in the same transaction group, the dbuf for the indirect block will be zeroed out to deal with the truncation, and then written for the write. During this process, we will lose hole birth time information for any holes in the range. In the case where a dnode is being freed, we need to determine whether the block should be converted to a higher-level hole in the zio pipeline, and if so do it when the dnode is being synced out. Porting Notes: * The DMU_OBJECT_END change in zfs_znode.c was already applied. * Added test cases from #5675 provided by @rincebrain for hole_birth issues. These test cases should be pushed upstream to OpenZFS. * Updated mk_files which is used by several rsend tests so the files created are a little more interesting and may contain holes. Authored by: Paul Dagnelie <[email protected]> Reviewed by: Matt Ahrens <[email protected]> Reviewed by: George Wilson <[email protected]> Approved by: Robert Mustacchi <[email protected]> Ported-by: Brian Behlendorf <[email protected]> OpenZFS-issue: https://www.illumos.org/issues/9438 OpenZFS-commit: openzfs/openzfs@738e2a3c External-issue: DLPX-46861 Closes #7746
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.
Reviewed by: George Wilson [email protected]
Reviewed by: Matthew Ahrens [email protected]
The problem here is that when you truncate and write a file in the same
transaction group, the dbuf for the indirect block will be zeroed out to
deal with the truncation, and then written for the write.
During this process, we will lose hole birth time information for any
holes in the range. This is partly fixed by:
which introduced dbuf_write_children_ready and would be the correct
time to do this processing; however, in the case where a dnode is being
freed, the old code includes necessary space accounting logic. We need a
hybrid approach between determining whether the block should be
converted to a higher-level hole in the zio pipeline, and doing it when
the dnode is being synced out.
Upstream bugs: DLPX-46861
Porting Notes:
compiled, corrected, and added by @rincebrain. These test cases
should be pushed upstream to OpenZFS.