Re: [Tarantool-patches] [PATCH v14 4/6] qsync: order access to the limbo terms

[Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Sun, Sep 12, 2021 at 05:44:11PM +0200, Vladislav Shpilevoy wrote:
> >  err:
> > +	txn_limbo_term_unlock(&txn_limbo);
> >  	diag_log();
> 
> 1. This function can go to 'err' before the lock is taken.

yup, thanks!

> 
> 
> 2. As for your complaint about the begin/commit/rollback API
> being not working because you can't unlock from a non-owner
> fiber - well, it works in your patch somehow, doesn't it?

Yes, it works with my patch because journal_write() are ordered,
we take and release lock explicitly inside caller code, ie inside
one same fiber(). Imagine two appliers running simultaneously

applier 1                       applier 2                       sched
---------                       ---------                       -----
apply_synchro_row
  txn_limbo_term_lock
    journal_write
      context-switch -->        apply_synchro_row
                                  txn_limbo_term_lock
                                    wait for release
                                                                apply_synchro_row_cb
                                                                context-switch
  txn_limbo_term_unlock <--                                     --+
  return
                                txn_limbo_term_lock finishes
                                ...

> Why do you in your patch unlock here, but in the newly proposed
> API you only tried to unlock in the trigger?

Because you proposed to *hide* locking completely inside limbo code,
so the caller won't know anything about it. So our commit/rollback
would handle locking internally, unfortunately this doesn't work.

> 
> You could call commit/rollback from this function, like you
> do with unlock now.

Not sure if I follow you here. Our journal engine implies completions
to be called. We pass such completion into journal entry creation. With
my patch everything remains as is except we take a lock explicitly and
release it then. Could you please point more explicitly what you've in mind?

> > +
> > +/** Fetch replica's term with lock taken. */
> > +static inline uint64_t
> > +txn_limbo_replica_term_locked(struct txn_limbo *limbo, uint32_t replica_id)
> 
> 3. Limbo can be made const here.

ok