From ee03210c5da8d9f39761dde4639e3d87209bbea9 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Sat, 31 Mar 2012 10:37:15 +0200 Subject: [PATCH 1/8] s3-aio-fork: Fix an alignment warning on OS/X (cherry picked from commit aef86982b845072d8624294f5c557eb315740467) --- source3/modules/vfs_aio_fork.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c index 4891cd2..187166c 100644 --- a/source3/modules/vfs_aio_fork.c +++ b/source3/modules/vfs_aio_fork.c @@ -167,7 +167,7 @@ static ssize_t read_fd(int fd, void *ptr, size_t nbytes, int *recvfd) errno = EINVAL; return -1; } - *recvfd = *((int *) CMSG_DATA(cmptr)); + memcpy(recvfd, CMSG_DATA(cmptr), sizeof(*recvfd)); } else { *recvfd = -1; /* descriptor was not passed */ } @@ -205,7 +205,7 @@ static ssize_t write_fd(int fd, void *ptr, size_t nbytes, int sendfd) cmptr->cmsg_len = CMSG_LEN(sizeof(int)); cmptr->cmsg_level = SOL_SOCKET; cmptr->cmsg_type = SCM_RIGHTS; - *((int *) CMSG_DATA(cmptr)) = sendfd; + memcpy(CMSG_DATA(cmptr), &sendfd, sizeof(sendfd)); #else ZERO_STRUCT(msg); msg.msg_accrights = (caddr_t) &sendfd; -- 1.7.7.3 From 39591687c2d62ab6304b9e3f7b946cbd2da3141f Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Sun, 8 Apr 2012 20:11:53 +0200 Subject: [PATCH 2/8] s3: Move the aio signal init to the vfs module On platforms that don't have an RT signal space, signal initialization fails. aio_fork and aio_pthread don't need the signal, so this would block them from running as well. (cherry picked from commit eff36099c12e936a880c9f2e102b9cf8a7166d40) --- source3/modules/vfs_default.c | 8 ++++++++ source3/smbd/aio.c | 28 +++++++--------------------- source3/smbd/proto.h | 1 + 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 27e9b9b..ebbc569 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1553,6 +1553,10 @@ static int vfswrap_fsetxattr(struct vfs_handle_struct *handle, struct files_stru static int vfswrap_aio_read(struct vfs_handle_struct *handle, struct files_struct *fsp, SMB_STRUCT_AIOCB *aiocb) { int ret; + if (!initialize_async_io_handler()) { + errno = ENOSYS; + return -1; + } /* * aio_read must be done as root, because in the glibc aio * implementation the helper thread needs to be able to send a signal @@ -1568,6 +1572,10 @@ static int vfswrap_aio_read(struct vfs_handle_struct *handle, struct files_struc static int vfswrap_aio_write(struct vfs_handle_struct *handle, struct files_struct *fsp, SMB_STRUCT_AIOCB *aiocb) { int ret; + if (!initialize_async_io_handler()) { + errno = ENOSYS; + return -1; + } /* * aio_write must be done as root, because in the glibc aio * implementation the helper thread needs to be able to send a signal diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index b0b90c0..3ae7adc 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -70,7 +70,7 @@ static void smbd_aio_signal_handler(struct tevent_context *ev_ctx, } -static bool initialize_async_io_handler(void) +bool initialize_async_io_handler(void) { static bool tried_signal_setup = false; @@ -155,11 +155,6 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn, size_t min_aio_read_size = lp_aio_read_size(SNUM(conn)); int ret; - /* Ensure aio is initialized. */ - if (!initialize_async_io_handler()) { - return NT_STATUS_RETRY; - } - if (fsp->base_fsp != NULL) { /* No AIO on streams yet */ DEBUG(10, ("AIO on streams not yet supported\n")); @@ -262,11 +257,6 @@ NTSTATUS schedule_aio_write_and_X(connection_struct *conn, size_t min_aio_write_size = lp_aio_write_size(SNUM(conn)); int ret; - /* Ensure aio is initialized. */ - if (!initialize_async_io_handler()) { - return NT_STATUS_RETRY; - } - if (fsp->base_fsp != NULL) { /* No AIO on streams yet */ DEBUG(10, ("AIO on streams not yet supported\n")); @@ -398,11 +388,6 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn, size_t min_aio_read_size = lp_aio_read_size(SNUM(conn)); int ret; - /* Ensure aio is initialized. */ - if (!initialize_async_io_handler()) { - return NT_STATUS_RETRY; - } - if (fsp->base_fsp != NULL) { /* No AIO on streams yet */ DEBUG(10, ("AIO on streams not yet supported\n")); @@ -502,11 +487,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn, size_t min_aio_write_size = lp_aio_write_size(SNUM(conn)); int ret; - /* Ensure aio is initialized. */ - if (!initialize_async_io_handler()) { - return NT_STATUS_RETRY; - } - if (fsp->base_fsp != NULL) { /* No AIO on streams yet */ DEBUG(10, ("AIO on streams not yet supported\n")); @@ -1021,6 +1001,12 @@ void cancel_aio_by_fsp(files_struct *fsp) } #else + +bool initialize_async_io_handler(void) +{ + return false; +} + NTSTATUS schedule_aio_read_and_X(connection_struct *conn, struct smb_request *smbreq, files_struct *fsp, SMB_OFF_T startpos, diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 0d31a1c..4b25513 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -64,6 +64,7 @@ void srv_set_signing(struct smbd_server_connection *conn, /* The following definitions come from smbd/aio.c */ +bool initialize_async_io_handler(void); NTSTATUS schedule_aio_read_and_X(connection_struct *conn, struct smb_request *req, files_struct *fsp, SMB_OFF_T startpos, -- 1.7.7.3 From bc69c4220a3c7a083568b447f719a2f5af679b0c Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Fri, 13 Apr 2012 14:52:14 -0700 Subject: [PATCH 3/8] s3: Initialize aio_pending_size from aio_pthread --- source3/modules/vfs_aio_pthread.c | 41 +++++++++++++++++++----------------- 1 files changed, 22 insertions(+), 19 deletions(-) diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index ceef822..07e9caa 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -25,6 +25,7 @@ #include "system/filesys.h" #include "system/shmem.h" #include "smbd/smbd.h" +#include "smbd/globals.h" #include "lib/pthreadpool/pthreadpool.h" struct aio_extra; @@ -49,21 +50,6 @@ static void aio_pthread_handle_completion(struct event_context *event_ctx, uint16 flags, void *p); -/************************************************************************ - How many threads to initialize ? - 100 per process seems insane as a default until you realize that - (a) Threads terminate after 1 second when idle. - (b) Throttling is done in SMB2 via the crediting algorithm. - (c) SMB1 clients are limited to max_mux (50) outstanding requests and - Windows clients don't use this anyway. - Essentially we want this to be unlimited unless smb.conf says different. -***********************************************************************/ - -static int aio_get_num_threads(struct vfs_handle_struct *handle) -{ - return lp_parm_int(SNUM(handle->conn), - "aio_pthread", "aio num threads", 100); -} /************************************************************************ Ensure thread pool is initialized. @@ -73,14 +59,12 @@ static bool init_aio_threadpool(struct vfs_handle_struct *handle) { struct fd_event *sock_event = NULL; int ret = 0; - int num_threads; if (pool) { return true; } - num_threads = aio_get_num_threads(handle); - ret = pthreadpool_init(num_threads, &pool); + ret = pthreadpool_init(aio_pending_size, &pool); if (ret) { errno = ret; return false; @@ -98,7 +82,7 @@ static bool init_aio_threadpool(struct vfs_handle_struct *handle) } DEBUG(10,("init_aio_threadpool: initialized with up to %d threads\n", - num_threads)); + aio_pending_size)); return true; } @@ -608,7 +592,26 @@ static int aio_pthread_suspend(struct vfs_handle_struct *handle, return ret; } +static int aio_pthread_connect(vfs_handle_struct *handle, const char *service, + const char *user) +{ + /********************************************************************* + * How many threads to initialize ? + * 100 per process seems insane as a default until you realize that + * (a) Threads terminate after 1 second when idle. + * (b) Throttling is done in SMB2 via the crediting algorithm. + * (c) SMB1 clients are limited to max_mux (50) outstanding + * requests and Windows clients don't use this anyway. + * Essentially we want this to be unlimited unless smb.conf + * says different. + *********************************************************************/ + aio_pending_size = lp_parm_int( + SNUM(handle->conn), "aio_pthread", "aio num threads", 100); + return SMB_VFS_NEXT_CONNECT(handle, service, user); +} + static struct vfs_fn_pointers vfs_aio_pthread_fns = { + .connect_fn = aio_pthread_connect, .aio_read = aio_pthread_read, .aio_write = aio_pthread_write, .aio_return_fn = aio_pthread_return_fn, -- 1.7.7.3 From 0999359d917276d947fc92c01ea603663533ef5c Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Fri, 13 Apr 2012 14:52:58 -0700 Subject: [PATCH 4/8] s3: Initialize aio_pending_size from aio_pthread --- source3/modules/vfs_aio_fork.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c index 187166c..8479ed0 100644 --- a/source3/modules/vfs_aio_fork.c +++ b/source3/modules/vfs_aio_fork.c @@ -23,6 +23,7 @@ #include "system/filesys.h" #include "system/shmem.h" #include "smbd/smbd.h" +#include "smbd/globals.h" #ifndef MAP_FILE #define MAP_FILE 0 @@ -879,7 +880,25 @@ static int aio_fork_suspend(struct vfs_handle_struct *handle, return ret; } +static int aio_fork_connect(vfs_handle_struct *handle, const char *service, + const char *user) +{ + /********************************************************************* + * How many threads to initialize ? + * 100 per process seems insane as a default until you realize that + * (a) Threads terminate after 1 second when idle. + * (b) Throttling is done in SMB2 via the crediting algorithm. + * (c) SMB1 clients are limited to max_mux (50) outstanding + * requests and Windows clients don't use this anyway. + * Essentially we want this to be unlimited unless smb.conf + * says different. + *********************************************************************/ + aio_pending_size = 100; + return SMB_VFS_NEXT_CONNECT(handle, service, user); +} + static struct vfs_fn_pointers vfs_aio_fork_fns = { + .connect_fn = aio_fork_connect, .aio_read = aio_fork_read, .aio_write = aio_fork_write, .aio_return_fn = aio_fork_return_fn, -- 1.7.7.3 From df52d9c806919fb44b78af124062302ed3f0acfa Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Apr 2012 13:48:29 -0700 Subject: [PATCH 5/8] Move the counting of outstanding_aio_calls into the lifecycle of the aio_extra struct. This way we can't end up with a mismatch between outstanding events and the counter. We may still have problems with canceling and not correctly freeing the aio struct, but at least the counter won't get out of sync anymore. (cherry picked from commit 95839102ad9c1b052924a99ee938991a305d1add) --- source3/smbd/aio.c | 8 ++------ 1 files changed, 2 insertions(+), 6 deletions(-) diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index 3ae7adc..430fbf2 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -105,6 +105,7 @@ static int handle_aio_smb2_write_complete(struct aio_extra *aio_ex, int errcode) static int aio_extra_destructor(struct aio_extra *aio_ex) { DLIST_REMOVE(aio_list_head, aio_ex); + outstanding_aio_calls--; return 0; } @@ -137,6 +138,7 @@ static struct aio_extra *create_aio_extra(TALLOC_CTX *mem_ctx, DLIST_ADD(aio_list_head, aio_ex); talloc_set_destructor(aio_ex, aio_extra_destructor); aio_ex->fsp = fsp; + outstanding_aio_calls++; return aio_ex; } @@ -230,7 +232,6 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn, return NT_STATUS_RETRY; } - outstanding_aio_calls++; aio_ex->smbreq = talloc_move(aio_ex, &smbreq); DEBUG(10,("schedule_aio_read_and_X: scheduled aio_read for file %s, " @@ -336,7 +337,6 @@ NTSTATUS schedule_aio_write_and_X(connection_struct *conn, return NT_STATUS_RETRY; } - outstanding_aio_calls++; aio_ex->smbreq = talloc_move(aio_ex, &smbreq); /* This should actually be improved to span the write. */ @@ -458,7 +458,6 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn, return NT_STATUS_RETRY; } - outstanding_aio_calls++; /* We don't need talloc_move here as both aio_ex and * smbreq are children of smbreq->smb2req. */ aio_ex->smbreq = smbreq; @@ -553,7 +552,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn, return NT_STATUS_RETRY; } - outstanding_aio_calls++; /* We don't need talloc_move here as both aio_ex and * smbreq are children of smbreq->smb2req. */ aio_ex->smbreq = smbreq; @@ -857,8 +855,6 @@ void smbd_aio_complete_aio_ex(struct aio_extra *aio_ex) files_struct *fsp = NULL; int ret = 0; - outstanding_aio_calls--; - DEBUG(10,("smbd_aio_complete_mid: mid[%llu]\n", (unsigned long long)aio_ex->smbreq->mid)); -- 1.7.7.3 From 8a360b184e6472e8c54ffab667b98aed7a7545d1 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 13 Apr 2012 14:56:08 -0700 Subject: [PATCH 6/8] Fix return_fn when aio was cancelled. We need to return -1, errno = ECANCELED. --- source3/modules/vfs_aio_fork.c | 5 +++++ source3/modules/vfs_aio_pthread.c | 5 +++++ 2 files changed, 10 insertions(+), 0 deletions(-) diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c index 8479ed0..14f1e01 100644 --- a/source3/modules/vfs_aio_fork.c +++ b/source3/modules/vfs_aio_fork.c @@ -710,6 +710,11 @@ static ssize_t aio_fork_return_fn(struct vfs_handle_struct *handle, child->aiocb = NULL; + if (child->cancelled) { + errno = ECANCELED; + return -1; + } + if (child->retval.size == -1) { errno = child->retval.ret_errno; } diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c index 07e9caa..8344c60 100644 --- a/source3/modules/vfs_aio_pthread.c +++ b/source3/modules/vfs_aio_pthread.c @@ -333,6 +333,11 @@ static ssize_t aio_pthread_return_fn(struct vfs_handle_struct *handle, pd->aiocb = NULL; + if (pd->cancelled) { + errno = ECANCELED; + return -1; + } + if (pd->ret_size == -1) { errno = pd->ret_errno; } -- 1.7.7.3 From a883cfa921c575a3fc8dd126a41fe9b8a080a228 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 12 Apr 2012 15:04:42 -0700 Subject: [PATCH 7/8] Remove cancel_aio_by_fsp(). It can never work and could lead to memory corruption as outstanding IO's complete. Also we never have any aio's on a call to close_normal_file() with close_type ERROR_CLOSE. (cherry picked from commit d399af30c183663f487bf4d086ec4377f84725b0) --- source3/smbd/aio.c | 39 +++++++++------------------------------ source3/smbd/close.c | 21 +++++++++------------ source3/smbd/proto.h | 1 - 3 files changed, 18 insertions(+), 43 deletions(-) diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index 430fbf2..83c3a83 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -874,12 +874,12 @@ void smbd_aio_complete_aio_ex(struct aio_extra *aio_ex) } /**************************************************************************** - We're doing write behind and the client closed the file. Wait up to 30 + We're doing write behind and the client closed the file. Wait up to 45 seconds (my arbitrary choice) for the aio to complete. Return 0 if all writes completed, errno to return if not. *****************************************************************************/ -#define SMB_TIME_FOR_AIO_COMPLETE_WAIT 29 +#define SMB_TIME_FOR_AIO_COMPLETE_WAIT 45 int wait_for_aio_completion(files_struct *fsp) { @@ -943,8 +943,14 @@ int wait_for_aio_completion(files_struct *fsp) "%d seconds\n", aio_completion_count, seconds_left)); /* Timeout. */ - cancel_aio_by_fsp(fsp); SAFE_FREE(aiocb_list); + /* We're hosed here - IO may complete + and trample over memory if we free + the aio_ex struct, but if we don't + we leak IO requests. I think smb_panic() + if the right thing to do here. JRA. + */ + smb_panic("AIO suspend timed out - cannot continue."); return EIO; } @@ -973,29 +979,6 @@ int wait_for_aio_completion(files_struct *fsp) return EIO; } -/**************************************************************************** - Cancel any outstanding aio requests. The client doesn't care about the reply. -*****************************************************************************/ - -void cancel_aio_by_fsp(files_struct *fsp) -{ - struct aio_extra *aio_ex; - - for( aio_ex = aio_list_head; aio_ex; aio_ex = aio_ex->next) { - if (aio_ex->fsp == fsp) { - /* Unlock now we're done. */ - SMB_VFS_STRICT_UNLOCK(fsp->conn, fsp, &aio_ex->lock); - - /* Don't delete the aio_extra record as we may have - completed and don't yet know it. Just do the - aio_cancel call and return. */ - SMB_VFS_AIO_CANCEL(fsp, &aio_ex->acb); - aio_ex->fsp = NULL; /* fsp will be closed when we - * return. */ - } - } -} - #else bool initialize_async_io_handler(void) @@ -1041,10 +1024,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn, return NT_STATUS_RETRY; } -void cancel_aio_by_fsp(files_struct *fsp) -{ -} - int wait_for_aio_completion(files_struct *fsp) { return 0; diff --git a/source3/smbd/close.c b/source3/smbd/close.c index 36cec1a..7d0cd14 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -619,19 +619,16 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, NTSTATUS status = NT_STATUS_OK; NTSTATUS tmp; connection_struct *conn = fsp->conn; + int ret; - if (close_type == ERROR_CLOSE) { - cancel_aio_by_fsp(fsp); - } else { - /* - * If we're finishing async io on a close we can get a write - * error here, we must remember this. - */ - int ret = wait_for_aio_completion(fsp); - if (ret) { - status = ntstatus_keeperror( - status, map_nt_error_from_unix(ret)); - } + /* + * If we're finishing async io on a close we can get a write + * error here, we must remember this. + */ + ret = wait_for_aio_completion(fsp); + if (ret) { + status = ntstatus_keeperror( + status, map_nt_error_from_unix(ret)); } /* diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h index 4b25513..ddf2d18 100644 --- a/source3/smbd/proto.h +++ b/source3/smbd/proto.h @@ -88,7 +88,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn, DATA_BLOB in_data, bool write_through); int wait_for_aio_completion(files_struct *fsp); -void cancel_aio_by_fsp(files_struct *fsp); void smbd_aio_complete_aio_ex(struct aio_extra *aio_ex); /* The following definitions come from smbd/blocking.c */ -- 1.7.7.3 From d8d6f7ce7899249f1318d0fe96fcb989c096f6d3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 13 Apr 2012 14:59:40 -0700 Subject: [PATCH 8/8] We never cancel SMB1 aio, only SMB2 aio - and in this case we always return a value. --- source3/smbd/aio.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c index 83c3a83..5545064 100644 --- a/source3/smbd/aio.c +++ b/source3/smbd/aio.c @@ -826,18 +826,18 @@ static bool handle_aio_completed(struct aio_extra *aio_ex, int *perr) return False; } - /* Unlock now we're done. */ - SMB_VFS_STRICT_UNLOCK(fsp->conn, fsp, &aio_ex->lock); - if (err == ECANCELED) { /* If error is ECANCELED then don't return anything to the * client. */ DEBUG(10,( "handle_aio_completed: operation mid %llu" - " canceled\n", - (unsigned long long)aio_ex->smbreq->mid)); - return True; + " canceled for file %s\n", + (unsigned long long)aio_ex->smbreq->mid, + fsp_str_dbg(aio_ex->fsp))); } + /* Unlock now we're done. */ + SMB_VFS_STRICT_UNLOCK(fsp->conn, fsp, &aio_ex->lock); + err = aio_ex->handle_completion(aio_ex, err); if (err) { *perr = err; /* Only save non-zero errors. */ -- 1.7.7.3