From 7b97c6b0971bdc07c6f79c4778f70c42320217c4 Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Thu, 12 Aug 2010 15:33:57 -0700 Subject: [PATCH 1/3] Add fix-commit-callback topic branch --- .topdeps | 1 + .topmsg | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+) create mode 100644 .topdeps create mode 100644 .topmsg diff --git a/.topdeps b/.topdeps new file mode 100644 index 0000000000..1f7391f92b --- /dev/null +++ b/.topdeps @@ -0,0 +1 @@ +master diff --git a/.topmsg b/.topmsg new file mode 100644 index 0000000000..a411500761 --- /dev/null +++ b/.topmsg @@ -0,0 +1,22 @@ +From: Ricardo M. Correia +Subject: [PATCH] fix commit callback + +The upstream commit cb code had a few bugs: + +1) The arguments of the list_move_tail() call in txg_dispatch_callbacks() +were reversed by mistake. This caused the commit callbacks to not be +called at all. + +2) ztest had a bug in ztest_dmu_commit_callbacks() where "error" was not +initialized correctly. This seems to have caused the test to always take +the simulated error code path, which made ztest unable to detect whether +commit cbs were being called for transactions that successfuly complete. + +3) ztest had another bug in ztest_dmu_commit_callbacks() where the commit +cb threshold was not being compared correctly. + +4) The commit cb taskq was using 'max_ncpus * 2' as the maxalloc argument +of taskq_create(), which could have caused unnecessary delays in the txg +sync thread. + +Signed-off-by: Brian Behlendorf From 8af066465261db695aabd26c4e453fa9d666eab2 Mon Sep 17 00:00:00 2001 From: "Ricardo M. Correia" Date: Thu, 12 Aug 2010 14:25:13 -0700 Subject: [PATCH 2/3] Fix upstream commit callback The upstream commit cb code had a few bugs: 1) The arguments of the list_move_tail() call in txg_dispatch_callbacks() were reversed by mistake. This caused the commit callbacks to not be called at all. 2) ztest had a bug in ztest_dmu_commit_callbacks() where "error" was not initialized correctly. This seems to have caused the test to always take the simulated error code path, which made ztest unable to detect whether commit cbs were being called for transactions that successfuly complete. 3) ztest had another bug in ztest_dmu_commit_callbacks() where the commit cb threshold was not being compared correctly. 4) The commit cb taskq was using 'max_ncpus * 2' as the maxalloc argument of taskq_create(), which could have caused unnecessary delays in the txg sync thread. Signed-off-by: Brian Behlendorf --- cmd/ztest/ztest.c | 47 ++++++++++++++++++++++++++++++++--------------- module/zfs/txg.c | 6 +++--- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/cmd/ztest/ztest.c b/cmd/ztest/ztest.c index eed92ec72e..2499362b0f 100644 --- a/cmd/ztest/ztest.c +++ b/cmd/ztest/ztest.c @@ -341,6 +341,22 @@ static boolean_t ztest_exiting; /* Global commit callback list */ static ztest_cb_list_t zcl; +/* Commit cb delay */ +static uint64_t zc_min_txg_delay = UINT64_MAX; +static int zc_cb_counter = 0; + +/* + * Minimum number of commit callbacks that need to be registered for us to check + * whether the minimum txg delay is acceptable. + */ +#define ZTEST_COMMIT_CB_MIN_REG 100 + +/* + * If a number of txgs equal to this threshold have been created after a commit + * callback has been registered but not called, then we assume there is an + * implementation bug. + */ +#define ZTEST_COMMIT_CB_THRESH (TXG_CONCURRENT_STATES + 1000) extern uint64_t metaslab_gang_bang; extern uint64_t metaslab_df_alloc_threshold; @@ -4063,18 +4079,20 @@ ztest_commit_callback(void *arg, int error) return; } - /* Was this callback added to the global callback list? */ - if (!data->zcd_added) - goto out; - + ASSERT(data->zcd_added); ASSERT3U(data->zcd_txg, !=, 0); - /* Remove our callback from the list */ (void) mutex_lock(&zcl.zcl_callbacks_lock); + + /* See if this cb was called more quickly */ + if ((synced_txg - data->zcd_txg) < zc_min_txg_delay) + zc_min_txg_delay = synced_txg - data->zcd_txg; + + /* Remove our callback from the list */ list_remove(&zcl.zcl_callbacks, data); + (void) mutex_unlock(&zcl.zcl_callbacks_lock); -out: umem_free(data, sizeof (ztest_cb_data_t)); } @@ -4092,13 +4110,6 @@ ztest_create_cb_data(objset_t *os, uint64_t txg) return (cb_data); } -/* - * If a number of txgs equal to this threshold have been created after a commit - * callback has been registered but not called, then we assume there is an - * implementation bug. - */ -#define ZTEST_COMMIT_CALLBACK_THRESH (TXG_CONCURRENT_STATES + 2) - /* * Commit callback test. */ @@ -4110,7 +4121,7 @@ ztest_dmu_commit_callbacks(ztest_ds_t *zd, uint64_t id) dmu_tx_t *tx; ztest_cb_data_t *cb_data[3], *tmp_cb; uint64_t old_txg, txg; - int i, error; + int i, error = 0; ztest_od_init(&od[0], id, FTAG, 0, DMU_OT_UINT64_OTHER, 0, 0); @@ -4190,7 +4201,7 @@ ztest_dmu_commit_callbacks(ztest_ds_t *zd, uint64_t id) */ tmp_cb = list_head(&zcl.zcl_callbacks); if (tmp_cb != NULL && - tmp_cb->zcd_txg > txg - ZTEST_COMMIT_CALLBACK_THRESH) { + tmp_cb->zcd_txg + ZTEST_COMMIT_CB_THRESH < txg) { fatal(0, "Commit callback threshold exceeded, oldest txg: %" PRIu64 ", open txg: %" PRIu64 "\n", tmp_cb->zcd_txg, txg); } @@ -4221,6 +4232,8 @@ ztest_dmu_commit_callbacks(ztest_ds_t *zd, uint64_t id) tmp_cb = cb_data[i]; } + zc_cb_counter += 3; + (void) mutex_unlock(&zcl.zcl_callbacks_lock); dmu_tx_commit(tx); @@ -5219,6 +5232,10 @@ ztest_run(ztest_shared_t *zs) for (uint64_t object = 1; object < 50; object++) dmu_prefetch(spa->spa_meta_objset, object, 0, 1ULL << 20); + /* Verify that at least one commit cb was called in a timely fashion */ + if (zc_cb_counter >= ZTEST_COMMIT_CB_MIN_REG) + VERIFY3U(zc_min_txg_delay, ==, 0); + spa_close(spa, FTAG); /* diff --git a/module/zfs/txg.c b/module/zfs/txg.c index f478ad0c67..7d84aa1aee 100644 --- a/module/zfs/txg.c +++ b/module/zfs/txg.c @@ -335,15 +335,15 @@ txg_dispatch_callbacks(dsl_pool_t *dp, uint64_t txg) * Commit callback taskq hasn't been created yet. */ tx->tx_commit_cb_taskq = taskq_create("tx_commit_cb", - max_ncpus, minclsyspri, max_ncpus, max_ncpus * 2, - TASKQ_PREPOPULATE); + 100, minclsyspri, max_ncpus, INT_MAX, + TASKQ_THREADS_CPU_PCT | TASKQ_PREPOPULATE); } cb_list = kmem_alloc(sizeof (list_t), KM_SLEEP); list_create(cb_list, sizeof (dmu_tx_callback_t), offsetof(dmu_tx_callback_t, dcb_node)); - list_move_tail(&tc->tc_callbacks[g], cb_list); + list_move_tail(cb_list, &tc->tc_callbacks[g]); (void) taskq_dispatch(tx->tx_commit_cb_taskq, (task_func_t *) txg_do_callbacks, cb_list, TQ_SLEEP); From eca561bddb4ad7f4c06e9c0f1976eeb6397f5cfc Mon Sep 17 00:00:00 2001 From: Brian Behlendorf Date: Thu, 12 Aug 2010 15:36:24 -0700 Subject: [PATCH 3/3] New TopGit dependency: fix-commit-callback --- .topdeps | 1 + 1 file changed, 1 insertion(+) diff --git a/.topdeps b/.topdeps index 24146e7fc9..dfcc9e73d2 100644 --- a/.topdeps +++ b/.topdeps @@ -30,3 +30,4 @@ fix-stack-dsl_scan_visitbp fix-stack-dbuf_hold_impl fix-bpobj_close fix-zfs_ioc_objset_stats +fix-commit-callback