[PATCH] smbd: Use mutex instead of fcntl lock for echohandler coordination
Christof Schmitt
cs at samba.org
Tue Jul 8 11:58:24 MDT 2014
On Tue, Jul 08, 2014 at 12:57:05AM +0200, Stefan (metze) Metzmacher wrote:
> Hi Christof,
>
> > +static bool smbd_echo_active(struct smbd_server_connection *sconn)
> > +{
> > + return (sconn->smb1.echo_handler.socket_lock_fd != -1
> > +#ifdef HAVE_ROBUST_MUTEXES
> > + || sconn->smb1.echo_handler.socket_mutex != NULL
> > +#endif
> > + );
> > +}
>
> Can you please use if statements like this:
>
>
> if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
> return true;
> }
>
> #ifdef HAVE_ROBUST_MUTEXES
> if (sconn->smb1.echo_handler.socket_mutex != NULL) {
> return true;
> }
> #endif
>
> return false;
>
> The rest look good thanks!
Updated in the attached patch.
Christof
-------------- next part --------------
>From 8ad47de77b069a2a10c1fd086f7200d24b7999ac Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 27 Jun 2014 13:05:59 -0700
Subject: [PATCH] smbd: Use mutex instead of fcntl lock for echohandler coordination
This is based on a patch from Volker. When the system supports roboust
mutexes, they will be used for the coordiations between worker and
echohandler process. This avoids another aspect of the fcntl scalibility
issue when handling many client connections. When mutexes are not
available, the code falls back to the fcntl lock.
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/smbd/globals.h | 12 +++-
source3/smbd/msdfs.c | 3 +-
source3/smbd/process.c | 192 ++++++++++++++++++++++++++++++++++++++++--------
3 files changed, 173 insertions(+), 34 deletions(-)
diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
index 81e22fb..70148fb 100644
--- a/source3/smbd/globals.h
+++ b/source3/smbd/globals.h
@@ -115,6 +115,7 @@ DATA_BLOB negprot_spnego(TALLOC_CTX *ctx, struct smbd_server_connection *sconn);
void smbd_lock_socket(struct smbd_server_connection *sconn);
void smbd_unlock_socket(struct smbd_server_connection *sconn);
+void smbd_echo_init(struct smbd_server_connection *sconn);
NTSTATUS smbd_do_locking(struct smb_request *req,
files_struct *fsp,
@@ -696,10 +697,19 @@ struct smbd_server_connection {
struct tevent_fd *fde;
struct {
+
/*
- * fd for the fcntl lock mutexing access to our sock
+ * fd for the fcntl lock and process shared
+ * robust mutex to coordinate access to the
+ * client socket. When the system supports
+ * process shared robust mutexes, those are
+ * used. If not, then the fcntl lock will be
+ * used.
*/
int socket_lock_fd;
+#ifdef HAVE_ROBUST_MUTEXES
+ pthread_mutex_t *socket_mutex;
+#endif
/*
* fd for the trusted pipe from
diff --git a/source3/smbd/msdfs.c b/source3/smbd/msdfs.c
index 97849d2..1239ade 100644
--- a/source3/smbd/msdfs.c
+++ b/source3/smbd/msdfs.c
@@ -247,8 +247,7 @@ static NTSTATUS create_conn_struct_as_root(TALLOC_CTX *ctx,
sconn->ev_ctx = ev;
sconn->msg_ctx = msg;
sconn->sock = -1;
- sconn->smb1.echo_handler.trusted_fd = -1;
- sconn->smb1.echo_handler.socket_lock_fd = -1;
+ smbd_echo_init(sconn);
conn = conn_new(sconn);
if (conn == NULL) {
diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index b1d522d..dd931e0 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -40,6 +40,7 @@
#include "../libcli/security/security_token.h"
#include "lib/id_cache.h"
#include "serverid.h"
+#include "system/threads.h"
/* Internal message queue for deferred opens. */
struct pending_message_list {
@@ -61,11 +62,33 @@ static struct pending_message_list *get_deferred_open_message_smb(
struct smbd_server_connection *sconn, uint64_t mid);
static bool smb_splice_chain(uint8_t **poutbuf, const uint8_t *andx_buf);
-static bool smbd_lock_socket_internal(struct smbd_server_connection *sconn)
+void smbd_echo_init(struct smbd_server_connection *sconn)
{
- bool ok;
+ sconn->smb1.echo_handler.trusted_fd = -1;
+ sconn->smb1.echo_handler.socket_lock_fd = -1;
+#ifdef HAVE_ROBUST_MUTEXES
+ sconn->smb1.echo_handler.socket_mutex = NULL;
+#endif
+}
- if (sconn->smb1.echo_handler.socket_lock_fd == -1) {
+static bool smbd_echo_active(struct smbd_server_connection *sconn)
+{
+ if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
+ return true;
+ }
+
+#ifdef HAVE_ROBUST_MUTEXES
+ if (sconn->smb1.echo_handler.socket_mutex != NULL) {
+ return true;
+ }
+#endif
+
+ return false;
+}
+
+static bool smbd_lock_socket_internal(struct smbd_server_connection *sconn)
+{
+ if (!smbd_echo_active(sconn)) {
return true;
}
@@ -77,15 +100,38 @@ static bool smbd_lock_socket_internal(struct smbd_server_connection *sconn)
DEBUG(10,("pid[%d] wait for socket lock\n", (int)getpid()));
- do {
- ok = fcntl_lock(
- sconn->smb1.echo_handler.socket_lock_fd,
- F_SETLKW, 0, 0, F_WRLCK);
- } while (!ok && (errno == EINTR));
+#ifdef HAVE_ROBUST_MUTEXES
+ if (sconn->smb1.echo_handler.socket_mutex != NULL) {
+ int ret = EINTR;
- if (!ok) {
- DEBUG(1, ("fcntl_lock failed: %s\n", strerror(errno)));
- return false;
+ while (ret == EINTR) {
+ ret = pthread_mutex_lock(
+ sconn->smb1.echo_handler.socket_mutex);
+ if (ret == 0) {
+ break;
+ }
+ }
+ if (ret != 0) {
+ DEBUG(1, ("pthread_mutex_lock failed: %s\n",
+ strerror(ret)));
+ return false;
+ }
+ }
+#endif
+
+ if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
+ bool ok;
+
+ do {
+ ok = fcntl_lock(
+ sconn->smb1.echo_handler.socket_lock_fd,
+ F_SETLKW, 0, 0, F_WRLCK);
+ } while (!ok && (errno == EINTR));
+
+ if (!ok) {
+ DEBUG(1, ("fcntl_lock failed: %s\n", strerror(errno)));
+ return false;
+ }
}
DEBUG(10,("pid[%d] got socket lock\n", (int)getpid()));
@@ -102,9 +148,7 @@ void smbd_lock_socket(struct smbd_server_connection *sconn)
static bool smbd_unlock_socket_internal(struct smbd_server_connection *sconn)
{
- bool ok;
-
- if (sconn->smb1.echo_handler.socket_lock_fd == -1) {
+ if (!smbd_echo_active(sconn)) {
return true;
}
@@ -114,15 +158,38 @@ static bool smbd_unlock_socket_internal(struct smbd_server_connection *sconn)
return true;
}
- do {
- ok = fcntl_lock(
- sconn->smb1.echo_handler.socket_lock_fd,
- F_SETLKW, 0, 0, F_UNLCK);
- } while (!ok && (errno == EINTR));
+#ifdef HAVE_ROBUST_MUTEXES
+ if (sconn->smb1.echo_handler.socket_mutex != NULL) {
+ int ret = EINTR;
- if (!ok) {
- DEBUG(1, ("fcntl_lock failed: %s\n", strerror(errno)));
- return false;
+ while (ret == EINTR) {
+ ret = pthread_mutex_unlock(
+ sconn->smb1.echo_handler.socket_mutex);
+ if (ret == 0) {
+ break;
+ }
+ }
+ if (ret != 0) {
+ DEBUG(1, ("pthread_mutex_unlock failed: %s\n",
+ strerror(ret)));
+ return false;
+ }
+ }
+#endif
+
+ if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
+ bool ok;
+
+ do {
+ ok = fcntl_lock(
+ sconn->smb1.echo_handler.socket_lock_fd,
+ F_SETLKW, 0, 0, F_UNLCK);
+ } while (!ok && (errno == EINTR));
+
+ if (!ok) {
+ DEBUG(1, ("fcntl_lock failed: %s\n", strerror(errno)));
+ return false;
+ }
}
DEBUG(10,("pid[%d] unlocked socket\n", (int)getpid()));
@@ -3135,16 +3202,74 @@ bool fork_echo_handler(struct smbd_server_connection *sconn)
int listener_pipe[2];
int res;
pid_t child;
+ bool use_mutex = false;
res = pipe(listener_pipe);
if (res == -1) {
DEBUG(1, ("pipe() failed: %s\n", strerror(errno)));
return false;
}
- sconn->smb1.echo_handler.socket_lock_fd = create_unlink_tmp(lp_lock_directory());
- if (sconn->smb1.echo_handler.socket_lock_fd == -1) {
- DEBUG(1, ("Could not create lock fd: %s\n", strerror(errno)));
- goto fail;
+
+#ifdef HAVE_ROBUST_MUTEXES
+ use_mutex = tdb_runtime_check_for_robust_mutexes();
+
+ if (use_mutex) {
+ pthread_mutexattr_t a;
+
+ sconn->smb1.echo_handler.socket_mutex =
+ anonymous_shared_allocate(sizeof(pthread_mutex_t));
+ if (sconn->smb1.echo_handler.socket_mutex == NULL) {
+ DEBUG(1, ("Could not create mutex shared memory: %s\n",
+ strerror(errno)));
+ goto fail;
+ }
+
+ res = pthread_mutexattr_init(&a);
+ if (res != 0) {
+ DEBUG(1, ("pthread_mutexattr_init failed: %s\n",
+ strerror(res)));
+ goto fail;
+ }
+ res = pthread_mutexattr_settype(&a, PTHREAD_MUTEX_ERRORCHECK);
+ if (res != 0) {
+ DEBUG(1, ("pthread_mutexattr_settype failed: %s\n",
+ strerror(res)));
+ pthread_mutexattr_destroy(&a);
+ goto fail;
+ }
+ res = pthread_mutexattr_setpshared(&a, PTHREAD_PROCESS_SHARED);
+ if (res != 0) {
+ DEBUG(1, ("pthread_mutexattr_setpshared failed: %s\n",
+ strerror(res)));
+ pthread_mutexattr_destroy(&a);
+ goto fail;
+ }
+ res = pthread_mutexattr_setrobust(&a, PTHREAD_MUTEX_ROBUST);
+ if (res != 0) {
+ DEBUG(1, ("pthread_mutexattr_setrobust failed: "
+ "%s\n", strerror(res)));
+ pthread_mutexattr_destroy(&a);
+ goto fail;
+ }
+ res = pthread_mutex_init(sconn->smb1.echo_handler.socket_mutex,
+ &a);
+ pthread_mutexattr_destroy(&a);
+ if (res != 0) {
+ DEBUG(1, ("pthread_mutex_init failed: %s\n",
+ strerror(res)));
+ goto fail;
+ }
+ }
+#endif
+
+ if (!use_mutex) {
+ sconn->smb1.echo_handler.socket_lock_fd =
+ create_unlink_tmp(lp_lock_directory());
+ if (sconn->smb1.echo_handler.socket_lock_fd == -1) {
+ DEBUG(1, ("Could not create lock fd: %s\n",
+ strerror(errno)));
+ goto fail;
+ }
}
child = fork();
@@ -3195,12 +3320,18 @@ fail:
if (listener_pipe[1] != -1) {
close(listener_pipe[1]);
}
- sconn->smb1.echo_handler.trusted_fd = -1;
if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
close(sconn->smb1.echo_handler.socket_lock_fd);
}
- sconn->smb1.echo_handler.trusted_fd = -1;
- sconn->smb1.echo_handler.socket_lock_fd = -1;
+
+#ifdef HAVE_ROBUST_MUTEXES
+ if (sconn->smb1.echo_handler.socket_mutex != NULL) {
+ pthread_mutex_destroy(sconn->smb1.echo_handler.socket_mutex);
+ anonymous_shared_free(sconn->smb1.echo_handler.socket_mutex);
+ }
+#endif
+ smbd_echo_init(sconn);
+
return false;
}
@@ -3417,8 +3548,7 @@ void smbd_process(struct tevent_context *ev_ctx,
sconn->ev_ctx = ev_ctx;
sconn->msg_ctx = msg_ctx;
sconn->sock = sock_fd;
- sconn->smb1.echo_handler.trusted_fd = -1;
- sconn->smb1.echo_handler.socket_lock_fd = -1;
+ smbd_echo_init(sconn);
if (!interactive) {
smbd_setup_sig_term_handler(sconn);
--
1.7.1
More information about the samba-technical
mailing list