From 0fdd6106bbd1b31322be87fee94c0c579ca459e6 Mon Sep 17 00:00:00 2001 From: Matthew Ahrens Date: Thu, 12 Mar 2020 10:55:02 -0700 Subject: dmu_objset_from_ds must be called with dp_config_rwlock held The normal lock order is that the dp_config_rwlock must be held before the ds_opening_lock. For example, dmu_objset_hold() does this. However, dmu_objset_open_impl() is called with the ds_opening_lock held, and if the dp_config_rwlock is not already held, it will attempt to acquire it. This may lead to deadlock, since the lock order is reversed. Looking at all the callers of dmu_objset_open_impl() (which is principally the callers of dmu_objset_from_ds()), almost all callers already have the dp_config_rwlock. However, there are a few places in the send and receive code paths that do not. For example: dsl_crypto_populate_key_nvlist, send_cb, dmu_recv_stream, receive_write_byref, redact_traverse_thread. This commit resolves the problem by requiring all callers ot dmu_objset_from_ds() to hold the dp_config_rwlock. In most cases, the code has been restructured such that we call dmu_objset_from_ds() earlier on in the send and receive processes, when we already have the dp_config_rwlock, and save the objset_t until we need it in the middle of the send or receive (similar to what we already do with the dsl_dataset_t). Thus we do not need to acquire the dp_config_rwlock in many new places. I also cleaned up code in dmu_redact_snap() and send_traverse_thread(). Reviewed-by: Paul Dagnelie Reviewed-by: Brian Behlendorf Reviewed-by: Paul Zuchowski Signed-off-by: Matthew Ahrens Closes #9662 Closes #10115 --- module/zfs/dmu_recv.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) (limited to 'module/zfs/dmu_recv.c') diff --git a/module/zfs/dmu_recv.c b/module/zfs/dmu_recv.c index 97a3c7cee..0fa0e015b 100644 --- a/module/zfs/dmu_recv.c +++ b/module/zfs/dmu_recv.c @@ -123,7 +123,7 @@ struct receive_writer_arg { typedef struct guid_map_entry { uint64_t guid; boolean_t raw; - dsl_dataset_t *gme_ds; + objset_t *gme_os; avl_node_t avlnode; } guid_map_entry_t; @@ -914,6 +914,7 @@ dmu_recv_begin_sync(void *arg, dmu_tx_t *tx) rrw_exit(&newds->ds_bp_rwlock, FTAG); drba->drba_cookie->drc_ds = newds; + drba->drba_cookie->drc_os = os; spa_history_log_internal_ds(newds, "receive", tx, " "); } @@ -1095,6 +1096,7 @@ dmu_recv_resume_begin_sync(void *arg, dmu_tx_t *tx) rrw_exit(&ds->ds_bp_rwlock, FTAG); drba->drba_cookie->drc_ds = ds; + VERIFY0(dmu_objset_from_ds(ds, &drba->drba_cookie->drc_os)); drba->drba_cookie->drc_should_save = B_TRUE; spa_history_log_internal_ds(ds, "resume receive", tx, " "); @@ -1227,11 +1229,12 @@ free_guid_map_onexit(void *arg) ds_hold_flags_t dsflags = DS_HOLD_FLAG_DECRYPT; if (gmep->raw) { - gmep->gme_ds->ds_objset->os_raw_receive = B_FALSE; + gmep->gme_os->os_raw_receive = B_FALSE; dsflags &= ~DS_HOLD_FLAG_DECRYPT; } - dsl_dataset_disown(gmep->gme_ds, dsflags, gmep); + dsl_dataset_disown(gmep->gme_os->os_dsl_dataset, + dsflags, gmep); kmem_free(gmep, sizeof (guid_map_entry_t)); } avl_destroy(ca); @@ -1797,8 +1800,7 @@ receive_write_byref(struct receive_writer_arg *rwa, &where)) == NULL) { return (SET_ERROR(EINVAL)); } - if (dmu_objset_from_ds(gmep->gme_ds, &ref_os)) - return (SET_ERROR(EINVAL)); + ref_os = gmep->gme_os; } else { ref_os = rwa->os; } @@ -2661,10 +2663,6 @@ dmu_recv_stream(dmu_recv_cookie_t *drc, int cleanup_fd, DMU_SUBSTREAM); ASSERT3U(drc->drc_drrb->drr_type, <, DMU_OST_NUMTYPES); - /* - * Open the objset we are modifying. - */ - VERIFY0(dmu_objset_from_ds(drc->drc_ds, &drc->drc_os)); ASSERT(dsl_dataset_phys(drc->drc_ds)->ds_flags & DS_FLAG_INCONSISTENT); ASSERT0(drc->drc_os->os_encrypted && (drc->drc_featureflags & DMU_BACKUP_FEATURE_EMBED_DATA)); @@ -3013,6 +3011,12 @@ dmu_recv_end_sync(void *arg, dmu_tx_t *tx) dsl_dataset_clone_swap_sync_impl(drc->drc_ds, origin_head, tx); + /* + * The objset was evicted by dsl_dataset_clone_swap_sync_impl, + * so drc_os is no longer valid. + */ + drc->drc_os = NULL; + dsl_dataset_snapshot_sync_impl(origin_head, drc->drc_tosnap, tx); @@ -3127,26 +3131,25 @@ add_ds_to_guidmap(const char *name, avl_tree_t *guid_map, uint64_t snapobj, err = dsl_dataset_own_obj(dp, snapobj, dsflags, gmep, &snapds); if (err == 0) { + err = dmu_objset_from_ds(snapds, &os); + if (err != 0) { + dsl_dataset_disown(snapds, dsflags, FTAG); + dsl_pool_rele(dp, FTAG); + kmem_free(gmep, sizeof (*gmep)); + return (err); + } /* * If this is a deduplicated raw send stream, we need * to make sure that we can still read raw blocks from * earlier datasets in the stream, so we set the * os_raw_receive flag now. */ - if (raw) { - err = dmu_objset_from_ds(snapds, &os); - if (err != 0) { - dsl_dataset_disown(snapds, dsflags, FTAG); - dsl_pool_rele(dp, FTAG); - kmem_free(gmep, sizeof (*gmep)); - return (err); - } + if (raw) os->os_raw_receive = B_TRUE; - } gmep->raw = raw; gmep->guid = dsl_dataset_phys(snapds)->ds_guid; - gmep->gme_ds = snapds; + gmep->gme_os = os; avl_add(guid_map, gmep); } else { kmem_free(gmep, sizeof (*gmep)); -- cgit v1.2.3