Issue 6065 - Adding ZVol as log device of seprate pool causes deadlock #6134
Conversation
|
@bprotopopov, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @tuxoko and @fajarnugraha to be potential reviewers. |
|
Addressed commit message style issues: |
module/zfs/zvol.c
Outdated
| * devices on top of zvols. In particular, management of device minor number | ||
| * operations - create, remove, rename, and set_snapdev - involves access to | ||
| * these structures. The zvol_state_lock is primarily used to protect the | ||
| * zvol_state_list, while the zv_state_lock is used to protect the contents |
There was a problem hiding this comment.
The names zvol_state_lock and zv_state_lock are so close how about adding zv-> here to make it clear it's a member of the structure. And I think we can break this in to two sentences.
- * zvol_state_list, while the zv_state_lock is used to protect the contents
+ * zvol_state_list. The zv->zv_state_lock is used to protect the contentsThere was a problem hiding this comment.
Sure, will change.
module/zfs/zvol.c
Outdated
| * operations - create, remove, rename, and set_snapdev - involves access to | ||
| * these structures. The zvol_state_lock is primarily used to protect the | ||
| * zvol_state_list, while the zv_state_lock is used to protect the contents | ||
| * of the zvol_state_t structures, as well as to make sure that when it the |
There was a problem hiding this comment.
s/when it the/when the/
module/zfs/zvol.c
Outdated
| return (SET_ERROR(error)); | ||
| if (zv != NULL) | ||
| mutex_exit(&zv->zv_state_lock); | ||
| return (SET_ERROR(ENXIO)); |
There was a problem hiding this comment.
Did you find an issue here returning error instead of ENXIO unconditionally?
There was a problem hiding this comment.
No, it looks like an oversight, will fix.
module/zfs/zvol.c
Outdated
| for (zv = list_head(&free_list); zv != NULL; zv = zv_next) { | ||
| zv_next = list_next(&free_list, zv); | ||
| zvol_free(zv); | ||
| } |
There was a problem hiding this comment.
How about using the while/list_head/list_remove idiom here.
while ((zv = list_head(&free_list)) != NULL) {
list_remove(&free_list, zv);
zvol_free(zv);
}
There was a problem hiding this comment.
Sure, seems reasonable to me.
|
Forced-pushed the changes. |
The lock is designed to protect internal state of zvol_state_t and to avoid taking spa_namespace_lock (e.g. in dmu_objset_own() code path) while holding zvol_stat_lock. Refactor the code accordingly. Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com>
Revert commit 1ee159f Fix lock order inversion with zvol_open()... as it did not account for use of zvols as vdevs. The latter use cases resulted in the lock order inversion deadlocks that involved spa_namespace_lock and bdev->bd_mutex. Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com>
|
Noticed a small typo in the commit message, re-pushed. |
|
Thanks for updating this. I've updated #6128 to include your original version of this patch. That PR additionally enables most of the test cases which depend on this functionality. Pushed to the bots for testing, everything worked as expected for me locally. |
|
awesome, @behlendorf, nice to hear |
|
The test failures were unrelated and I haven't observed any issues with this change locally. I'm merging it to master and we'll get the test cases which depend on it enabled soon to avoid regressing. @bprotopopov thanks for sorting this out! |
Fix lock order inversion with zvol_open() as it did not account for use of zvols as vdevs. The latter use cases resulted in the lock order inversion deadlocks that involved spa_namespace_lock and bdev->bd_mutex. Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Issue #6065 Issue #6134
Introduced zv_state_lock to protect internal state of zvol_state_t.
Reverted commit 1ee159f
Fix lock order inversion with zvol_open()...
Description
Introduced zv_state_lock to protect internal state of zvol_state_t and to avoid taking spa_namespace_lock (e.g. in dmu_objset_own() code path) while holding zvol_stat_lock. Refactore the code accordingly.
Reverted commit 1ee159f
Fix lock order inversion with zvol_open()...
as it did not account for use of zvols as vdevs. The latter use cases resulted in the lock order inversion deadlocks that involved spa_namespace_lock and bdev->bd_mutex.
Motivation and Context
This is needed to allow use of zvols as vdevs (logs, caches, regular vdevs).
How Has This Been Tested?
Tested use of zvols as regular vdevs, logs, and caches. Added and removed devices, exported, and re-imported pools. Performed basic I/O testing. Ran full suit of zfs-test (did not enable additional relevant test cases at this time).
Types of changes
Checklist:
Signed-off-by.