Increase allowed 'special_small_blocks' maximum value #9355

behlendorf · 2019-09-24T21:22:51Z

Motivation and Context

Description

There may be circumstances where it's desirable that all blocks
in a specified dataset be stored on the special device. Relax
the artificial 128K limit and allow the special_small_blocks
property to be set up to 1M. When blocks >1MB have been enabled
via the zfs_max_recordsize module option, this limit is increased
accordingly.

How Has This Been Tested?

Verified that zfs set special_small_blocks=1M works correctly when
the allocation classes feature is enabled. Additionally, verified the limit
can be increased up to 16M when zfs_max_recordsize in increased.

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.

The difference between the sizes could be positive or negative. Leaving the types as unsigned means the result overflows when the difference is negative and removing the labs() means we'll have introduced a bug. The subtraction results in the correct value when the unsigned integer is interpreted as a signed integer by labs(). Clang doesn't see that we're doing a subtraction and abusing the types. It sees the result of the subtraction, an unsigned value, being passed to an absolute value function and emits a warning which we treat as an error. Reviewed by: Youzhong Yang <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#9355

PrivatePuffin

This PR could use some review love, it's simple/clear and should be relatively quick to merge and review.

PrivatePuffin · 2019-11-13T08:25:54Z

Test failures seem to me to be totally unrelated to this PR... But someone else could be beter suited to review those.

lib/libzfs/libzfs_dataset.c

There may be circumstances where it's desirable that all blocks in a specified dataset be stored on the special device. Relax the artificial 128K limit and allow the special_small_blocks property to be set up to 1M. When blocks >1MB have been enabled via the zfs_max_recordsize module option, this limit is increased accordingly. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#9131

codecov · 2019-11-19T09:39:04Z

Codecov Report

Merging #9355 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9355      +/-   ##
==========================================
- Coverage   79.11%   79.04%   -0.07%     
==========================================
  Files         419      419              
  Lines      123704   123708       +4     
==========================================
- Hits        97870    97789      -81     
- Misses      25834    25919      +85

Flag	Coverage Δ
#kernel	`79.7% <ø> (-0.03%)`	⬇️
#user	`66.63% <100%> (-0.3%)`	⬇️

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 8221bcf...3ef695c. Read the comment docs.

The difference between the sizes could be positive or negative. Leaving the types as unsigned means the result overflows when the difference is negative and removing the labs() means we'll have introduced a bug. The subtraction results in the correct value when the unsigned integer is interpreted as a signed integer by labs(). Clang doesn't see that we're doing a subtraction and abusing the types. It sees the result of the subtraction, an unsigned value, being passed to an absolute value function and emits a warning which we treat as an error. Reviewed by: Youzhong Yang <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#9355

There may be circumstances where it's desirable that all blocks in a specified dataset be stored on the special device. Relax the artificial 128K limit and allow the special_small_blocks property to be set up to 1M. When blocks >1MB have been enabled via the zfs_max_recordsize module option, this limit is increased accordingly. Reviewed-by: Don Brady <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#9131 Closes openzfs#9355

The difference between the sizes could be positive or negative. Leaving the types as unsigned means the result overflows when the difference is negative and removing the labs() means we'll have introduced a bug. The subtraction results in the correct value when the unsigned integer is interpreted as a signed integer by labs(). Clang doesn't see that we're doing a subtraction and abusing the types. It sees the result of the subtraction, an unsigned value, being passed to an absolute value function and emits a warning which we treat as an error. Reviewed by: Youzhong Yang <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes openzfs#9355

There may be circumstances where it's desirable that all blocks in a specified dataset be stored on the special device. Relax the artificial 128K limit and allow the special_small_blocks property to be set up to 1M. When blocks >1MB have been enabled via the zfs_max_recordsize module option, this limit is increased accordingly. Reviewed-by: Don Brady <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes openzfs#9131 Closes openzfs#9355

The difference between the sizes could be positive or negative. Leaving the types as unsigned means the result overflows when the difference is negative and removing the labs() means we'll have introduced a bug. The subtraction results in the correct value when the unsigned integer is interpreted as a signed integer by labs(). Clang doesn't see that we're doing a subtraction and abusing the types. It sees the result of the subtraction, an unsigned value, being passed to an absolute value function and emits a warning which we treat as an error. Reviewed by: Youzhong Yang <[email protected]> Reviewed-by: Igor Kozhukhov <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ryan Moeller <[email protected]> Closes #9355

There may be circumstances where it's desirable that all blocks in a specified dataset be stored on the special device. Relax the artificial 128K limit and allow the special_small_blocks property to be set up to 1M. When blocks >1MB have been enabled via the zfs_max_recordsize module option, this limit is increased accordingly. Reviewed-by: Don Brady <[email protected]> Reviewed-by: Kjeld Schouten <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Closes #9131 Closes #9355

In ZFS 0.8.3, the limit was increased to 1M [0][1]. [0] https://github.com/openzfs/zfs/releases/tag/zfs-0.8.3 [1] openzfs/zfs#9355 Signed-off-by: Fiona Ebner <[email protected]>

behlendorf added the Status: Code Review Needed Ready for review and testing label Sep 24, 2019

behlendorf requested a review from don-brady September 24, 2019 21:22

behlendorf mentioned this pull request Sep 24, 2019

Option to set special_small_blocks higher than 128k - why not allow it to be higher? #9131

Closed

behlendorf force-pushed the issue-9131 branch from afad256 to 7e0a502 Compare September 25, 2019 18:55

behlendorf force-pushed the issue-9131 branch from 7e0a502 to 254d15a Compare November 11, 2019 20:41

PrivatePuffin approved these changes Nov 13, 2019

View reviewed changes

don-brady reviewed Nov 13, 2019

View reviewed changes

lib/libzfs/libzfs_dataset.c Show resolved Hide resolved

lib/libzfs/libzfs_dataset.c Show resolved Hide resolved

behlendorf force-pushed the issue-9131 branch from 254d15a to 3ef695c Compare November 19, 2019 01:05

don-brady approved these changes Dec 2, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 2, 2019

behlendorf merged commit 624222a into openzfs:master Dec 3, 2019

behlendorf deleted the issue-9131 branch April 19, 2021 19:24

rincebrain mentioned this pull request Aug 6, 2024

Allow special_small_blocks to exceed 128K (recommend 1M) #16417

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase allowed 'special_small_blocks' maximum value #9355

Increase allowed 'special_small_blocks' maximum value #9355

behlendorf commented Sep 24, 2019

PrivatePuffin left a comment •

edited

Loading

PrivatePuffin commented Nov 13, 2019

codecov bot commented Nov 19, 2019 •

edited

Loading

Increase allowed 'special_small_blocks' maximum value #9355

Increase allowed 'special_small_blocks' maximum value #9355

Conversation

behlendorf commented Sep 24, 2019

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

PrivatePuffin left a comment • edited Loading

Choose a reason for hiding this comment

PrivatePuffin commented Nov 13, 2019

codecov bot commented Nov 19, 2019 • edited Loading

Codecov Report

PrivatePuffin left a comment •

edited

Loading

codecov bot commented Nov 19, 2019 •

edited

Loading