[PATCH] Make tdb transaction lock recursive (samba version)

[Michael Adam]

Pushed to master - thanks!

Rusty Russell wrote:
> This patch replaces 6ed27edbcd3ba1893636a8072c8d7a621437daf7 and
> 1a416ff13ca7786f2e8d24c66addf00883e9cb12, which fixed the bug where traversals
> inside transactions would release the transaction lock early.
> 
> This solution is more general, and solves the more minor symptom that nested
> traversals would also release the transaction lock early.  (It was also suggestd in
> Volker's comment in 6ed27ed).
> 
> This patch also applies to ctdb, if the traverse.c part is removed (ctdb's tdb
> code never received the previous two fixes).
> 
> Tested using the testsuite from ccan (adapted to the samba code).  Thanks to
> Michael Adam for feedback.
> 
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> 
> diff --git a/lib/tdb/common/lock.c b/lib/tdb/common/lock.c
> index f156c0f..d812fbf 100644
> --- a/lib/tdb/common/lock.c
> +++ b/lib/tdb/common/lock.c
> @@ -301,16 +301,21 @@ int tdb_unlock(struct tdb_context *tdb, int list, int ltype)
>   */
>  int tdb_transaction_lock(struct tdb_context *tdb, int ltype)
>  {
> -	if (tdb->have_transaction_lock || tdb->global_lock.count) {
> +	if (tdb->global_lock.count) {
> +		return 0;
> +	}
> +	if (tdb->transaction_lock_count > 0) {
> +		tdb->transaction_lock_count++;
>  		return 0;
>  	}
> +
>  	if (tdb->methods->tdb_brlock(tdb, TRANSACTION_LOCK, ltype, 
>  				     F_SETLKW, 0, 1) == -1) {
>  		TDB_LOG((tdb, TDB_DEBUG_ERROR, "tdb_transaction_lock: failed to get transaction lock\n"));
>  		tdb->ecode = TDB_ERR_LOCK;
>  		return -1;
>  	}
> -	tdb->have_transaction_lock = 1;
> +	tdb->transaction_lock_count++;
>  	return 0;
>  }
>  
> @@ -320,12 +325,16 @@ int tdb_transaction_lock(struct tdb_context *tdb, int ltype)
>  int tdb_transaction_unlock(struct tdb_context *tdb)
>  {
>  	int ret;
> -	if (!tdb->have_transaction_lock) {
> +	if (tdb->global_lock.count) {
> +		return 0;
> +	}
> +	if (tdb->transaction_lock_count > 0) {
> +		tdb->transaction_lock_count--;
>  		return 0;
>  	}
>  	ret = tdb->methods->tdb_brlock(tdb, TRANSACTION_LOCK, F_UNLCK, F_SETLKW, 0, 1);
>  	if (ret == 0) {
> -		tdb->have_transaction_lock = 0;
> +		tdb->transaction_lock_count = 0;
>  	}
>  	return ret;
>  }
> diff --git a/lib/tdb/common/tdb_private.h b/lib/tdb/common/tdb_private.h
> index ffac89f..b9a1145 100644
> --- a/lib/tdb/common/tdb_private.h
> +++ b/lib/tdb/common/tdb_private.h
> @@ -166,7 +168,7 @@ struct tdb_context {
>  	struct tdb_transaction *transaction;
>  	int page_size;
>  	int max_dead_records;
> -	bool have_transaction_lock;
> +	int transaction_lock_count;
>  	volatile sig_atomic_t *interrupt_sig_ptr;
>  };
>  
> diff --git a/lib/tdb/common/traverse.c b/lib/tdb/common/traverse.c
> index 69c81e6..07b0c23 100644
> --- a/lib/tdb/common/traverse.c
> +++ b/lib/tdb/common/traverse.c
> @@ -204,23 +204,18 @@ int tdb_traverse_read(struct tdb_context *tdb,
>  {
>  	struct tdb_traverse_lock tl = { NULL, 0, 0, F_RDLCK };
>  	int ret;
> -	bool in_transaction = (tdb->transaction != NULL);
>  
>  	/* we need to get a read lock on the transaction lock here to
>  	   cope with the lock ordering semantics of solaris10 */
> -	if (!in_transaction) {
> -		if (tdb_transaction_lock(tdb, F_RDLCK)) {
> -			return -1;
> -		}
> +	if (tdb_transaction_lock(tdb, F_RDLCK)) {
> +		return -1;
>  	}
>  
>  	tdb->traverse_read++;
>  	ret = tdb_traverse_internal(tdb, fn, private_data, &tl);
>  	tdb->traverse_read--;
>  
> -	if (!in_transaction) {
> -		tdb_transaction_unlock(tdb);
> -	}
> +	tdb_transaction_unlock(tdb);
>  
>  	return ret;
>  }
> @@ -237,25 +232,20 @@ int tdb_traverse(struct tdb_context *tdb,
>  {
>  	struct tdb_traverse_lock tl = { NULL, 0, 0, F_WRLCK };
>  	int ret;
> -	bool in_transaction = (tdb->transaction != NULL);
>  
>  	if (tdb->read_only || tdb->traverse_read) {
>  		return tdb_traverse_read(tdb, fn, private_data);
>  	}
>  	
> -	if (!in_transaction) {
> -		if (tdb_transaction_lock(tdb, F_WRLCK)) {
> -			return -1;
> -		}
> +	if (tdb_transaction_lock(tdb, F_WRLCK)) {
> +		return -1;
>  	}
>  
>  	tdb->traverse_write++;
>  	ret = tdb_traverse_internal(tdb, fn, private_data, &tl);
>  	tdb->traverse_write--;
>  
> -	if (!in_transaction) {
> -		tdb_transaction_unlock(tdb);
> -	}
> +	tdb_transaction_unlock(tdb);
>  
>  	return ret;
>  }
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20090720/81eb97e5/attachment.pgp>