[ovs-dev] [PATCH] ovn-controller: Avoid overlooking changes that occur during commit.

[Ben Pfaff]

Thanks Alex, I applied this to master.

On Tue, Jul 28, 2015 at 04:01:13PM -0700, Alex Wang wrote:
> Thx for fixing this~!
> 
> Acked-by: Alex Wang <alexw at nicira.com>
> 
> On Tue, Jul 28, 2015 at 1:41 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > A commit to the database takes multiple trips through the main loop.  In
> > that time, the database could change, but ovn-controller can't properly
> > react to the changes until the commit has succeeded or failed.  Since
> > commit f1fd765733 (ovn-controller: Avoid blocking to commit OVSDB
> > transactions), Open vSwitch has failed to properly re-check the contents
> > of the database following a successful commit.  That meant that it was
> > possible for ovn-controller to fail to react to a database change until
> > much later, if nothing else happened for some time.
> >
> > Reported-by; Alex Wang <alexw at nicira.com>
> > Reported-at: http://openvswitch.org/pipermail/dev/2015-July/058176.html
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  ovn/controller/ovn-controller.c | 48
> > +++++++++++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/ovn/controller/ovn-controller.c
> > b/ovn/controller/ovn-controller.c
> > index 0657140..affc14b 100644
> > --- a/ovn/controller/ovn-controller.c
> > +++ b/ovn/controller/ovn-controller.c
> > @@ -189,28 +189,40 @@ idl_loop_commit_and_wait(struct idl_loop *loop)
> >      struct ovsdb_idl_txn *txn = loop->committing_txn;
> >      if (txn) {
> >          enum ovsdb_idl_txn_status status = ovsdb_idl_txn_commit(txn);
> > -        switch (status) {
> > -        case TXN_INCOMPLETE:
> > -            break;
> > +        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();
> > +                }
> > +                break;
> > +
> > +            case TXN_SUCCESS:
> > +                /* If the database has already changed since we started
> > the
> > +                 * commit, re-evaluate it immediately to avoid missing a
> > change
> > +                 * for a while. */
> > +                if (ovsdb_idl_get_seqno(loop->idl) !=
> > loop->precommit_seqno) {
> > +                    poll_immediate_wake();
> > +                }
> > +                break;
> > +
> > +            case TXN_UNCHANGED:
> > +            case TXN_ABORTED:
> > +            case TXN_NOT_LOCKED:
> > +            case TXN_ERROR:
> > +                break;
> > +
> > +            case TXN_UNCOMMITTED:
> > +            case TXN_INCOMPLETE:
> > +                OVS_NOT_REACHED();
> >
> > -        case TXN_TRY_AGAIN:
> > -            loop->skip_seqno = loop->precommit_seqno;
> > -            if (ovsdb_idl_get_seqno(loop->idl) != loop->skip_seqno) {
> > -                poll_immediate_wake();
> >              }
> > -            /* Fall through. */
> > -        case TXN_UNCHANGED:
> > -        case TXN_ABORTED:
> > -        case TXN_SUCCESS:
> > -        case TXN_NOT_LOCKED:
> > -        case TXN_ERROR:
> >              ovsdb_idl_txn_destroy(txn);
> >              loop->committing_txn = NULL;
> > -            break;
> > -
> > -        case TXN_UNCOMMITTED:
> > -            OVS_NOT_REACHED();
> > -
> >          }
> >      }
> >
> > --
> > 2.1.3
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >