FreeBSD: Fix zvol_*_open() lock inversion

[ghost]

Motivation and Context

These are the changes for FreeBSD corresponding to the changes made for Linux in #12863, see that PR for motivation.

Description

Changes from #12863 applied for zvol_geom_open and zvol_cdev_open on FreeBSD.
This also adds a check for the zvol dying which we had in zvol_geom_open but was missing in zvol_cdev_open. The check causes the open to fail early with ENXIO when we are in the middle of changing volmode.
While porting the changes I noticed the comments in this code look a bit nicer on Linux so went ahead and added a second commit to clean them up a bit for FreeBSD as well.

How Has This Been Tested?

ZTS passed on FreeBSD.

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:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[ghost]

I need to write the body of the commit message for the main commit before this goes in, but I'll let ZTS run for now.

[behlendorf]

I don't think you need to delay here, but you may want to schedule(). The situation may be different on FreeBSD, but on Linux at least it was possible to get stuck spinning here on a single processor system without the schedule.

[amotin]

kern_yield(PRI_USER), I suppose. But it will still spin if some other thread is holding spa_namespace_lock, just probably not deadlock on UP, indeed.

[amotin]

I am not intimate with all the multiple locks there, but it looks wrong to me to first check for mutex_owned(&spa_namespace_lock), that assumes the namespace lock may be held from outside the function, but then try to enforce lock ordering with zv_suspend_lock taken inside this function.

@behlendorf ^^^ the same issue I suppose in Linux.

[amotin]

kern_yield(PRI_USER), I suppose. But it will still spin if some other thread is holding spa_namespace_lock, just probably not deadlock on UP, indeed.

[behlendorf]

The lock ordering requirement is that we need to make sure the take the spa_namespace_lock before the zv_suspend_lock. Otherwise, it's possible to encounter this lock inversion, #12863 (comment), between zvol_suspend() and zvol_open()/zvol_geom_open().

[amotin]

The lock ordering requirement is that we need to make sure the take the spa_namespace_lock before the zv_suspend_lock.

But in your commit you've moved zv_suspend_lock acquisition up, before the spa_namespace_lock. I am at loss.

[behlendorf]

But in your commit you've moved zv_suspend_lock acquisition up, before the spa_namespace_lock. I am at loss.

Sorry, I really should have proofread my comment better, I got it exactly backwards. We need to take the spa_namespace_lock after the zv_suspend_lock since this is the order required by the zvol_suspend call path. Altering that ordering would be invasive, and if you look at the comment at the top of spa_open_common() it does already handle the case where the spa_namespace_lock is already held.

[ghost]

I have the commit updated and am letting it run through 50 iterations of the rsend tag in ZTS to be safe. The locking is certainly spaghetti code here but it seems correct to me, or at least it obeys the ordering described in the comment at the top of module/zfs/zvol.c (which doesn't mention spa_namespace_lock, fwiw).

[amotin]

Sorry, I really should have proofread my comment better, I got it exactly backwards. We need to take the spa_namespace_lock after the zv_suspend_lock since this is the order required by the zvol_suspend call path. Altering that ordering would be invasive, and if you look at the comment at the top of spa_open_common() it does already handle the case where the spa_namespace_lock is already held.

@behlendorf That resolves my confusion, but does not answer my question: Can zvol_open()/zvol_*_open() be called with spa_namespace_lock held (I can think of only nested pools)? If yes, then there is still chance of lock order reversal, since caller won't hold zv_suspend_lock/zv_state_lock for the zvol in advance. If no, then check for mutex_owned(&spa_namespace_lock) makes no sense to me.

[ghost]

I don't see any way to enter the zvol open functions with spa_namespace_lock held. My understanding is that the mutex_owned check is there for the first open retry logic, which should acquire the locks in the correct order with spa_namespace_lock taken last.

[amotin]

mutex_owned check is there for the first open retry logic, which should acquire the locks in the correct order with spa_namespace_lock taken last.

That would be true before the change(s), since previously spa_namespace_lock was acquired before the retry. But with the change(s) retry is done without the lock acquisition, spinning in retrial instead of waiting for the lock once.

[behlendorf]

Can zvol_open()/zvol_*_open() be called with spa_namespace_lock held

It can, but nested pools are the only scenario I'm aware of where it's possible. Sadly you're right, this change doesn't handle that particular case. It only covers the more common case we happened were seeing with some zvol consumers. Fixing both looked like it would require far more disruptive locking changes that I was trying to avoid. To my knowledge layering a zpool on zvol has always been a bit dodgy on every platform so I wasn't too focused on it.

[AttilaFueloep]

To my knowledge layering a zpool on zvol has always been a bit dodgy on every platform so I wasn't too focused on it.

Yes, I can confirm, tried this (actually one half of a mirror vdev on a zvol ) on SmartOS some years ago and ran into deadlocks every now and then.

Although I can't find any pointers right now, I vaguely remember discussing nested zfs years ago. If memory serves me well, the standpoint was that while not officially supported, the team tries to not break it lightly. Indeed nesting a pool on another can come in handy on rare occasions ,but I agree that the effort has to be weight up.

[behlendorf]

@freqlabs @amotin if you guys are happy with this I think it's reasonable to merge.

[ghost]

It's okay with me. Fixing the locking for nested pools is not where want to go with this.

[amotin]

I am not happy with that custom "spinlock", but I accept there is a reason for such a mess.