Skip to content
Permalink
Browse files

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 <pzuchowski@datto.com>
Fixes #8380
  • Loading branch information...
PaulZ-98 committed Sep 24, 2019
1 parent 7b50929 commit 4566858c1a738785e68b6a3d1f5c2bd8d6241ef2
@@ -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);

@@ -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);
}
@@ -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
@@ -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)
goto out;

/* Now reserve space since we passed all the checks. */
for (check_ds = list_head(&datasets); check_ds != NULL;
check_ds = list_next(&datasets, check_ds)) {
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);
if (error != 0)
rv = error;
}
if (rv == 0)
goto out;

ASSERT3U(rv, ==, ENOSPC);

/* Undo the reserved space since no snapshot will be taken. */
for (check_ds = list_head(&datasets); check_ds != NULL;
check_ds = list_next(&datasets, check_ds)) {
dsl_dir_sync(check_ds->ds_dir, tx);
}

out:
while ((check_ds = list_remove_head(&datasets)) != NULL) {
dsl_dataset_rele(check_ds, FTAG);
}
list_destroy(&datasets);

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);
@@ -860,7 +860,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]
@@ -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

0 comments on commit 4566858

Please sign in to comment.
You can’t perform that action at this time.