[PATCH] Slightly simplify g_lock_trylock

Jeremy Allison jra at samba.org
Mon Aug 13 23:32:58 UTC 2018


On Mon, Aug 13, 2018 at 09:00:28PM +0200, Volker Lendecke via samba-technical wrote:
> Hi!
> 
> Array handling is hell (at least to me), so I did the attached patch
> as a proposal. To me it's easier to understand after this patch.
> 
> What do you think?

Wow. Took me a while to go through but I am now convinced
I understand it :-). The original code was.. dense :-).

RB+ from me, nice clean up and certainly makes this a
*LOT* easier to understand.

The only possible change I'd like is the addition of a
comment before the:

-               store_status = g_lock_store(rec, &lck, mylock);
+               store_status = g_lock_store(
+                       rec,
+                       &lck,
+                       mylock.pid.pid != 0 ? &mylock : NULL);

hunk explaining that using NULL as a parameter
to g_lock_store() means update without adding
any new record (just compacting the removed array
to get rid of any elements we removed above).

Would help any future reviewers I think.

But if you think that should be obvious from
the name of the 'add' parameter in the:

static NTSTATUS g_lock_store(struct db_record *rec, struct g_lock *lck,
                             struct g_lock_rec *add)

definition I'm OK without it.

Nice work !

Jeremy.

> -- 
> SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
> phone: +49-551-370000-0, fax: +49-551-370000-9
> AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
> http://www.sernet.de, mailto:kontakt at sernet.de
> 
> Meet us at Storage Developer Conference (SDC)
> Santa Clara, CA USA, September 24th-27th 2018

> From 78d0e0021c1f6e732234ba0a927a0a6f01da58d4 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 13 Aug 2018 14:12:47 +0200
> Subject: [PATCH 1/2] g_lock: Avoid a double call to serverid_exist
> 
> If we try to G_LOCK_READ while a G_LOCK_WRITE is active, we do the
> serverid_exists call twice. Avoid that.
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/lib/g_lock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
> index bffbd6bab4d..c824f4b0a58 100644
> --- a/source3/lib/g_lock.c
> +++ b/source3/lib/g_lock.c
> @@ -226,7 +226,8 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
>  
>  		g_lock_get_rec(&lck, i, &check_rec);
>  
> -		if (!serverid_exists(&check_rec.pid)) {
> +		if ((check_rec.lock_type == G_LOCK_READ) &&
> +		    !serverid_exists(&check_rec.pid)) {
>  			g_lock_rec_del(&lck, i);
>  			modified = true;
>  		}
> -- 
> 2.11.0
> 
> 
> From ba6c8834e9c5d2a9e7ec5b61f4c2df440a1441a7 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 13 Aug 2018 15:07:06 +0200
> Subject: [PATCH 2/2] g_lock: Simplify g_lock_trylock
> 
> While chasing a bug in g_lock (not in master) I saw some opportunity to
> simplify g_lock_trylock a bit. This is array handling, and array
> handling is just extremely error-prone. This *might* be a little less
> efficient or large numbers of READ locks, but this remains to be
> seen. For now, simplify the code.
> 
> First, we make two passes now: One to remove ourselves, and the other
> one to search for conflicts. Mixing up both made it pretty hard for me
> to follow the code.
> 
> Second, I've removed the _mylock and mylock pointer/struct logic and
> replaced it with the "mylock.pid.pid != 0 ? &mylock : NULL" when calling
> g_lock_store. To me, this focuses the logic whether to add ourselves in
> one place instead of spreading it around in the whole routine.
> 
> Signed-off-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/lib/g_lock.c | 48 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/source3/lib/g_lock.c b/source3/lib/g_lock.c
> index c824f4b0a58..8586e486da7 100644
> --- a/source3/lib/g_lock.c
> +++ b/source3/lib/g_lock.c
> @@ -200,8 +200,7 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
>  	TDB_DATA data;
>  	size_t i;
>  	struct g_lock lck;
> -	struct g_lock_rec _mylock;
> -	struct g_lock_rec *mylock = NULL;
> +	struct g_lock_rec mylock = {0};
>  	NTSTATUS status;
>  	bool modified = false;
>  	bool ok;
> @@ -233,9 +232,13 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
>  		}
>  	}
>  
> -	i = 0;
> +	/*
> +	 * For the lock upgrade/downgrade case, remove ourselves from
> +	 * the list. We re-add ourselves later after we checked the
> +	 * other entries for conflict.
> +	 */
>  
> -	while (i < lck.num_recs) {
> +	for (i=0; i<lck.num_recs; i++) {
>  		struct g_lock_rec lock;
>  
>  		g_lock_get_rec(&lck, i, &lock);
> @@ -245,20 +248,26 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
>  				status = NT_STATUS_WAS_LOCKED;
>  				goto done;
>  			}
> -			if (mylock != NULL) {
> -				status = NT_STATUS_INTERNAL_DB_CORRUPTION;
> -				goto done;
> -			}
> -			_mylock = lock;
> -			mylock = &_mylock;
> -			/*
> -			 * Remove "our" lock entry. Re-add it later
> -			 * with our new lock type.
> -			 */
> +
> +			mylock = lock;
>  			g_lock_rec_del(&lck, i);
>  			modified = true;
> -			continue;
> +			break;
>  		}
> +	}
> +
> +	/*
> +	 * Check for conflicts with everybody else. Not a for-loop
> +	 * because we remove stale entries in the meantime,
> +	 * decrementing lck.num_recs.
> +	 */
> +
> +	i = 0;
> +
> +	while (i < lck.num_recs) {
> +		struct g_lock_rec lock;
> +
> +		g_lock_get_rec(&lck, i, &lock);
>  
>  		if (g_lock_conflicts(type, lock.lock_type)) {
>  			struct server_id pid = lock.pid;
> @@ -288,18 +297,21 @@ static NTSTATUS g_lock_trylock(struct db_record *rec, struct server_id self,
>  
>  	modified = true;
>  
> -	_mylock = (struct g_lock_rec) {
> +	mylock = (struct g_lock_rec) {
>  		.pid = self,
>  		.lock_type = type
>  	};
> -	mylock = &_mylock;
>  
>  	status = NT_STATUS_OK;
>  done:
>  	if (modified) {
>  		NTSTATUS store_status;
>  
> -		store_status = g_lock_store(rec, &lck, mylock);
> +		store_status = g_lock_store(
> +			rec,
> +			&lck,
> +			mylock.pid.pid != 0 ? &mylock : NULL);
> +
>  		if (!NT_STATUS_IS_OK(store_status)) {
>  			DBG_WARNING("g_lock_record_store failed: %s\n",
>  				    nt_errstr(store_status));
> -- 
> 2.11.0
> 




More information about the samba-technical mailing list