[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