From 387d61bd3c9d23291fe5512be55b8b203db9221d Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 1 Oct 2015 11:32:20 -0400 Subject: [PATCH] Fix ZFS deadlock on db_mtx and dn_holds Illumos 5056 was intended to fix this deadlock, but it addressed it in a manner that added significant complexity without much in terms of documented gains aside from fixing the deadlock. It also introduced a runtime failure when assertions were enabled. It has been reverted in favor of this simpler solution where we protect user callbacks with a new db_user_mtx that allows us to drop the db_mtx during the callback. Signed-off-by: Richard Yao --- include/sys/dbuf.h | 12 ++++++++---- module/zfs/dbuf.c | 20 +++++++++++++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/include/sys/dbuf.h b/include/sys/dbuf.h index 8ade4c9c0b79..6e4ea27b204c 100644 --- a/include/sys/dbuf.h +++ b/include/sys/dbuf.h @@ -195,6 +195,14 @@ typedef struct dmu_buf_impl { */ uint8_t db_level; + /* + * db_user_mtx protects the two members below it. + * stuff we store for the user (see dmu_buf_set_user) + */ + kmutex_t db_user_mtx; + void *db_user_ptr; + dmu_buf_evict_func_t *db_evict_func; + /* db_mtx protects the members below */ kmutex_t db_mtx; @@ -227,10 +235,6 @@ typedef struct dmu_buf_impl { /* Data which is unique to data (leaf) blocks: */ - /* stuff we store for the user (see dmu_buf_set_user) */ - void *db_user_ptr; - dmu_buf_evict_func_t *db_evict_func; - uint8_t db_immediate_evict; uint8_t db_freed_in_flight; diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c index 79a966b25082..8cf92c75508a 100644 --- a/module/zfs/dbuf.c +++ b/module/zfs/dbuf.c @@ -91,6 +91,7 @@ dbuf_cons(void *vdb, void *unused, int kmflag) dmu_buf_impl_t *db = vdb; bzero(db, sizeof (dmu_buf_impl_t)); + mutex_init(&db->db_user_mtx, NULL, MUTEX_DEFAULT, NULL); mutex_init(&db->db_mtx, NULL, MUTEX_DEFAULT, NULL); cv_init(&db->db_changed, NULL, CV_DEFAULT, NULL); refcount_create(&db->db_holds); @@ -103,6 +104,7 @@ static void dbuf_dest(void *vdb, void *unused) { dmu_buf_impl_t *db = vdb; + mutex_destroy(&db->db_user_mtx); mutex_destroy(&db->db_mtx); cv_destroy(&db->db_changed); refcount_destroy(&db->db_holds); @@ -268,12 +270,24 @@ dbuf_evict_user(dmu_buf_impl_t *db) { ASSERT(MUTEX_HELD(&db->db_mtx)); - if (db->db_level != 0 || db->db_evict_func == NULL) + if (db->db_level != 0) return; + mutex_enter(&db->db_user_mtx); + mutex_exit(&db->db_mtx); + + if (db->db_evict_func == NULL) { + mutex_exit(&db->db_user_mtx); + mutex_enter(&db->db_mtx); + return; + } + db->db_evict_func(&db->db, db->db_user_ptr); db->db_user_ptr = NULL; db->db_evict_func = NULL; + + mutex_exit(&db->db_user_mtx); + mutex_enter(&db->db_mtx); } boolean_t @@ -2393,7 +2407,7 @@ dmu_buf_update_user(dmu_buf_t *db_fake, void *old_user_ptr, void *user_ptr, ASSERT((user_ptr == NULL) == (evict_func == NULL)); - mutex_enter(&db->db_mtx); + mutex_enter(&db->db_user_mtx); if (db->db_user_ptr == old_user_ptr) { db->db_user_ptr = user_ptr; @@ -2402,7 +2416,7 @@ dmu_buf_update_user(dmu_buf_t *db_fake, void *old_user_ptr, void *user_ptr, old_user_ptr = db->db_user_ptr; } - mutex_exit(&db->db_mtx); + mutex_exit(&db->db_user_mtx); return (old_user_ptr); }