[PATCH] Slightly simplify g_lock_trylock
Volker Lendecke
Volker.Lendecke at SerNet.DE
Mon Aug 13 19:00:28 UTC 2018
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?
Thanks, Volker
--
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
-------------- next part --------------
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