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

Remove unused check from dmu_tx_count_write() #11384

Merged
merged 1 commit into from
Dec 22, 2020

Conversation

behlendorf
Copy link
Contributor

Motivation and Context

Resolve issue #3731.

Description

Individual transactions may not be larger than DMU_MAX_ACCESS.
This is enforced by the assertions in dmu_tx_hold_write() and
dmu_tx_hold_write_by_dnode(). There's an additional check in
dmu_tx_count_write() however it has no effect and only sets a
local err variable. We could enable this check, however since
it's already enforced by ASSERTs elsewhere I opted to remove it
instead.

How Has This Been Tested?

Locally compiled.

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:

Individual transactions may not be larger than DMU_MAX_ACCESS.
This is enforced by the assertions in dmu_tx_hold_write() and
dmu_tx_hold_write_by_dnode().  There's an additional check in
dmu_tx_count_write() however it has no effect and only sets a
local err variable.  We could enable this check, however since
it's already enforced by ASSERTs elsewhere I opted to remove it
instead.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 21, 2020
@ahrens
Copy link
Member

ahrens commented Dec 21, 2020

If I understand correctly this resolves #3731 because the it's saying that this is dead code, not that write() actually returns EFBIG (which is not true). I'm going to change its synopsis to reflect that.

I take it that since DMU_MAX_ACCESS should be enforced by calling code, we are OK with failing the ASSERT in the case of a bug in the calling (ZPL) code. Or on production builds, with driving on and probably everything will work even with larger writes (unless the pool totally runs out of space).

@behlendorf
Copy link
Contributor Author

That's right. My thought was that it's sufficient to rely on the calling code and existing ASSERTs to enforce this. Since it's effectively been this way for years now this wouldn't change the production build behavior at all. That said, I'm not at all against returning EFBIG here instead if that's preferred. It does seem to be what the original code intended and it would be another safety check. It would just be an unused code path for all existing callers.

@behlendorf behlendorf merged commit 0c763f7 into openzfs:master Dec 22, 2020
ghost pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Dec 23, 2020
Individual transactions may not be larger than DMU_MAX_ACCESS.
This is enforced by the assertions in dmu_tx_hold_write() and
dmu_tx_hold_write_by_dnode().  There's an additional check in
dmu_tx_count_write() however it has no effect and only sets a
local err variable.  We could enable this check, however since
it's already enforced by ASSERTs elsewhere I opted to remove it
instead.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#3731 
Closes openzfs#11384
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
Individual transactions may not be larger than DMU_MAX_ACCESS.
This is enforced by the assertions in dmu_tx_hold_write() and
dmu_tx_hold_write_by_dnode().  There's an additional check in
dmu_tx_count_write() however it has no effect and only sets a
local err variable.  We could enable this check, however since
it's already enforced by ASSERTs elsewhere I opted to remove it
instead.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#3731 
Closes openzfs#11384
@behlendorf behlendorf deleted the issue-3731 branch April 19, 2021 19:20
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
Individual transactions may not be larger than DMU_MAX_ACCESS.
This is enforced by the assertions in dmu_tx_hold_write() and
dmu_tx_hold_write_by_dnode().  There's an additional check in
dmu_tx_count_write() however it has no effect and only sets a
local err variable.  We could enable this check, however since
it's already enforced by ASSERTs elsewhere I opted to remove it
instead.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes openzfs#3731 
Closes openzfs#11384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants