[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