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

Default to zfs_dmu_offset_next_sync=1 #12746

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Enable strict hole reporting by default to avoid the unclear
(but functionally correct) behavior of only reporting holes
in files which were not recently dirtied.

Description

Strict hole reporting was previously disabled by default as a
performance optimization. However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS. Change the default behavior to
always report holes and force the TXG sync.

How Has This Been Tested?

This behavior has also been tested in the context of issue #11900.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 10, 2021
@sempervictus
Copy link
Contributor

@behlendorf - any thoughts on what the performance impact would be and under which conditions its most likely to be felt?

@behlendorf
Copy link
Contributor Author

@sempervictus this will be felt most by any workload which writes a new file then immediately (within a few seconds) uses SEEK_HOLE/SEEK_DATA to determine where the holes/data are in that file. That first lseek() will incur the additional latency of a full pool sync but subsequent lseek() calls will not (unless that file is written to again). Files which haven't been recently modified won't see any change in behavior. This should be less of an issue for SSD/NVMe based pools with faster sync times, and more of an issues for large HDD pools with a lot of uncommitted data in the writeback cache.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 30, 2021
@behlendorf behlendorf merged commit 05b3eb6 into openzfs:master Nov 30, 2021
nabijaczleweli pushed a commit to nabijaczleweli/zfs that referenced this pull request Mar 29, 2022
Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Upstream-commit: 05b3eb6
Closes openzfs#12746
nabijaczleweli pushed a commit to nabijaczleweli/zfs that referenced this pull request Mar 29, 2022
Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Upstream-commit: 05b3eb6
Ref: openzfs#13261
Closes openzfs#12746
behlendorf added a commit that referenced this pull request Apr 1, 2022
Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Upstream-commit: 05b3eb6
Ref: #13261
Closes #12746
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#12746
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
Strict hole reporting was previously disabled by default as a
performance optimization.  However, this has lead to confusion
over the expected behavior and a variety of workarounds being
adopted by consumers of ZFS.  Change the default behavior to
always report holes and force the TXG sync.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#12746
johnalotoski added a commit to input-output-hk/bitte-world that referenced this pull request Dec 9, 2022
* Setting /sys/module/zfs/parameters/zfs_dmu_offset_next_sync to 1
  instead of overlay utilization
* Ref: openzfs/zfs/pull/12746

This reverts commit 02462d5.
johnalotoski added a commit to input-output-hk/bitte-world that referenced this pull request Dec 10, 2022
johnalotoski added a commit to input-output-hk/ci-world that referenced this pull request Dec 10, 2022
gmelikov pushed a commit to openzfs/openzfs-docs that referenced this pull request Feb 20, 2023
@shodanshok
Copy link
Contributor

@behlendorf just to be sure: is it safe to use the old default setting zfs_dmu_offset_next_sync=0 ? If I understand it correctly, the only possible drawback is to read some zeroes regions rather than skipping them via SEEK_HOLE, right?

@behlendorf
Copy link
Contributor Author

@shodanshok yes that's right.

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.

6 participants