Re: [Tarantool-patches] [PATCH v10 2/4] limbo: order access to the limbo terms

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 06.08.2021 18:20, Cyrill Gorcunov wrote:
> On Fri, Aug 06, 2021 at 01:29:57AM +0200, Vladislav Shpilevoy wrote:
>>>  apply_synchro_row(uint32_t replica_id, struct xrow_header *row)
>>>  {
>>>  	assert(iproto_type_is_synchro_request(row->type));
>>> +	int rc = 0;
>>>  
>>>  	struct synchro_request req;
>>>  	if (xrow_decode_synchro(row, &req) != 0)
>>>  		goto err;
>>>  
>>> +	txn_limbo_term_lock(&txn_limbo);
>>
>> Maybe you should hide the lock from the API. Instead, do similar to
>> what transactions do:
>>
>> 	int txn_limbo_process_begin(limbo *);
>> 	void txn_limbo_process_commit(limbo *, request *);
>> 	void txn_limbo_process_rollback(limbo *);
>>
>> begin would take the lock, commit would do the request and
>> unlock, rollback would only unlock. Commit and rollback you
>> call from apply_synchro_row_cb depend in on the WAL write
>> result.
>>
>> Then the locks would disappear from the API, right?
> 
> Unfortunatelly locking is needed not only for processing but
> for reading terms as well. We have a few helpers more which
> are waiting the other fibers to complete before reading terms.
> 
> applier_apply_tx
>   applier_synchro_filter_tx
>     txn_limbo_is_replica_outdated
>       txn_limbo_term_lock
>         txn_limbo_replica_term_locked
>       txn_limbo_term_unlock
> 
> And a number of calls for txn_limbo_replica_term which reads
> term in a locked way because we need to eliminate potential
> race here and fetch only last written data.
> 
> So no, locking won't disappear. Another option may be to
> introduce preemption disabling (just like kernel does for
> tasks which should not be rescheduled on a core while
> they are wating for some action to complete). Then our
> write for synchro packets would look like
> 
> 	preempt_disable();
> 	rc = journal_write();
> 	preempt_enable();
> 
> which would guarantee us that while we're waiting the journal
> to finish its write no other fibers from the cord will be
> executed and we gotta be woken up once write is complete.
> 
> This way I think we will be allowed to drop locking at all
> because main problem is exactly because of other fibers get
> running while we're writing synchro data.
> 
>> In the next patch you would make txn_limbo_process_begin()
>> also take the request to validate it. Then the 'filtering'
>> would become internal to the limbo.

I didn't propose to drop the locking. I said it could be hidden
inside of the limbo's API. In the only example above you show:

>       txn_limbo_term_lock
>         txn_limbo_replica_term_locked
>       txn_limbo_term_unlock

Here the lock is done inside of the limbo's API too. It is
not exposed on the limbo's API level. So the questions is the
same - can it be hidden inside of the API? Are there any usages
of the lock done explicitly out of the limo?