[PATCH] smbd: Use mutex instead of fcntl lock for echohandler coordination
Stefan (metze) Metzmacher
metze at samba.org
Thu Jul 3 00:51:25 MDT 2014
Hi Christof,
Am 02.07.2014 22:49, schrieb Christof Schmitt:
>>From be394a99160cc9921731e50dba9c0e8af96796a4 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 | 9 +++-
> source3/smbd/msdfs.c | 1 +
> source3/smbd/process.c | 141 +++++++++++++++++++++++++++++++++++++++---------
> 3 files changed, 124 insertions(+), 27 deletions(-)
>
> diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
> index d9ca5e3..b7f4ae9 100644
> --- a/source3/smbd/globals.h
> +++ b/source3/smbd/globals.h
> @@ -694,10 +694,17 @@ 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;
> + pthread_mutex_t *socket_mutex;
We should use
#ifdef PTHREAD_MUTEX_ROBUST
pthread_mutex_t *socket_mutex;
#else
const void *socket_mutex;
#endif
> /*
> * fd for the trusted pipe from
> diff --git a/source3/smbd/msdfs.c b/source3/smbd/msdfs.c
> index 97849d2..2569941 100644
> --- a/source3/smbd/msdfs.c
> +++ b/source3/smbd/msdfs.c
> @@ -249,6 +249,7 @@ static NTSTATUS create_conn_struct_as_root(TALLOC_CTX *ctx,
> sconn->sock = -1;
> sconn->smb1.echo_handler.trusted_fd = -1;
> sconn->smb1.echo_handler.socket_lock_fd = -1;
> + sconn->smb1.echo_handler.socket_mutex = NULL;
>
> conn = conn_new(sconn);
> if (conn == NULL) {
> diff --git a/source3/smbd/process.c b/source3/smbd/process.c
> index b1d522d..35d1885 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 "pthread.h"
Please use "system/threads.h"
> /* Internal message queue for deferred opens. */
> struct pending_message_list {
> @@ -63,9 +64,8 @@ static bool smb_splice_chain(uint8_t **poutbuf, const uint8_t *andx_buf);
>
> static bool smbd_lock_socket_internal(struct smbd_server_connection *sconn)
> {
> - bool ok;
> -
> - if (sconn->smb1.echo_handler.socket_lock_fd == -1) {
> + if (sconn->smb1.echo_handler.socket_lock_fd == -1 &&
> + sconn->smb1.echo_handler.socket_mutex == NULL) {
> return true;
> }
>
> @@ -77,15 +77,34 @@ 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));
> + if (sconn->smb1.echo_handler.socket_mutex != NULL) {
> + int ret = EINTR;
We also need some #ifdef PTHREAD_MUTEX_ROBUST here...
> - 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;
> + }
> + } else {
> + 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 +121,8 @@ 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 (sconn->smb1.echo_handler.socket_lock_fd == -1 &&
> + sconn->smb1.echo_handler.socket_mutex == NULL) {
> return true;
> }
>
> @@ -114,15 +132,34 @@ 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));
> + if (sconn->smb1.echo_handler.socket_mutex != NULL) {
> + int ret = EINTR;
And here #ifdef PTHREAD_MUTEX_ROBUST
> - 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;
> + }
> + } else {
> + 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()));
> @@ -3141,10 +3178,56 @@ bool fork_echo_handler(struct smbd_server_connection *sconn)
> 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;
> +
> + if (tdb_runtime_check_for_robust_mutexes()) {
> + pthread_mutexattr_t a;
This is not enough, we need #ifdef PTHREAD_MUTEX_ROBUST before
tdb_runtime_check_for_robust_mutexes().
> + sconn->smb1.echo_handler.socket_mutex =
> + (pthread_mutex_t *)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;
> + }
In tdb we also have
ret = pthread_mutexattr_settype(&ma, PTHREAD_MUTEX_ERRORCHECK);
Should we also use it here?
> + 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_np(
> + &a, PTHREAD_MUTEX_ROBUST_NP);
> + if (res != 0) {
> + DEBUG(1, ("pthread_mutexattr_setrobust_np failed: "
> + "%s\n", strerror(res)));
You need to remove the _np/_NP here to make it portable,
system/thread.h will take care of older systems.
> + 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;
> + }
> + } else {
> + 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();
> @@ -3199,8 +3282,13 @@ fail:
> if (sconn->smb1.echo_handler.socket_lock_fd != -1) {
> close(sconn->smb1.echo_handler.socket_lock_fd);
> }
> + if (sconn->smb1.echo_handler.socket_mutex != NULL) {
Do we need some additional cleanup here?
Maybe some non linux systems need pthread_mutex_destroy()?
> + anonymous_shared_free(sconn->smb1.echo_handler.socket_mutex);
> + }
> sconn->smb1.echo_handler.trusted_fd = -1;
> sconn->smb1.echo_handler.socket_lock_fd = -1;
> + sconn->smb1.echo_handler.socket_mutex = NULL;
> +
> return false;
> }
>
> @@ -3419,6 +3507,7 @@ void smbd_process(struct tevent_context *ev_ctx,
> sconn->sock = sock_fd;
> sconn->smb1.echo_handler.trusted_fd = -1;
> sconn->smb1.echo_handler.socket_lock_fd = -1;
> + sconn->smb1.echo_handler.socket_mutex = NULL;
>
> if (!interactive) {
> smbd_setup_sig_term_handler(sconn);
> -- 1.7.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 246 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20140703/05dfea69/attachment.pgp>
More information about the samba-technical
mailing list