[PATCHSET] Simplify brl_get_locks logic

Volker Lendecke Volker.Lendecke at SerNet.DE
Sun Oct 6 08:14:41 MDT 2013


Hi!

Attached find a patchset that simplifies the brl_get_locks
functionality. The existing code is pretty complex with
regards to the readonly code.

Please review & push.

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

*****************************************************************
visit us on it-sa:IT security exhibitions in Nürnberg, Germany
October 8th - 10th 2013, hall 12, booth 333
free tickets available via code 270691 on: www.it-sa.de/gutschein
******************************************************************
-------------- next part --------------
From a991df14da62d7c8fcd4702e203a876fe9b5c79e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 11 Sep 2013 10:17:05 +0000
Subject: [PATCH 1/5] smbd: Avoid an if-statement per read/write in the
 non-clustered case

Without clustering, fsp->brlock_rec will never be set anyway. In the
clustering case we can't use the seqnum trick, so this is slow enough
that the additional if-statement does not matter in this case anyway. In
the non-clustered case it might. Have not measured it, but every little
bit helps I guess.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/locking/brlock.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index ee4354c..a0b94cd 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -2062,15 +2062,15 @@ struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
 {
 	struct byte_range_lock *br_lock;
 
-	if (lp_clustering()) {
-		return brl_get_locks_internal(talloc_tos(), fsp, true);
-	}
-
 	if ((fsp->brlock_rec != NULL)
 	    && (dbwrap_get_seqnum(brlock_db) == fsp->brlock_seqnum)) {
 		return fsp->brlock_rec;
 	}
 
+	if (lp_clustering()) {
+		return brl_get_locks_internal(talloc_tos(), fsp, true);
+	}
+
 	TALLOC_FREE(fsp->brlock_rec);
 
 	br_lock = brl_get_locks_internal(talloc_tos(), fsp, true);
-- 
1.7.9.5


From 18578de701dea790f6f2882719e970e6c2518f85 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 11 Sep 2013 11:36:54 +0000
Subject: [PATCH 2/5] smbd: Restructure brl_get_locks_readonly

This is step 1 to get rid of brl_get_locks_internal with its complex readonly
business. It also optimizes 2 things: First, it uses dbwrap_parse_record to
avoid a talloc and memcpy, and second it uses talloc_pooled_object.

And -- hopefully it is easier to understand the caching logic with
fsp->brlock_rec and the clustering escape.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/locking/brlock.c |  113 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 103 insertions(+), 10 deletions(-)

diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index a0b94cd..e0335dc 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -2058,30 +2058,123 @@ struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx,
 	return brl_get_locks_internal(mem_ctx, fsp, False);
 }
 
-struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
+struct brl_get_locks_readonly_state {
+	TALLOC_CTX *mem_ctx;
+	struct byte_range_lock **br_lock;
+};
+
+static void brl_get_locks_readonly_parser(TDB_DATA key, TDB_DATA data,
+					  void *private_data)
 {
+	struct brl_get_locks_readonly_state *state =
+		(struct brl_get_locks_readonly_state *)private_data;
 	struct byte_range_lock *br_lock;
 
+	br_lock = talloc_pooled_object(
+		state->mem_ctx, struct byte_range_lock, 1, data.dsize);
+	if (br_lock == NULL) {
+		*state->br_lock = NULL;
+		return;
+	}
+	br_lock->lock_data = (struct lock_struct *)talloc_memdup(
+		br_lock, data.dptr, data.dsize);
+	br_lock->num_locks = data.dsize / sizeof(struct lock_struct);
+
+	*state->br_lock = br_lock;
+}
+
+struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
+{
+	struct byte_range_lock *br_lock = NULL;
+	struct byte_range_lock *rw = NULL;
+
 	if ((fsp->brlock_rec != NULL)
 	    && (dbwrap_get_seqnum(brlock_db) == fsp->brlock_seqnum)) {
+		/*
+		 * We have cached the brlock_rec and the database did not
+		 * change.
+		 */
 		return fsp->brlock_rec;
 	}
 
-	if (lp_clustering()) {
-		return brl_get_locks_internal(talloc_tos(), fsp, true);
+	if (!fsp->lockdb_clean) {
+		/*
+		 * Fetch the record in R/W mode to give validate_lock_entries
+		 * a chance to kick in once.
+		 */
+		rw = brl_get_locks_internal(talloc_tos(), fsp, false);
+		if (rw == NULL) {
+			return NULL;
+		}
+		fsp->lockdb_clean = true;
 	}
 
-	TALLOC_FREE(fsp->brlock_rec);
+	if (rw != NULL) {
+		size_t lock_data_size;
 
-	br_lock = brl_get_locks_internal(talloc_tos(), fsp, true);
-	if (br_lock == NULL) {
-		return NULL;
+		/*
+		 * Make a copy of the already retrieved and sanitized rw record
+		 */
+		lock_data_size = rw->num_locks * sizeof(struct lock_struct);
+		br_lock = talloc_pooled_object(
+			fsp, struct byte_range_lock, 1, lock_data_size);
+		if (br_lock == NULL) {
+			goto fail;
+		}
+		br_lock->num_locks = rw->num_locks;
+		br_lock->lock_data = (struct lock_struct *)talloc_memdup(
+			br_lock, rw->lock_data, lock_data_size);
+	} else {
+		struct brl_get_locks_readonly_state state;
+		NTSTATUS status;
+
+		/*
+		 * Parse the record fresh from the database
+		 */
+
+		state.mem_ctx = fsp;
+		state.br_lock = &br_lock;
+
+		status = dbwrap_parse_record(
+			brlock_db,
+			make_tdb_data((uint8_t *)&fsp->file_id,
+				      sizeof(fsp->file_id)),
+			brl_get_locks_readonly_parser, &state);
+		if (!NT_STATUS_IS_OK(status)) {
+			DEBUG(3, ("Could not parse byte range lock record: "
+				  "%s\n", nt_errstr(status)));
+			goto fail;
+		}
+		if (br_lock == NULL) {
+			goto fail;
+		}
 	}
-	fsp->brlock_seqnum = dbwrap_get_seqnum(brlock_db);
 
-	fsp->brlock_rec = talloc_move(fsp, &br_lock);
+	br_lock->fsp = fsp;
+	br_lock->modified = false;
+	br_lock->read_only = true;
+	br_lock->record = NULL;
+
+	if (lp_clustering()) {
+		/*
+		 * In the cluster case we can't cache the brlock struct
+		 * because dbwrap_get_seqnum does not work reliably over
+		 * ctdb. Thus we have to throw away the brlock struct soon.
+		 */
+		talloc_steal(talloc_tos(), br_lock);
+	} else {
+		/*
+		 * Cache the brlock struct, invalidated when the dbwrap_seqnum
+		 * changes. See beginning of this routine.
+		 */
+		TALLOC_FREE(fsp->brlock_rec);
+		fsp->brlock_rec = br_lock;
+		fsp->brlock_seqnum = dbwrap_get_seqnum(brlock_db);
+	}
 
-	return fsp->brlock_rec;
+fail:
+	TALLOC_FREE(rw);
+	return br_lock;
 }
 
 struct brl_revalidate_state {
-- 
1.7.9.5


From 84d07dd133390df8e41cb0512319fdd97aef37b0 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 11 Sep 2013 11:51:44 +0000
Subject: [PATCH 3/5] smbd: brl_get_locks_internal is always called r/w now

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/locking/brlock.c |   48 +++++++++++-----------------------------------
 1 file changed, 11 insertions(+), 37 deletions(-)

diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index e0335dc..f57c7e5 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -1930,11 +1930,10 @@ static int byte_range_lock_destructor(struct byte_range_lock *br_lck)
 ********************************************************************/
 
 static struct byte_range_lock *brl_get_locks_internal(TALLOC_CTX *mem_ctx,
-					files_struct *fsp, bool read_only)
+						      files_struct *fsp)
 {
 	TDB_DATA key, data;
 	struct byte_range_lock *br_lck = talloc(mem_ctx, struct byte_range_lock);
-	bool do_read_only = read_only;
 
 	if (br_lck == NULL) {
 		return NULL;
@@ -1947,40 +1946,23 @@ static struct byte_range_lock *brl_get_locks_internal(TALLOC_CTX *mem_ctx,
 	key.dptr = (uint8 *)&fsp->file_id;
 	key.dsize = sizeof(struct file_id);
 
-	if (!fsp->lockdb_clean) {
-		/* We must be read/write to clean
-		   the dead entries. */
-		do_read_only = false;
-	}
-
-	if (do_read_only) {
-		NTSTATUS status;
-		status = dbwrap_fetch(brlock_db, br_lck, key, &data);
-		if (!NT_STATUS_IS_OK(status)) {
-			DEBUG(3, ("Could not fetch byte range lock record\n"));
-			TALLOC_FREE(br_lck);
-			return NULL;
-		}
-		br_lck->record = NULL;
-	} else {
-		br_lck->record = dbwrap_fetch_locked(brlock_db, br_lck, key);
-
-		if (br_lck->record == NULL) {
-			DEBUG(3, ("Could not lock byte range lock entry\n"));
-			TALLOC_FREE(br_lck);
-			return NULL;
-		}
+	br_lck->record = dbwrap_fetch_locked(brlock_db, br_lck, key);
 
-		data = dbwrap_record_get_value(br_lck->record);
+	if (br_lck->record == NULL) {
+		DEBUG(3, ("Could not lock byte range lock entry\n"));
+		TALLOC_FREE(br_lck);
+		return NULL;
 	}
 
+	data = dbwrap_record_get_value(br_lck->record);
+
 	if ((data.dsize % sizeof(struct lock_struct)) != 0) {
 		DEBUG(3, ("Got invalid brlock data\n"));
 		TALLOC_FREE(br_lck);
 		return NULL;
 	}
 
-	br_lck->read_only = do_read_only;
+	br_lck->read_only = false;
 	br_lck->lock_data = NULL;
 
 	talloc_set_destructor(br_lck, byte_range_lock_destructor);
@@ -2041,21 +2023,13 @@ static struct byte_range_lock *brl_get_locks_internal(TALLOC_CTX *mem_ctx,
 		}
 	}
 
-	if (do_read_only != read_only) {
-		/*
-		 * this stores the record and gets rid of
-		 * the write lock that is needed for a cleanup
-		 */
-		byte_range_lock_flush(br_lck);
-	}
-
 	return br_lck;
 }
 
 struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx,
 					files_struct *fsp)
 {
-	return brl_get_locks_internal(mem_ctx, fsp, False);
+	return brl_get_locks_internal(mem_ctx, fsp);
 }
 
 struct brl_get_locks_readonly_state {
@@ -2102,7 +2076,7 @@ struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
 		 * Fetch the record in R/W mode to give validate_lock_entries
 		 * a chance to kick in once.
 		 */
-		rw = brl_get_locks_internal(talloc_tos(), fsp, false);
+		rw = brl_get_locks_internal(talloc_tos(), fsp);
 		if (rw == NULL) {
 			return NULL;
 		}
-- 
1.7.9.5


From 098ae2f09419c19ffcaadca6924296fbacda53ff Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 11 Sep 2013 11:53:26 +0000
Subject: [PATCH 4/5] smbd: Remove the brl_get_locks wrapper

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/locking/brlock.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index f57c7e5..78315bc 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -1929,8 +1929,7 @@ static int byte_range_lock_destructor(struct byte_range_lock *br_lck)
  TALLOC_FREE(brl) will release the lock in the destructor.
 ********************************************************************/
 
-static struct byte_range_lock *brl_get_locks_internal(TALLOC_CTX *mem_ctx,
-						      files_struct *fsp)
+struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx, files_struct *fsp)
 {
 	TDB_DATA key, data;
 	struct byte_range_lock *br_lck = talloc(mem_ctx, struct byte_range_lock);
@@ -2026,12 +2025,6 @@ static struct byte_range_lock *brl_get_locks_internal(TALLOC_CTX *mem_ctx,
 	return br_lck;
 }
 
-struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx,
-					files_struct *fsp)
-{
-	return brl_get_locks_internal(mem_ctx, fsp);
-}
-
 struct brl_get_locks_readonly_state {
 	TALLOC_CTX *mem_ctx;
 	struct byte_range_lock **br_lock;
@@ -2076,7 +2069,7 @@ struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
 		 * Fetch the record in R/W mode to give validate_lock_entries
 		 * a chance to kick in once.
 		 */
-		rw = brl_get_locks_internal(talloc_tos(), fsp);
+		rw = brl_get_locks(talloc_tos(), fsp);
 		if (rw == NULL) {
 			return NULL;
 		}
-- 
1.7.9.5


From fb7657869782e751d85a7b713e7a373b28a1683c Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 11 Sep 2013 11:54:37 +0000
Subject: [PATCH 5/5] smbd: Remove byte_range_lock->read_only

With the rewritten brl_get_lock_readonly we only set the destructor for
r/w lock records anyway.

Signed-off-by: Volker Lendecke <vl at samba.org>
---
 source3/locking/brlock.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/source3/locking/brlock.c b/source3/locking/brlock.c
index 78315bc..0d45501 100644
--- a/source3/locking/brlock.c
+++ b/source3/locking/brlock.c
@@ -47,7 +47,6 @@ struct byte_range_lock {
 	struct files_struct *fsp;
 	unsigned int num_locks;
 	bool modified;
-	bool read_only;
 	struct lock_struct *lock_data;
 	struct db_record *record;
 };
@@ -1879,10 +1878,6 @@ int brl_forall(void (*fn)(struct file_id id, struct server_id pid,
 
 static void byte_range_lock_flush(struct byte_range_lock *br_lck)
 {
-	if (br_lck->read_only) {
-		SMB_ASSERT(!br_lck->modified);
-	}
-
 	if (!br_lck->modified) {
 		goto done;
 	}
@@ -1910,10 +1905,7 @@ static void byte_range_lock_flush(struct byte_range_lock *br_lck)
 	}
 
  done:
-
-	br_lck->read_only = true;
 	br_lck->modified = false;
-
 	TALLOC_FREE(br_lck->record);
 }
 
@@ -1961,7 +1953,6 @@ struct byte_range_lock *brl_get_locks(TALLOC_CTX *mem_ctx, files_struct *fsp)
 		return NULL;
 	}
 
-	br_lck->read_only = false;
 	br_lck->lock_data = NULL;
 
 	talloc_set_destructor(br_lck, byte_range_lock_destructor);
@@ -2119,7 +2110,6 @@ struct byte_range_lock *brl_get_locks_readonly(files_struct *fsp)
 
 	br_lock->fsp = fsp;
 	br_lock->modified = false;
-	br_lock->read_only = true;
 	br_lock->record = NULL;
 
 	if (lp_clustering()) {
-- 
1.7.9.5



More information about the samba-technical mailing list