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.

Sign up for GitHub

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

Jump to bottom

Remove zl_issuer_lock from zil_suspend(). #15103

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Remove zl_issuer_lock from zil_suspend(). #15103

merged 1 commit into from
Jul 25, 2023

Conversation

amotin
Copy link
Member

@amotin amotin commented Jul 24, 2023

This locking was recently added as part of #14979. But appears it is illegal to take zl_issuer_lock while holding dp_config_rwlock, taken by dsl_pool_hold(). It causes deadlock with sync thread in spa_sync_upgrades(). On a second throught though, we should not need this locking, since zil_commit_impl() we call below takes zl_issuer_lock, that should sufficiently protect zl_suspend reads, combined with other logic from #14979.

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:

@amotin
Remove zl_issuer_lock from zil_suspend().
5395d11 
This locking was recently added as part of openzfs#14979. But appears it
is illegal to take zl_issuer_lock while holding dp_config_rwlock,
taken by dsl_pool_hold().  It causes deadlock with sync thread in
spa_sync_upgrades().  On a second throught though, we should not
need this locking, since zil_commit_impl() we call below takes
zl_issuer_lock, that should sufficiently protect zl_suspend reads,
combined with other logic from openzfs#14979.

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
@amotin amotin requested a review from behlendorf July 24, 2023 19:37
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jul 24, 2023
@amotin amotin requested a review from grwilson July 24, 2023 19:45
@amotin
Copy link
Member Author

amotin commented Jul 24, 2023

It should fix this: http://build.zfsonlinux.org/builders/CentOS%209%20x86_64%20%28TEST%29/builds/858/steps/shell_4/logs/console

@amotin amotin mentioned this pull request Jul 25, 2023
Merged
13 tasks
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 25, 2023
@behlendorf behlendorf merged commit 2848de1 into openzfs:master Jul 25, 2023
18 of 19 checks passed
@amotin amotin deleted the zil_suspend_fix branch July 25, 2023 16:10
amotin added a commit to amotin/zfs that referenced this pull request Jul 25, 2023
@amotin
Remove zl_issuer_lock from zil_suspend().
676fc30 
This locking was recently added as part of openzfs#14979. But appears it
is illegal to take zl_issuer_lock while holding dp_config_rwlock,
taken by dsl_pool_hold().  It causes deadlock with sync thread in
spa_sync_upgrades().  On a second thought, we should not
need this locking, since zil_commit_impl() we call below takes
zl_issuer_lock, that should sufficiently protect zl_suspend reads,
combined with other logic from openzfs#14979.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15103
behlendorf pushed a commit that referenced this pull request Jul 25, 2023
@amotin @behlendorf
Remove zl_issuer_lock from zil_suspend().
991834f 
This locking was recently added as part of #14979. But appears it
is illegal to take zl_issuer_lock while holding dp_config_rwlock,
taken by dsl_pool_hold().  It causes deadlock with sync thread in
spa_sync_upgrades().  On a second thought, we should not
need this locking, since zil_commit_impl() we call below takes
zl_issuer_lock, that should sufficiently protect zl_suspend reads,
combined with other logic from #14979.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes #15103
@behlendorf behlendorf mentioned this pull request Jul 25, 2023
Open
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
Remove zl_issuer_lock from zil_suspend().
d892ae0 
This locking was recently added as part of openzfs#14979. But appears it
is illegal to take zl_issuer_lock while holding dp_config_rwlock,
taken by dsl_pool_hold().  It causes deadlock with sync thread in
spa_sync_upgrades().  On a second thought, we should not
need this locking, since zil_commit_impl() we call below takes
zl_issuer_lock, that should sufficiently protect zl_suspend reads,
combined with other logic from openzfs#14979.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15103
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@grwilson grwilson Awaiting requested review from grwilson

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amotin @behlendorf