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

VOP_RENAME fixes for FreeBSD #12717

Merged
merged 1 commit into from
Nov 19, 2021
Merged

VOP_RENAME fixes for FreeBSD #12717

merged 1 commit into from
Nov 19, 2021

Conversation

markjdb
Copy link
Contributor

@markjdb markjdb commented Nov 2, 2021

This set of patches fixes some problems encountered while testing a fix for a different, unrelated problem.

The VOP_RENAME implementation for FreeBSD had a couple of lock leaks and one use-after-free. This patch series addresses them, and hopefully makes zfs_freebsd_rename() and callees easier to read.

Motivation and Context

The rename implementation for FreeBSD had some bugs in the face of rollback and forced unmount. While they're not very easy to hit organically, stress tests hit them readily.

Description

The changes consist mostly of refactoring to ensure that ZFS_ENTER doesn't cause a lock leak.

How Has This Been Tested?

A test case for this was added to Peter Holm's venerable stress2 test suite: https://github.com/freebsd/freebsd-src/blob/main/tools/test/stress2/README

It consists of running various dumb I/O-intensive programs in a dataset which is periodically unmounted or rolled back to a snapshot.

I also booted with this change on a desktop system and am running ZFS regression tests.

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:

@ghost ghost added the Status: Code Review Needed Ready for review and testing label Nov 4, 2021
@ghost ghost self-requested a review November 4, 2021 12:16
ghost
ghost approved these changes Nov 14, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this does read much better.

@tonynguien tonynguien self-requested a review November 16, 2021 16:23
tonynguien
tonynguien requested changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@tonynguien tonynguien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. I have just one comment about the use of ZFS_VERIFY_ZP().

module/os/freebsd/zfs/zfs_vnops_os.c Show resolved Hide resolved
module/os/freebsd/zfs/zfs_vnops_os.c Show resolved Hide resolved
module/os/freebsd/zfs/zfs_vnops_os.c Show resolved Hide resolved
@tonynguien
Copy link
Contributor

@markjdb Would it make sense to squash some/all of the commits? The preference is single commit per request unless there's history we really should preserve. Thanks.

@markjdb
Copy link
Contributor Author

markjdb commented Nov 19, 2021

@markjdb Would it make sense to squash some/all of the commits? The preference is single commit per request unless there's history we really should preserve. Thanks.

I did it this way to make bisection a bit easier in case there's a regression, and to make the changes easier to read. I'm ok with squashing them though.

@markjdb
Fix several bugs in the FreeBSD rename VOP implementation
6be51b2 
- To avoid a use-after-free, zfsvfs->z_log needs to be loaded after the
  teardown lock is acquired with ZFS_ENTER().
- Avoid leaking vnode locks in zfs_rename_relock() and zfs_rename_()
  when the ZFS_ENTER() macros forces an early return.

Refactor the rename implementation so that ZFS_ENTER() can be used
safely.  As a bonus, this lets us use the ZFS_VERIFY_ZP() macro instead
of open-coding its implementation.

Reported-by: Peter Holm <[email protected]>
Tested-by: Peter Holm <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Sponsored-by: The FreeBSD Foundation
@tonynguien
Copy link
Contributor

@markjdb Would it make sense to squash some/all of the commits? The preference is single commit per request unless there's history we really should preserve. Thanks.

I did it this way to make bisection a bit easier in case there's a regression, and to make the changes easier to read. I'm ok with squashing them though.

Yes, the commits helped with readability. Perhaps, we can squash into two commits if we want to preserve some history, one with current first three and another with the last two commits.

@markjdb
Copy link
Contributor Author

markjdb commented Nov 19, 2021

Yes, the commits helped with readability. Perhaps, we can squash into two commits if we want to preserve some history, one with current first three and another with the last two commits.

I just went ahead and squashed them all together. :)

I think it's fine to just drop the history, the squashed commit isn't really that big.

@tonynguien
Copy link
Contributor

Yes, the commits helped with readability. Perhaps, we can squash into two commits if we want to preserve some history, one with current first three and another with the last two commits.

I just went ahead and squashed them all together. :)

I think it's fine to just drop the history, the squashed commit isn't really that big.

Yes, I didn't see the 5 commits when I reviewed the code ;) Sounds good. Thank you.

@tonynguien tonynguien merged commit ded851b into openzfs:master Nov 19, 2021
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 13, 2021
@markjdb @tonyhutter
Fix several bugs in the FreeBSD rename VOP implementation
57f4373 
- To avoid a use-after-free, zfsvfs->z_log needs to be loaded after the
  teardown lock is acquired with ZFS_ENTER().
- Avoid leaking vnode locks in zfs_rename_relock() and zfs_rename_()
  when the ZFS_ENTER() macros forces an early return.

Refactor the rename implementation so that ZFS_ENTER() can be used
safely.  As a bonus, this lets us use the ZFS_VERIFY_ZP() macro instead
of open-coding its implementation.

Reported-by: Peter Holm <[email protected]>
Tested-by: Peter Holm <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Sponsored-by: The FreeBSD Foundation
Closes openzfs#12717
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 13, 2021
@markjdb @tonyhutter
Fix several bugs in the FreeBSD rename VOP implementation
1933733 
- To avoid a use-after-free, zfsvfs->z_log needs to be loaded after the
  teardown lock is acquired with ZFS_ENTER().
- Avoid leaking vnode locks in zfs_rename_relock() and zfs_rename_()
  when the ZFS_ENTER() macros forces an early return.

Refactor the rename implementation so that ZFS_ENTER() can be used
safely.  As a bonus, this lets us use the ZFS_VERIFY_ZP() macro instead
of open-coding its implementation.

Reported-by: Peter Holm <[email protected]>
Tested-by: Peter Holm <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Sponsored-by: The FreeBSD Foundation
Closes openzfs#12717
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@markjdb @nicman23
Fix several bugs in the FreeBSD rename VOP implementation
bc9a645 
- To avoid a use-after-free, zfsvfs->z_log needs to be loaded after the
  teardown lock is acquired with ZFS_ENTER().
- Avoid leaking vnode locks in zfs_rename_relock() and zfs_rename_()
  when the ZFS_ENTER() macros forces an early return.

Refactor the rename implementation so that ZFS_ENTER() can be used
safely.  As a bonus, this lets us use the ZFS_VERIFY_ZP() macro instead
of open-coding its implementation.

Reported-by: Peter Holm <[email protected]>
Tested-by: Peter Holm <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Sponsored-by: The FreeBSD Foundation
Closes openzfs#12717
nicman23 pushed a commit to nicman23/zfs that referenced this pull request Aug 22, 2022
@markjdb @nicman23
Fix several bugs in the FreeBSD rename VOP implementation
3036a5f 
- To avoid a use-after-free, zfsvfs->z_log needs to be loaded after the
  teardown lock is acquired with ZFS_ENTER().
- Avoid leaking vnode locks in zfs_rename_relock() and zfs_rename_()
  when the ZFS_ENTER() macros forces an early return.

Refactor the rename implementation so that ZFS_ENTER() can be used
safely.  As a bonus, this lets us use the ZFS_VERIFY_ZP() macro instead
of open-coding its implementation.

Reported-by: Peter Holm <[email protected]>
Tested-by: Peter Holm <[email protected]>
Reviewed-by: Ryan Moeller <[email protected]>
Reviewed-by: Tony Nguyen <[email protected]>
Signed-off-by: Mark Johnston <[email protected]>
Sponsored-by: The FreeBSD Foundation
Closes openzfs#12717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonynguien tonynguien tonynguien approved these changes

Assignees

@tonynguien tonynguien

Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@markjdb @tonynguien