Snap -r with existing snaps can cause panic

Certain arrangment of existing snaps with the 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 · Nov 13, 2019 · e00aa2f · e00aa2f
1 parent 64c77c4
commit e00aa2f
Show file tree

Hide file tree

Showing 7 changed files with 279 additions and 13 deletions.
diff --git a/include/sys/dsl_dataset.h b/include/sys/dsl_dataset.h
@@ -188,6 +188,9 @@ typedef struct dsl_dataset {
 	uint64_t ds_bookmarks_obj;  /* DMU_OTN_ZAP_METADATA */
 	avl_tree_t ds_bookmarks; /* dsl_bookmark_node_t */
 
+	/* used in snapshot check function */
+	list_node_t ds_check;
+
 	/* has internal locking: */
 	dsl_deadlist_t ds_deadlist;
 	bplist_t ds_pending_deadlist;
@@ -446,6 +449,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/dmu_recv.c b/module/zfs/dmu_recv.c
@@ -2935,6 +2935,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);
@@ -2943,6 +2948,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
@@ -457,7 +457,7 @@ dnode_sync_free_range(void *arg, uint64_t blkid, uint64_t nblks)
 void
 dnode_evict_dbufs(dnode_t *dn)
 {
-	dmu_buf_impl_t *db_marker;
+	dmu_buf_impl_t *db_marker, *insert_point;
 	dmu_buf_impl_t *db, *db_next;
 
 	db_marker = kmem_alloc(sizeof (dmu_buf_impl_t), KM_SLEEP);
@@ -474,12 +474,30 @@ 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.  Work backward to find the correct
+			 * insertion point.
+			 */
 			db_marker->db_level = db->db_level;
 			db_marker->db_blkid = db->db_blkid;
 			db_marker->db_state = DB_SEARCH;
-			avl_insert_here(&dn->dn_dbufs, db_marker, db,
-			    AVL_BEFORE);
-
+			insert_point = AVL_PREV(&dn->dn_dbufs, db);
+			while (insert_point != NULL &&
+			    insert_point->db_level == db->db_level &&
+			    insert_point->db_blkid == db->db_blkid) {
+				insert_point = AVL_PREV(&dn->dn_dbufs,
+				    insert_point);
+			}
+			if (insert_point == NULL) {
+				avl_insert_here(&dn->dn_dbufs, db_marker,
+				    avl_first(&dn->dn_dbufs), AVL_BEFORE);
+			} else {
+				avl_insert_here(&dn->dn_dbufs, db_marker,
+				    insert_point, AVL_AFTER);
+			}
 			/*
 			 * We need to use the "marker" dbuf rather than
 			 * simply getting the next dbuf, because

diff --git a/module/zfs/dsl_dataset.c b/module/zfs/dsl_dataset.c
@@ -1435,7 +1435,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;
@@ -1515,13 +1515,10 @@ 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);
 }
 
+
 int
 dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 {
@@ -1627,12 +1624,17 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 		nvlist_free(cnt_track);
 	}
 
+	list_t datasets;
+	dsl_dataset_t *check_ds;
+	list_create(&datasets, sizeof (struct dsl_dataset),
+	    offsetof(struct dsl_dataset, ds_check));
 	for (pair = nvlist_next_nvpair(ddsa->ddsa_snaps, NULL);
 	    pair != NULL; pair = nvlist_next_nvpair(ddsa->ddsa_snaps, pair)) {
 		int error = 0;
 		dsl_dataset_t *ds;
 		char *name, *atp = NULL;
 		char dsname[ZFS_MAX_DATASET_NAME_LEN];
+		boolean_t dsheld = B_FALSE;
 
 		name = nvpair_name(pair);
 		if (strlen(name) >= ZFS_MAX_DATASET_NAME_LEN)
@@ -1644,23 +1646,60 @@ dsl_dataset_snapshot_check(void *arg, dmu_tx_t *tx)
 			if (error == 0)
 				(void) strlcpy(dsname, name, atp - name + 1);
 		}
-		if (error == 0)
+		if (error == 0) {
 			error = dsl_dataset_hold(dp, dsname, FTAG, &ds);
+			dsheld = (error == 0);
+		}
 		if (error == 0) {
 			/* 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) {
 			if (ddsa->ddsa_errors != NULL) {
 				fnvlist_add_int32(ddsa->ddsa_errors,
 				    name, error);
 			}
+			if (dsheld)
+				dsl_dataset_rele(ds, FTAG);
 			rv = error;
+		} else {
+			list_insert_tail(&datasets, ds);
 		}
 	}
+	if (rv != 0) {
+		while ((check_ds = list_remove_head(&datasets)) != NULL) {
+			dsl_dataset_rele(check_ds, FTAG);
+		}
+		list_destroy(&datasets);
+		return (rv);
+	}
+
+	/* Now reserve space since we passed all the checks. */
+	while ((check_ds = 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(check_ds, tx);
+		dsl_dataset_rele(check_ds, FTAG);
+		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);
 }
@@ -1984,6 +2023,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/common.run b/tests/runfiles/common.run
@@ -804,7 +804,7 @@ 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_017_pos']
+    'snapshot_017_pos', 'snapshot_fail_existing.ksh']
 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