Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenZFS 9438 - Holes can lose birth time info if a block has a mix of birth times #7746

Closed
wants to merge 2 commits into from

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Address remaining hole_birth issue, see #4996. This issue was originally mitigated in 9ea9e0b and 690fe64 by never sending holes with birth times. This PR pulls in the finalized fix from OpenZFS and adds a test case. The default behavior of never sending hole birth times has not been changed.

OpenZFS-issue: https://www.illumos.org/issues/9438
OpenZFS-commit: openzfs/openzfs@738e2a3c
External-issue: DLPX-46861

Description

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:

How Has This Been Tested?

Locally built, and test cases from @rincebrain were added. All rsend tests pass locally. Pending full test results from the CI.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

… 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
Follow up commit for OpenZFS 9438.  See the OpenZFS-issue link below
for a complete analysis.

Authored by: Matthew Ahrens <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://illumos.org/issues/9439
OpenZFS-commit: openzfs/openzfs@779220d
External-issue: DLPX-46861
Copy link
Contributor

@pcd1193182 pcd1193182 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My one question is about the test cases; in #5675, there were several test cases. Here, there is just one. It looks like you merged them all into one test case, is that right? If so, the diff looks good to me.

@behlendorf
Copy link
Contributor Author

@pcd1193182 yes, that's right. After looking at each test case individually I thought it made the most sense to simplify and condense them in to a single test. Of the four test cases provided, two were functional identical so I dropped one and kept all the others.

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #7746 into master will decrease coverage by 0.07%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7746      +/-   ##
==========================================
- Coverage   78.13%   78.06%   -0.08%     
==========================================
  Files         368      368              
  Lines      111986   112017      +31     
==========================================
- Hits        87501    87441      -60     
- Misses      24485    24576      +91
Flag Coverage Δ
#kernel 78.8% <97.36%> (ø) ⬆️
#user 66.75% <97.36%> (-0.34%) ⬇️

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 3a549dc...0f0a630. Read the comment docs.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) Reviewed labels Jul 27, 2018
behlendorf pushed a commit that referenced this pull request Jul 30, 2018
Follow up commit for OpenZFS 9438.  See the OpenZFS-issue link below
for a complete analysis.

Authored by: Matthew Ahrens <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Paul Dagnelie <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Brian Behlendorf <[email protected]>

OpenZFS-issue: https://illumos.org/issues/9439
OpenZFS-commit: openzfs/openzfs@779220d
External-issue: DLPX-46861
Closes #7746
@behlendorf behlendorf deleted the openzfs-9438 branch April 19, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants