[PATCH] caching optimization in share mode locks.
Jeremy Allison
jra at samba.org
Fri Apr 3 13:35:50 MDT 2015
History: An OEM came to me with the (attached
as the .png file) profile from cachegrind on
Samba 4.1.x.
It shows the ndr decoding adding a significant
penalty to the code of fetching share mode data
structures.
After some discussion with Volker, he came
up with the idea of adding a 64-bit sequence
number to the share mode entry, which is
incremented whenever the share mode changes.
This allows an smbd to cache a the share
mode entry in the memcache, and re-use
it directly if the sequence number hasn't
changed - avoiding the ndr decoding step
by flipping the talloc ownership of the
entry to and from the memcache.
Patches implementing this attached (and
they're pretty short). It passes make
tests and all local testing for me.
Please review + comment or push :-).
Cheers,
Jeremy.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smb-4.x.get-share-mode-lock.png
Type: image/png
Size: 231868 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150403/8bd48c06/attachment-0001.png>
-------------- next part --------------
From f789caaf359a3921edff4f2f81867eac8719ccbd Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 Apr 2015 11:19:22 -0700
Subject: [PATCH 1/2] lib: memcache. Add a new talloc cache type -
SHARE_MODE_LOCK_CACHE.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
lib/util/memcache.c | 1 +
lib/util/memcache.h | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/util/memcache.c b/lib/util/memcache.c
index 2f2e77c..9e9a208 100644
--- a/lib/util/memcache.c
+++ b/lib/util/memcache.c
@@ -53,6 +53,7 @@ static bool memcache_is_talloc(enum memcache_number n)
case GETPWNAM_CACHE:
case PDB_GETPWSID_CACHE:
case SINGLETON_CACHE_TALLOC:
+ case SHARE_MODE_LOCK_CACHE:
result = true;
break;
default:
diff --git a/lib/util/memcache.h b/lib/util/memcache.h
index 97490b9..2602fb7 100644
--- a/lib/util/memcache.h
+++ b/lib/util/memcache.h
@@ -40,7 +40,8 @@ enum memcache_number {
PDB_GETPWSID_CACHE, /* talloc */
SINGLETON_CACHE_TALLOC, /* talloc */
SINGLETON_CACHE,
- SMB1_SEARCH_OFFSET_MAP
+ SMB1_SEARCH_OFFSET_MAP,
+ SHARE_MODE_LOCK_CACHE /* talloc */
};
/*
--
2.2.0.rc0.207.ga3a616c
From aeeeb1fc5b09595cb87e2413b4a8ec397236aad8 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 2 Apr 2015 11:19:49 -0700
Subject: [PATCH 2/2] s3: locking: Add a memcache based lock cache.
Based on an idea by Volker to optimize cpu usage when
parsing struct share_mode_data entries.
Add a 64-bit sequence number to the share mode entry,
and after the entry is stored back in the db, cache
the in-memory version using talloc reparenting into the
memcache.
On read, check if the db versions sequence number
matches the in-memory cache, and if so reparent the
memcache version back onto the required memory context.
Saves all the ndr decoding when multiple
accesses to the same lock entry happen in succession.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/librpc/idl/open_files.idl | 1 +
source3/locking/share_mode_lock.c | 137 ++++++++++++++++++++++++++++++++++++--
2 files changed, 132 insertions(+), 6 deletions(-)
diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl
index 0a9d5fd..4be55e3 100644
--- a/source3/librpc/idl/open_files.idl
+++ b/source3/librpc/idl/open_files.idl
@@ -72,6 +72,7 @@ interface open_files
} delete_token;
typedef [public] struct {
+ uint8 sequence_number[8];
[string,charset(UTF8)] char *servicepath;
[string,charset(UTF8)] char *base_name;
[string,charset(UTF8)] char *stream_name;
diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
index 138950f..9774434 100644
--- a/source3/locking/share_mode_lock.c
+++ b/source3/locking/share_mode_lock.c
@@ -48,6 +48,7 @@
#include "../librpc/gen_ndr/ndr_open_files.h"
#include "source3/lib/dbwrap/dbwrap_watch.h"
#include "locking/leases_db.h"
+#include "../lib/util/memcache.h"
#undef DBGC_CLASS
#define DBGC_CLASS DBGC_LOCKING
@@ -121,6 +122,99 @@ static TDB_DATA locking_key(const struct file_id *id)
}
/*******************************************************************
+ Share mode cache utility functions that store/delete/retrieve
+ entries from memcache.
+
+ For now share the statcache (global cache) memory space. If
+ a lock record gets orphaned due to the sequence number being
+ updated by another smbd, it will eventually fall out of the
+ cache via the normal LRU trim mechanism. If necessary we can
+ always make this a separate (smaller) cache.
+******************************************************************/
+
+static void share_mode_memcache_delete(struct share_mode_data *d)
+{
+ DEBUG(10,("deleted entry for file %s seq ",
+ d->base_name));
+ dump_data(10, d->sequence_number, 8);
+
+ memcache_delete(NULL,
+ SHARE_MODE_LOCK_CACHE,
+ data_blob_const(d->sequence_number, 8));
+}
+
+static void share_mode_memcache_store(struct share_mode_data *d)
+{
+ DEBUG(10,("stored entry for file %s seq ",
+ d->base_name));
+ dump_data(10, d->sequence_number, 8);
+
+ /* Ensure everything stored in the cache is pristine. */
+ d->modified = false;
+ d->fresh = false;
+
+ /*
+ * Ensure the memory going into the cache
+ * doesn't have a destructor so it can be
+ * cleanly freed by share_mode_memcache_delete().
+ */
+ talloc_set_destructor(d, NULL);
+
+ /* Cache will own d after this call. */
+ memcache_add_talloc(NULL,
+ SHARE_MODE_LOCK_CACHE,
+ data_blob_const(d->sequence_number, 8),
+ &d);
+}
+
+static int share_mode_data_nofree_destructor(struct share_mode_data *d)
+{
+ return -1;
+}
+
+static struct share_mode_data *share_mode_memcache_fetch(TALLOC_CTX *mem_ctx,
+ DATA_BLOB *blob)
+{
+ struct share_mode_data *d;
+ void *ptr;
+
+ if (blob->data == NULL || blob->length < 8) {
+ return NULL;
+ }
+ /* sequence number key is at start of blob. */
+ ptr = memcache_lookup_talloc(NULL,
+ SHARE_MODE_LOCK_CACHE,
+ data_blob_const(blob->data, 8));
+ if (ptr == NULL) {
+ DEBUG(10,("failed to find entry for seq "));
+ dump_data(10, blob->data, 8);
+ return NULL;
+ }
+ /* Move onto mem_ctx. */
+ d = talloc_move(mem_ctx, &ptr);
+
+ /*
+ * Now we own d, prevent the cache from freeing it
+ * when we delete the entry.
+ */
+ talloc_set_destructor(d, share_mode_data_nofree_destructor);
+
+ /* Remove from the cache. We own it now. */
+ memcache_delete(NULL,
+ SHARE_MODE_LOCK_CACHE,
+ data_blob_const(d->sequence_number, 8));
+
+ /* And reset the destructor to none. */
+ talloc_set_destructor(d, NULL);
+
+ DEBUG(10,("fetched entry for file %s seq ",
+ d->base_name));
+ dump_data(10, d->sequence_number, 8);
+
+ return d;
+}
+
+/*******************************************************************
Get all share mode entries for a dev/inode pair.
********************************************************************/
@@ -132,15 +226,21 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
uint32_t i;
DATA_BLOB blob;
+ blob.data = dbuf.dptr;
+ blob.length = dbuf.dsize;
+
+ /* See if we already have a cached copy of this sequence number. */
+ d = share_mode_memcache_fetch(mem_ctx, &blob);
+ if (d != NULL) {
+ return d;
+ }
+
d = talloc(mem_ctx, struct share_mode_data);
if (d == NULL) {
DEBUG(0, ("talloc failed\n"));
goto fail;
}
- blob.data = dbuf.dptr;
- blob.length = dbuf.dsize;
-
ndr_err = ndr_pull_struct_blob_all(
&blob, d, d, (ndr_pull_flags_fn_t)ndr_pull_share_mode_data);
if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) {
@@ -189,12 +289,20 @@ static TDB_DATA unparse_share_modes(struct share_mode_data *d)
{
DATA_BLOB blob;
enum ndr_err_code ndr_err;
+ uint64_t seq;
if (DEBUGLEVEL >= 10) {
DEBUG(10, ("unparse_share_modes:\n"));
NDR_PRINT_DEBUG(share_mode_data, d);
}
+ share_mode_memcache_delete(d);
+
+ /* Update the sequence number. */
+ memcpy(&seq, d->sequence_number, 8);
+ seq += 1;
+ memcpy(d->sequence_number, &seq, 8);
+
remove_stale_share_mode_entries(d);
if (d->num_share_modes == 0) {
@@ -246,7 +354,11 @@ static int share_mode_data_destructor(struct share_mode_data *d)
smb_panic(errmsg);
}
}
- goto done;
+ /*
+ * Nothing to store in cache - allow the normal
+ * release of record lock and memory free.
+ */
+ return 0;
}
status = dbwrap_record_store(d->record, data, TDB_REPLACE);
@@ -262,9 +374,19 @@ static int share_mode_data_destructor(struct share_mode_data *d)
smb_panic(errmsg);
}
- done:
+ /*
+ * Release the record lock before putting in the cache.
+ */
+ TALLOC_FREE(d->record);
- return 0;
+ /*
+ * Reparent d into the in-memory cache so it can be reused if the
+ * sequence number matches. See parse_share_modes()
+ * for details.
+ */
+
+ share_mode_memcache_store(d);
+ return -1;
}
/*******************************************************************
@@ -288,6 +410,9 @@ static struct share_mode_data *fresh_share_mode_lock(
if (d == NULL) {
goto fail;
}
+ /* New record - new sequence number. */
+ generate_random_buffer(d->sequence_number, 8);
+
d->base_name = talloc_strdup(d, smb_fname->base_name);
if (d->base_name == NULL) {
goto fail;
--
2.2.0.rc0.207.ga3a616c
More information about the samba-technical
mailing list