From 371ae66e9d5c7524431773c4a479fcae1ea3b652 Mon Sep 17 00:00:00 2001 From: "Neal H. Walfield" Date: Tue, 30 Aug 2016 15:37:45 +0200 Subject: [PATCH] g10: Improve TOFU batch update code. * g10/gpg.h (tofu): Rename field batch_update_ref to batch_updated_wanted. * g10/tofu.c (struct tofu_dbs_s): Rename field batch_update to in_batch_transaction. (begin_transaction): Only end an extant batch transaction if we are not in a normal transaction. When ending a batch transaction, really end it. Update ctrl->tofu.batch_update_started when starting a batch transaction. (end_transaction): Only release a batch transaction if ONLY_BATCH is true. When releasing a batch transaction, assert that there is no open normal transaction. Only allow DBS to be NULL if ONLY_BATCH is true. (tofu_begin_batch_update): Don't update ctrl->tofu.batch_update_started. (opendbs): Call end_transaction unconditionally. -- Signed-off-by: Neal H. Walfield --- g10/gpg.h | 2 +- g10/tofu.c | 108 +++++++++++++++++++++++++++++++---------------------- 2 files changed, 64 insertions(+), 46 deletions(-) diff --git a/g10/gpg.h b/g10/gpg.h index 154da0de4..33a3af629 100644 --- a/g10/gpg.h +++ b/g10/gpg.h @@ -83,7 +83,7 @@ struct server_control_s struct { tofu_dbs_t dbs; int in_transaction; - int batch_update_ref; + int batch_updated_wanted; time_t batch_update_started; } tofu; diff --git a/g10/tofu.c b/g10/tofu.c index 338fb3e2e..f84609e7b 100644 --- a/g10/tofu.c +++ b/g10/tofu.c @@ -81,7 +81,7 @@ struct tofu_dbs_s sqlite3_stmt *register_insert; } s; - int batch_update; + int in_batch_transaction; }; @@ -169,26 +169,39 @@ begin_transaction (ctrl_t ctrl, int only_batch) log_assert (dbs); - if (ctrl->tofu.batch_update_ref + /* If we've been in batch update mode for a while (on average, more + * than 500 ms), to prevent starving other gpg processes, we drop + * and retake the batch lock. + * + * Note: if we wanted higher resolution, we could use + * npth_clock_gettime. */ + if (/* No real transactions. */ + ctrl->tofu.in_transaction == 0 + /* There is an open batch transaction. */ + && dbs->in_batch_transaction + /* And some time has gone by since it was started. */ && ctrl->tofu.batch_update_started != gnupg_get_time ()) { - /* We've been in batch update mode for a while (on average, more - * than 500 ms). To prevent starving other gpg processes, we - * drop and retake the batch lock. - * - * Note: if we wanted higher resolution, we could use - * npth_clock_gettime. */ - if (dbs->batch_update) - end_transaction (ctrl, 1); + /* If we are in a batch update, then batch updates better have + been enabled. */ + log_assert (ctrl->tofu.batch_updated_wanted); - ctrl->tofu.batch_update_started = gnupg_get_time (); + end_transaction (ctrl, 2); /* Yield to allow another process a chance to run. */ gpgrt_yield (); } - if (ctrl->tofu.batch_update_ref && !dbs->batch_update) + if (/* Batch mode is enabled. */ + ctrl->tofu.batch_updated_wanted + /* But we don't have an open batch transaction. */ + && !dbs->in_batch_transaction) { + /* We are in batch mode, but we don't have an open batch + * transaction. Since the batch save point must be the outer + * save point, it must be taken before the inner save point. */ + log_assert (ctrl->tofu.in_transaction == 0); + rc = gpgsql_stepx (dbs->db, &dbs->s.savepoint_batch, NULL, NULL, &err, "savepoint batch;", SQLITE_ARG_END); @@ -200,7 +213,8 @@ begin_transaction (ctrl_t ctrl, int only_batch) return gpg_error (GPG_ERR_GENERAL); } - dbs->batch_update = 1; + dbs->in_batch_transaction = 1; + ctrl->tofu.batch_update_started = gnupg_get_time (); } if (only_batch) @@ -235,35 +249,44 @@ end_transaction (ctrl_t ctrl, int only_batch) int rc; char *err = NULL; - if (!dbs) - return 0; /* Shortcut to allow for easier cleanup code. */ - - if ((!ctrl->tofu.batch_update_ref || only_batch == 2) && dbs->batch_update) + if (only_batch) { - /* The batch transaction is still in open, but we left batch - * mode. */ - dbs->batch_update = 0; + if (!dbs) + return 0; /* Shortcut to allow for easier cleanup code. */ - rc = gpgsql_stepx (dbs->db, &dbs->s.savepoint_batch_commit, - NULL, NULL, &err, - "release batch;", SQLITE_ARG_END); - if (rc) + /* If we are releasing the batch transaction, then we better not + be in a normal transaction. */ + log_assert (ctrl->tofu.in_transaction == 0); + + if (/* Batch mode disabled? */ + (!ctrl->tofu.batch_updated_wanted || only_batch == 2) + /* But, we still have an open batch transaction? */ + && dbs->in_batch_transaction) { - log_error (_("error committing transaction on TOFU database: %s\n"), - err); - sqlite3_free (err); - return gpg_error (GPG_ERR_GENERAL); + /* The batch transaction is still in open, but we've left + * batch mode. */ + dbs->in_batch_transaction = 0; + + rc = gpgsql_stepx (dbs->db, &dbs->s.savepoint_batch_commit, + NULL, NULL, &err, + "release batch;", SQLITE_ARG_END); + if (rc) + { + log_error (_("error committing transaction on TOFU database: %s\n"), + err); + sqlite3_free (err); + return gpg_error (GPG_ERR_GENERAL); + } + + return 0; } - /* Releasing an outer transaction releases an open inner - transactions. We're done. */ return 0; } - if (only_batch) - return 0; - + log_assert (dbs); log_assert (ctrl->tofu.in_transaction > 0); + rc = gpgsql_exec_printf (dbs->db, NULL, NULL, &err, "release inner%d;", ctrl->tofu.in_transaction); if (rc) @@ -287,8 +310,7 @@ rollback_transaction (ctrl_t ctrl) int rc; char *err = NULL; - if (!dbs) - return 0; /* Shortcut to allow for easier cleanup code. */ + log_assert (dbs); log_assert (ctrl->tofu.in_transaction > 0); /* Be careful to not any progress made by closed transactions in @@ -313,19 +335,16 @@ rollback_transaction (ctrl_t ctrl) void tofu_begin_batch_update (ctrl_t ctrl) { - if (!ctrl->tofu.batch_update_ref) - ctrl->tofu.batch_update_started = gnupg_get_time (); - - ctrl->tofu.batch_update_ref ++; + ctrl->tofu.batch_updated_wanted ++; } void tofu_end_batch_update (ctrl_t ctrl) { - log_assert (ctrl->tofu.batch_update_ref > 0); - ctrl->tofu.batch_update_ref --; + log_assert (ctrl->tofu.batch_updated_wanted > 0); + ctrl->tofu.batch_updated_wanted --; - if (!ctrl->tofu.batch_update_ref) + if (!ctrl->tofu.batch_updated_wanted) end_transaction (ctrl, 1); } @@ -693,14 +712,13 @@ tofu_closedbs (ctrl_t ctrl) tofu_dbs_t dbs; sqlite3_stmt **statements; - log_assert(ctrl->tofu.in_transaction == 0); + log_assert (ctrl->tofu.in_transaction == 0); dbs = ctrl->tofu.dbs; if (!dbs) return; /* Not initialized. */ - if (dbs->batch_update) - end_transaction (ctrl, 2); + end_transaction (ctrl, 2); /* Arghh, that is a surprising use of the struct. */ for (statements = (void *) &dbs->s;