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

Fix z_hold_mtx deadlock #4124

Closed
wants to merge 2 commits into from
Closed

Fix z_hold_mtx deadlock #4124

wants to merge 2 commits into from

Conversation

behlendorf
Copy link
Contributor

Add a zfs_object_mutex_size module option to facilitate resizing the
the per-dataset znode mutex array. Increasing this value may help
make the deadlock described in #4106 less common, but this is not a
proper fix. This patch is primarily to aid debugging and analysis.

Signed-off-by: Brian Behlendorf behlendorf1@llnl.gov
Issue #4106

@behlendorf behlendorf mentioned this pull request Dec 18, 2015
Closed
@dweeezil
Copy link
Contributor

OK, that the same thread wouldn't take the lock twice to me wasn't clear to me.

@behlendorf behlendorf changed the title Add zfs_object_mutex_size module option Fix z_hold_mtx deadlock Dec 28, 2015
@behlendorf behlendorf mentioned this pull request Dec 28, 2015
Closed
BjoKaSH
BjoKaSH reviewed Jan 5, 2016
View reviewed changes
module/zfs/zfs_znode.c
* never block waiting on a z_hold_locks which just happens to have hashed
* to the same index.
*
* 2) All locks to used serialize access to an object are per-object and never
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be "All locks used to ..." instead of "... to used ..."?
(I am not a native speaker and might be wrong, but it confused me on first read.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for catching it I'll fix that when I refresh the patch.

@behlendorf
Add zfs_object_mutex_size module option
8313d66 
Add a zfs_object_mutex_size module option to facilitate resizing the
the per-dataset znode mutex array.  Increasing this value may help
make the deadlock described in openzfs#4106 less common, but this is not a
proper fix.  This patch is primarily to aid debugging and analysis.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#4106
@behlendorf
Fix zsb->z_hold_mtx deadlock
471336e 
The zfs_znode_hold_enter() / zfs_znode_hold_exit() functions are used to
serialize access to a znode and its SA buffer while the object is being
created or destroyed.  This kind of locking would normally reside in the
znode itself but in this case that's impossible because the znode and SA
buffer may not yet exist.  Therefore the locking is handled externally
with an array of mutexs and AVLs trees which contain per-object locks.

In zfs_znode_hold_enter() a per-object lock is created as needed, inserted
in to the correct AVL tree and finally the per-object lock is held.  In
zfs_znode_hold_exit() the process is reversed.  The per-object lock is
released, removed from the AVL tree and destroyed if there are no waiters.

This scheme has two important properties:

1) No memory allocations are performed while holding one of the z_hold_locks.
   This ensures evict(), which can be called from direct memory reclaim, will
   never block waiting on a z_hold_locks which just happens to have hashed
   to the same index.

2) All locks used to serialize access to an object are per-object and never
   shared.  This minimizes lock contention without creating a large number
   of dedicated locks.

On the downside it does require znode_lock_t structures to be frequently
allocated and freed.  However, because these are backed by a kmem cache
and very short lived this cost is minimal.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#4106
@behlendorf
Copy link
Contributor Author

@dweeezil what are your feeling about merging this to master. While it's not a pretty fix it does appear to have resolved a very real class of deadlock which users are hitting today. I'm inclined to merge it since it's seen substantial testing and I haven't heard any better solution proposed (or implemented).

@dweeezil
Copy link
Contributor

@behlendorf I've got no problem with this being parameterized. The patch looks fine to me.

That said, I'm pretty sure the original problem will be fixed by the spl cv_wait patch. I do like the option of having this tunable available in the future.

@behlendorf
Copy link
Contributor Author

Well the condition variable patch solves half of the issue. We still had the deadlock due to the memory allocations under the mutex . Those are addressed by the second patch in this stack.

@kernelOfTruth kernelOfTruth mentioned this pull request Jan 14, 2016
Closed
@behlendorf
Copy link
Contributor Author

Merged to master, we can revisit if these is a better way to solve this problem but for now the patch addresses a real issue encountered by users.

c96c36f Fix zsb->z_hold_mtx deadlock
0720116 Add zfs_object_mutex_size module option

@behlendorf behlendorf closed this Jan 16, 2016
behlendorf added a commit to behlendorf/zfs that referenced this pull request Jan 19, 2016
@behlendorf
Close possible zfs_znode_held() race
a6aa706 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs#4124
@behlendorf behlendorf mentioned this pull request Jan 19, 2016
Closed
behlendorf added a commit to behlendorf/zfs that referenced this pull request Jan 20, 2016
@behlendorf
Close possible zfs_znode_held() race
37c5634 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
behlendorf added a commit that referenced this pull request Jan 29, 2016
@behlendorf
Close possible zfs_znode_held() race
0a2f957 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes #4244
Issue #4124
goulvenriou pushed a commit to Alyseo/zfs that referenced this pull request Feb 3, 2016
@behlendorf
Close possible zfs_znode_held() race
9ffa48b 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
Flefebvre pushed a commit to Flefebvre/zfs that referenced this pull request Feb 3, 2016
@behlendorf
Close possible zfs_znode_held() race
9f42c2f 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
Flefebvre pushed a commit to Flefebvre/zfs that referenced this pull request Feb 3, 2016
@behlendorf
Close possible zfs_znode_held() race
cb77974 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
Flefebvre pushed a commit to Flefebvre/zfs that referenced this pull request Feb 3, 2016
@behlendorf
Close possible zfs_znode_held() race
4045802 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
tuxoko pushed a commit to tuxoko/zfs that referenced this pull request Feb 8, 2016
@behlendorf
Close possible zfs_znode_held() race
8133fa5 
Check if the lock is held while holding the z_hold_locks() lock.
This prevents a possible use-after-free bug for callers which are
not holding the lock.  There currently are no such callers so this
can't cause a problem today but it has been fixed regardless.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <tuxoko@gmail.com>
Closes openzfs#4244
Issue openzfs#4124
@behlendorf behlendorf deleted the issue-4106 branch April 19, 2021 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@behlendorf @dweeezil @BjoKaSH