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 <[email protected]>
ClusterHQ · Oct 1, 2015 · 387d61b · 387d61b
1 parent b1d5794
commit 387d61b
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 7 deletions.
diff --git 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
@@ -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);
 }