Skip to content

Fix lock reversal on device removal cancel #17164

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
Merged
merged 1 commit into from
Apr 1, 2025
Merged

Fix lock reversal on device removal cancel #17164

merged 1 commit into from
Apr 1, 2025

Conversation

amotin
Copy link
Member

@amotin amotin commented Mar 21, 2025 •
edited
Loading

FreeBSD kernel's WITNESS code detected lock ordering violation in spa_vdev_remove_cancel_sync(). It took svr_lock while holding ms_lock, which is opposite to other places. I was thinking to resolve it similar to #17145, but looking closer I don't think we even need svr_lock at that point, since we already asserted svr_allocd_segs is empty, and we don't need to add there segments we are going to call free_mapped_segment_cb for.

How Has This Been Tested?

Not really. Just trying to make CI happy.

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
Fix lock reversal on device removal cancel
9f43530 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Mar 24, 2025
@amotin amotin merged commit 301da59 into openzfs:master Apr 1, 2025
46 of 52 checks passed
@amotin amotin deleted the remove_cancel_lor branch April 1, 2025 13:31
@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 1, 2025
robn pushed a commit to robn/zfs that referenced this pull request Apr 4, 2025
@amotin @robn
Fix lock reversal on device removal cancel
395db27 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Allan Jude <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#17164

(cherry picked from commit 301da59)
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 8, 2025
@amotin @tonyhutter
Fix lock reversal on device removal cancel
50abcfa 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Allan Jude <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#17164
fuporovvStack pushed a commit to fuporovvStack/zfs that referenced this pull request Apr 11, 2025
@amotin @FedorUporovVstack
Fix lock reversal on device removal cancel
377c7ab 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Allan Jude <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#17164
fuporovvStack pushed a commit to fuporovvStack/zfs that referenced this pull request Apr 11, 2025
@amotin @FedorUporovVstack
Fix lock reversal on device removal cancel
4ff1ba9 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Allan Jude <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#17164
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Apr 16, 2025
@amotin @tonyhutter
Fix lock reversal on device removal cancel
6f2080f 
FreeBSD kernel's WITNESS code detected lock ordering violation in
spa_vdev_remove_cancel_sync().  It took svr_lock while holding
ms_lock, which is opposite to other places.  I was thinking to
resolve it similar to openzfs#17145, but looking closer I don't think
we even need svr_lock at that point, since we already asserted
svr_allocd_segs is empty, and we don't need to add there segments
we are going to call free_mapped_segment_cb for.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Allan Jude <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Closes openzfs#17164
amotin added a commit to amotin/zfs that referenced this pull request May 21, 2025
@amotin
Fix null dereference in spa_vdev_remove_cancel_sync()
908bfb0 
We don't really need to access space map to know where the metaslab
ends, while msp->ms_sm might be NULL.

Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Fixes openzfs#17164
Fixes openzfs#17359
@amotin amotin mentioned this pull request May 21, 2025
Merged
14 tasks
amotin added a commit that referenced this pull request May 22, 2025
@amotin
Fix null dereference in spa_vdev_remove_cancel_sync()
5c30b24 
We don't really need to access space map to know where the metaslab
ends, while msp->ms_sm might be NULL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Reviewed by: Igor Kozhukhov <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Fixes #17164
Fixes #17359
Closes #17361
robn pushed a commit to robn/zfs that referenced this pull request May 23, 2025
@amotin @robn
Fix null dereference in spa_vdev_remove_cancel_sync()
a332ba2 
We don't really need to access space map to know where the metaslab
ends, while msp->ms_sm might be NULL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Reviewed by: Igor Kozhukhov <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Fixes openzfs#17164
Fixes openzfs#17359
Closes openzfs#17361
(cherry picked from commit 5c30b24)
@robn robn mentioned this pull request May 23, 2025
Open
14 tasks
robn pushed a commit to robn/zfs that referenced this pull request May 24, 2025
@amotin @robn
Fix null dereference in spa_vdev_remove_cancel_sync()
4321bf5 
We don't really need to access space map to know where the metaslab
ends, while msp->ms_sm might be NULL.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Rob Norris <[email protected]>
Reviewed by: Igor Kozhukhov <[email protected]>
Signed-off-by:	Alexander Motin <[email protected]>
Sponsored by:	iXsystems, Inc.
Fixes openzfs#17164
Fixes openzfs#17359
Closes openzfs#17361
(cherry picked from commit 5c30b24)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allanjude allanjude allanjude approved these changes

@tonyhutter tonyhutter tonyhutter approved these changes

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.
3 participants
@amotin @allanjude @tonyhutter