[ovs-dev] [PATCH ovs] ovsdb idl: Try committing the pending txn in ovsdb_idl_loop_run.
Han Zhou
hzhou at ovn.org
Wed Jun 3 23:13:23 UTC 2020
On Wed, Jun 3, 2020 at 9:51 AM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at ovn.org>
>
> The function ovsdb_idl_loop_run(), after calling ovsdb_idl_run(),
> returns a transaction object (of type 'struct ovsdb_idl_txn').
> The returned transaction object can be NULL if there is a pending
> transaction (loop->committing_txn) in the idl loop object.
>
> Normally the clients of idl library, first call ovsdb_idl_loop_run(),
> then do their own processing and create any idl transactions during
> this processing and then finally call ovsdb_idl_loop_commit_and_wait().
>
> If ovsdb_idl_loop_run() returns NULL transaction object, then much
> of the processing done by the client gets wasted as in the case
> of ovn-controller.
>
> The client (in this case ovn-controller), can skip the processing
> and instead call ovsdb_idl_loop_commit_and_wait() if the transaction
> oject is NULL. But ovn-controller uses IDL tracking and it may
> loose the tracked changes in that run.
>
> This patch tries to improve this scenario, by checking if the
> pending transaction can be committed in the ovsdb_idl_loop_run()
> itself and if the pending transaction is cleared (because of the
> response messages from ovsdb-server due to a transaction message
> in the previous run), ovsdb_idl_loop_run() can return a valid
> transaction object.
Thanks Numan for addressing this. I think the patch is to trying to make
the TXN_SUCCESS take effect as early as when transaction "reply" is
received and handled in ovsdb_idl_loop_run(). The approach looks good to
me, but here is a comment below.
>
> CC: Han Zhou <hzhou at ovn.org>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
> lib/ovsdb-idl.c | 136 ++++++++++++++++++++++++++++++------------------
> 1 file changed, 85 insertions(+), 51 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index f54e360e3..400fa3077 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -385,6 +385,7 @@ static void ovsdb_idl_send_cond_change(struct
ovsdb_idl *idl);
> static void ovsdb_idl_destroy_indexes(struct ovsdb_idl_table *);
> static void ovsdb_idl_add_to_indexes(const struct ovsdb_idl_row *);
> static void ovsdb_idl_remove_from_indexes(const struct ovsdb_idl_row *);
> +static int ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop);
>
> static void
> ovsdb_idl_db_init(struct ovsdb_idl_db *db, const struct ovsdb_idl_class
*class,
> @@ -5340,6 +5341,12 @@ struct ovsdb_idl_txn *
> ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop)
> {
> ovsdb_idl_run(loop->idl);
> +
> + /* See if we can commit the loop->committing_txn. */
> + if (loop->committing_txn) {
> + ovsdb_idl_try_commit_loop_txn(loop);
This funciton calls poll_immediate_wake(). I think we shouldn't wake here.
It is necessary for ovsdb_idl_loop_commit_and_wait() but not here.
> + }
> +
> loop->open_txn = (loop->committing_txn
> || ovsdb_idl_get_seqno(loop->idl) ==
loop->skip_seqno
> ? NULL
> @@ -5347,6 +5354,83 @@ ovsdb_idl_loop_run(struct ovsdb_idl_loop *loop)
> return loop->open_txn;
> }
>
> +/* Attempts to commit the current transaction, if one is open.
> + *
> + * If a transaction was open, in this or a previous iteration of the
main loop,
> + * and had not before finished committing (successfully or
unsuccessfully), the
> + * return value is one of:
> + *
> + * 1: The transaction committed successfully (or it did not change
anything in
> + * the database).
> + * 0: The transaction failed.
> + * -1: The commit is still in progress.
> + *
> + * Thus, the return value is -1 if the transaction is in progress and
otherwise
> + * true for success, false for failure.
> + *
> + * (In the corner case where the IDL sends a transaction to the database
and
> + * the database commits it, and the connection between the IDL and the
database
> + * drops before the IDL receives the message confirming the commit, this
> + * function can return 0 even though the transaction succeeded.)
> + */
> +static int
> +ovsdb_idl_try_commit_loop_txn(struct ovsdb_idl_loop *loop)
> +{
> + if (!loop->committing_txn) {
> + /* Not a meaningful return value: no transaction was in
progress. */
> + return 1;
> + }
> +
> + int retval;
> + struct ovsdb_idl_txn *txn = loop->committing_txn;
> +
> + enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn);
> + if (status != TXN_INCOMPLETE) {
> + switch (status) {
> + case TXN_TRY_AGAIN:
> + /* We want to re-evaluate the database when it's changed from
> + * the contents that it had when we started the commit.
(That
> + * might have already happened.) */
> + loop->skip_seqno = loop->precommit_seqno;
> + if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) {
> + poll_immediate_wake();
> + }
> + retval = 0;
> + break;
> +
> + case TXN_SUCCESS:
> + /* Possibly some work on the database was deferred because no
> + * further transaction could proceed. Wake up again. */
> + retval = 1;
> + loop->cur_cfg = loop->next_cfg;
> + poll_immediate_wake();
> + break;
> +
> + case TXN_UNCHANGED:
> + retval = 1;
> + loop->cur_cfg = loop->next_cfg;
> + break;
> +
> + case TXN_ABORTED:
> + case TXN_NOT_LOCKED:
> + case TXN_ERROR:
> + retval = 0;
> + break;
> +
> + case TXN_UNCOMMITTED:
> + case TXN_INCOMPLETE:
> + default:
> + OVS_NOT_REACHED();
> + }
> + ovsdb_idl_txn_destroy(txn);
> + loop->committing_txn = NULL;
> + } else {
> + retval = -1;
> + }
> +
> + return retval;
> +}
> +
> /* Attempts to commit the current transaction, if one is open, and sets
up the
> * poll loop to wake up when some more work might be needed.
> *
> @@ -5377,57 +5461,7 @@ ovsdb_idl_loop_commit_and_wait(struct
ovsdb_idl_loop *loop)
> loop->precommit_seqno = ovsdb_idl_get_seqno(loop->idl);
> }
>
> - struct ovsdb_idl_txn *txn = loop->committing_txn;
> - int retval;
> - if (txn) {
> - enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn);
> - if (status != TXN_INCOMPLETE) {
> - switch (status) {
> - case TXN_TRY_AGAIN:
> - /* We want to re-evaluate the database when it's changed
from
> - * the contents that it had when we started the commit.
(That
> - * might have already happened.) */
> - loop->skip_seqno = loop->precommit_seqno;
> - if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) {
> - poll_immediate_wake();
> - }
> - retval = 0;
> - break;
> -
> - case TXN_SUCCESS:
> - /* Possibly some work on the database was deferred
because no
> - * further transaction could proceed. Wake up again. */
> - retval = 1;
> - loop->cur_cfg = loop->next_cfg;
> - poll_immediate_wake();
> - break;
> -
> - case TXN_UNCHANGED:
> - retval = 1;
> - loop->cur_cfg = loop->next_cfg;
> - break;
> -
> - case TXN_ABORTED:
> - case TXN_NOT_LOCKED:
> - case TXN_ERROR:
> - retval = 0;
> - break;
> -
> - case TXN_UNCOMMITTED:
> - case TXN_INCOMPLETE:
> - default:
> - OVS_NOT_REACHED();
> - }
> - ovsdb_idl_txn_destroy(txn);
> - loop->committing_txn = NULL;
> - } else {
> - retval = -1;
> - }
> - } else {
> - /* Not a meaningful return value: no transaction was in
progress. */
> - retval = 1;
> - }
> -
> + int retval = ovsdb_idl_try_commit_loop_txn(loop);
> ovsdb_idl_wait(loop->idl);
>
> return retval;
> --
> 2.26.2
>
More information about the dev
mailing list