Snap -r with existing snaps can cause panic

Certain arrangment of existing snaps with same name as specified in new snap -r command can create a situation during sync phase with entries in dp_dirty_dirs but no dirty dbufs in the MOS. Also when zfs tests are destroying datasets and then the pool there is a race condition between dmu_objset_evict and dbuf_evict_one, resulting in a hang. Signed-off-by: Paul Zuchowski <[email protected]> Fixes openzfs#8380
datto · Jun 14, 2019 · 577f4fe · 577f4fe
1 parent 4c0883f
commit 577f4fe
Show file tree

Hide file tree

Showing 9 changed files with 296 additions and 11 deletions.
diff --git a/include/sys/dbuf.h b/include/sys/dbuf.h
@@ -317,6 +317,8 @@ void dbuf_rele_and_unlock(dmu_buf_impl_t *db, void *tag, boolean_t evicting);
 
 dmu_buf_impl_t *dbuf_find(struct objset *os, uint64_t object, uint8_t level,
     uint64_t blkid);
+dmu_buf_impl_t *dbuf_find_evicting(struct objset *os, uint64_t object,
+    uint8_t level, uint64_t blkid);
 
 int dbuf_read(dmu_buf_impl_t *db, zio_t *zio, uint32_t flags);
 void dmu_buf_will_not_fill(dmu_buf_t *db, dmu_tx_t *tx);

diff --git a/include/sys/dsl_dataset.h b/include/sys/dsl_dataset.h
@@ -422,6 +422,8 @@ void dsl_dataset_clone_swap_sync_impl(dsl_dataset_t *clone,
     dsl_dataset_t *origin_head, dmu_tx_t *tx);
 int dsl_dataset_snapshot_check_impl(dsl_dataset_t *ds, const char *snapname,
     dmu_tx_t *tx, boolean_t recv, uint64_t cnt, cred_t *cr);
+int dsl_dataset_snapshot_reserve_space(dsl_dataset_t *ds, dmu_tx_t *tx);
+
 void dsl_dataset_snapshot_sync_impl(dsl_dataset_t *ds, const char *snapname,
     dmu_tx_t *tx);
 

diff --git a/module/zfs/dbuf.c b/module/zfs/dbuf.c
@@ -328,8 +328,9 @@ dbuf_hash(void *os, uint64_t obj, uint8_t lvl, uint64_t blkid)
 	(dbuf)->db_level == (level) &&			\
 	(dbuf)->db_blkid == (blkid))
 
-dmu_buf_impl_t *
-dbuf_find(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid)
+static dmu_buf_impl_t *
+dbuf_find_impl(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid,
+    boolean_t find_evicting)
 {
 	dbuf_hash_table_t *h = &dbuf_hash_table;
 	uint64_t hv;
@@ -341,9 +342,12 @@ dbuf_find(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid)
 
 	mutex_enter(DBUF_HASH_MUTEX(h, idx));
 	for (db = h->hash_table[idx]; db != NULL; db = db->db_hash_next) {
+		if (db->db_state != DB_EVICTING && find_evicting)
+			continue;
 		if (DBUF_EQUAL(db, os, obj, level, blkid)) {
 			mutex_enter(&db->db_mtx);
-			if (db->db_state != DB_EVICTING) {
+			if ((db->db_state == DB_EVICTING && find_evicting) ||
+			    (db->db_state != DB_EVICTING && !find_evicting)) {
 				mutex_exit(DBUF_HASH_MUTEX(h, idx));
 				return (db);
 			}
@@ -354,6 +358,19 @@ dbuf_find(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid)
 	return (NULL);
 }
 
+dmu_buf_impl_t *
+dbuf_find_evicting(objset_t *os, uint64_t obj, uint8_t level,
+    uint64_t blkid)
+{
+	return (dbuf_find_impl(os, obj, level, blkid, B_TRUE));
+}
+
+dmu_buf_impl_t *
+dbuf_find(objset_t *os, uint64_t obj, uint8_t level, uint64_t blkid)
+{
+	return (dbuf_find_impl(os, obj, level, blkid, B_FALSE));
+}
+
 static dmu_buf_impl_t *
 dbuf_find_bonus(objset_t *os, uint64_t object)
 {

diff --git a/module/zfs/dmu_recv.c b/module/zfs/dmu_recv.c
@@ -2681,6 +2681,11 @@ dmu_recv_end_check(void *arg, dmu_tx_t *tx)
 		}
 		error = dsl_dataset_snapshot_check_impl(origin_head,
 		    drc->drc_tosnap, tx, B_TRUE, 1, drc->drc_cred);
+		if (error != 0) {
+			dsl_dataset_rele(origin_head, FTAG);
+			return (error);
+		}
+		error = dsl_dataset_snapshot_reserve_space(origin_head, tx);
 		dsl_dataset_rele(origin_head, FTAG);
 		if (error != 0)
 			return (error);
@@ -2689,6 +2694,10 @@ dmu_recv_end_check(void *arg, dmu_tx_t *tx)
 	} else {
 		error = dsl_dataset_snapshot_check_impl(drc->drc_ds,
 		    drc->drc_tosnap, tx, B_TRUE, 1, drc->drc_cred);
+		if (error != 0)
+			return (error);
+
+		error = dsl_dataset_snapshot_reserve_space(drc->drc_ds, tx);
 	}
 	return (error);
 }

diff --git a/module/zfs/dnode_sync.c b/module/zfs/dnode_sync.c
@@ -427,6 +427,7 @@ dnode_evict_dbufs(dnode_t *dn)
 {
 	dmu_buf_impl_t *db_marker;
 	dmu_buf_impl_t *db, *db_next;
+	dmu_buf_impl_t *evicting_db;
 
 	db_marker = kmem_alloc(sizeof (dmu_buf_impl_t), KM_SLEEP);
 
@@ -442,6 +443,24 @@ dnode_evict_dbufs(dnode_t *dn)
 		mutex_enter(&db->db_mtx);
 		if (db->db_state != DB_EVICTING &&
 		    zfs_refcount_is_zero(&db->db_holds)) {
+			/*
+			 * It is rare but if dn_dbufs has a dbuf with
+			 * this same bookmark but having db_state of
+			 * DB_EVICTING, we won't be able to insert
+			 * the db_marker.  Skip this dbuf if that is
+			 * the case.
+			 */
+			evicting_db = dbuf_find_evicting(dn->dn_objset,
+			    dn->dn_object, db->db_level, db->db_blkid);
+
+			if (evicting_db != NULL) {
+				mutex_exit(&evicting_db->db_mtx);
+				db->db_pending_evict = TRUE;
+				mutex_exit(&db->db_mtx);
+				db_next = AVL_NEXT(&dn->dn_dbufs, db);
+				continue;
+			}
+
 			db_marker->db_level = db->db_level;
 			db_marker->db_blkid = db->db_blkid;
 			db_marker->db_state = DB_SEARCH;

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
@@ -1339,7 +1339,7 @@ dsl_dataset_dirty(dsl_dataset_t *ds, dmu_tx_t *tx)
 	}
 }
 
-static int
+int
 dsl_dataset_snapshot_reserve_space(dsl_dataset_t *ds, dmu_tx_t *tx)
 {
 	uint64_t asize;
@@ -1419,18 +1419,20 @@ dsl_dataset_snapshot_check_impl(dsl_dataset_t *ds, const char *snapname,
 			return (error);
 	}
 
-	error = dsl_dataset_snapshot_reserve_space(ds, tx);
-	if (error != 0)
-		return (error);
-
 	return (0);
 }
 
+struct checknode {
+    list_node_t link;
+    dsl_dataset_t *ds;
+};
+
 int
 dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 {
 	dsl_dataset_snapshot_arg_t *ddsa = arg;
 	dsl_pool_t *dp = dmu_tx_pool(tx);
+	struct checknode *chknode;
 	nvpair_t *pair;
 	int rv = 0;
 
@@ -1531,6 +1533,9 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 		nvlist_free(cnt_track);
 	}
 
+	list_t datasets;
+	list_create(&datasets, sizeof (struct checknode),
+	    offsetof(struct checknode, link));
 	for (pair = nvlist_next_nvpair(ddsa->ddsa_snaps, NULL);
 	    pair != NULL; pair = nvlist_next_nvpair(ddsa->ddsa_snaps, pair)) {
 		int error = 0;
@@ -1554,7 +1559,6 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 			/* passing 0/NULL skips dsl_fs_ss_limit_check */
 			error = dsl_dataset_snapshot_check_impl(ds,
 			    atp + 1, tx, B_FALSE, 0, NULL);
-			dsl_dataset_rele(ds, FTAG);
 		}
 
 		if (error != 0) {
@@ -1564,6 +1568,42 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 			}
 			rv = error;
 		}
+		chknode = kmem_alloc(sizeof (*chknode), KM_SLEEP);
+		chknode->ds = ds;
+		list_insert_tail(&datasets, chknode);
+	}
+	if (rv != 0) {
+		while ((chknode = list_remove_head(&datasets)) != NULL) {
+			dsl_dataset_rele(chknode->ds, FTAG);
+			kmem_free(chknode, sizeof (*chknode));
+		}
+		list_destroy(&datasets);
+		return (rv);
+	}
+
+	/* Now reserve space since we passed all the checks. */
+	while ((chknode = list_remove_head(&datasets)) != NULL) {
+		int error = 0;
+
+		/*
+		 * This accumulates dsl_dir_t dd_space_to_write which may
+		 * need to be undone if we get ENOSPC while reserving.
+		 */
+		error = dsl_dataset_snapshot_reserve_space(chknode->ds, tx);
+		dsl_dataset_rele(chknode->ds, FTAG);
+		kmem_free(chknode, sizeof (*chknode));
+		if (error != 0)
+			rv = error;
+	}
+	list_destroy(&datasets);
+	if (rv == 0)
+		return (rv);
+
+	ASSERT3U(rv, ==, ENOSPC);
+	dsl_dir_t *dd;
+	/* Undo the reserved space since no snapshot will be taken. */
+	while ((dd = txg_list_remove(&dp->dp_dirty_dirs, tx->tx_txg)) != NULL) {
+		dsl_dir_sync(dd, tx);
 	}
 
 	return (rv);
@@ -1863,6 +1903,11 @@ dsl_dataset_snapshot_tmp_check(void *arg, dmu_tx_t *tx)
 		dsl_dataset_rele(ds, FTAG);
 		return (error);
 	}
+	error = dsl_dataset_snapshot_reserve_space(ds, tx);
+	if (error != 0) {
+		dsl_dataset_rele(ds, FTAG);
+		return (error);
+	}
 
 	dsl_dataset_rele(ds, FTAG);
 	return (0);

diff --git a/tests/runfiles/linux.run b/tests/runfiles/linux.run
@@ -816,7 +816,8 @@ tests = ['clone_001_pos', 'rollback_001_pos', 'rollback_002_pos',
     'snapshot_006_pos', 'snapshot_007_pos', 'snapshot_008_pos',
     'snapshot_009_pos', 'snapshot_010_pos', 'snapshot_011_pos',
     'snapshot_012_pos', 'snapshot_013_pos', 'snapshot_014_pos',
-    'snapshot_015_pos', 'snapshot_016_pos', 'snapshot_017_pos']
+    'snapshot_015_pos', 'snapshot_016_pos', 'snapshot_017_pos',
+    'snapshot_fail_existing']
 tags = ['functional', 'snapshot']
 
 [tests/functional/snapused]

diff --git a/tests/zfs-tests/tests/functional/snapshot/Makefile.am b/tests/zfs-tests/tests/functional/snapshot/Makefile.am
@@ -22,7 +22,8 @@ dist_pkgdata_SCRIPTS = \
 	snapshot_014_pos.ksh \
 	snapshot_015_pos.ksh \
 	snapshot_016_pos.ksh \
-	snapshot_017_pos.ksh
+	snapshot_017_pos.ksh \
+	snapshot_fail_existing.ksh
 
 dist_pkgdata_DATA = \
 	snapshot.cfg