[SCM] Samba Shared Repository - branch master updated
Jeremy Allison
jra at samba.org
Thu Apr 12 18:24:03 MDT 2012
The branch, master has been updated
via b8dea7e Wrong assertion/comparison: Compare value not pointer
via da786cd We never cancel SMB1 aio, only SMB2 aio - and in this case we always return a value.
via d399af3 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.
via fd38486 Fix return_fn when aio was cancelled. We need to return -1, errno = ECANCELED.
via 9583910 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.
via 80a4e38 Fix the same bug reported by Kirill Malkin <kirill.malkin at starboardstorage.com> and fixed by Volker for vfs_aio_fork as ref 0aacdbfada46329e0ad9dacfa90041a1c7dbf3e8.
from f6328b1 s3: Fix a typo
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit b8dea7e82d0acb9e55e8bfe9a089c250d7380102
Author: Olaf Flebbe <o.flebbe at science-computing.de>
Date: Thu Apr 12 11:29:41 2012 +0200
Wrong assertion/comparison: Compare value not pointer
Signed-off-by: Jeremy Allison <jra at samba.org>
Autobuild-User: Jeremy Allison <jra at samba.org>
Autobuild-Date: Fri Apr 13 02:23:36 CEST 2012 on sn-devel-104
commit da786cddd68d8777cad5e3d6c6fe5a3beac2a1a7
Author: Jeremy Allison <jra at samba.org>
Date: Thu Apr 12 15:11:22 2012 -0700
We never cancel SMB1 aio, only SMB2 aio - and in this case we always return a value.
So pass_cancel is no longer needed.
commit d399af30c183663f487bf4d086ec4377f84725b0
Author: Jeremy Allison <jra at samba.org>
Date: Thu Apr 12 15:04:42 2012 -0700
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.
commit fd3848636498fa357ff0085a126e374a3e91d14b
Author: Jeremy Allison <jra at samba.org>
Date: Thu Apr 12 15:04:08 2012 -0700
Fix return_fn when aio was cancelled. We need to return -1, errno = ECANCELED.
commit 95839102ad9c1b052924a99ee938991a305d1add
Author: Jeremy Allison <jra at samba.org>
Date: Thu Apr 12 13:48:29 2012 -0700
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.
commit 80a4e38d6465d8ae47ca113027421af852d34316
Author: Jeremy Allison <jra at samba.org>
Date: Thu Apr 12 13:15:23 2012 -0700
Fix the same bug reported by Kirill Malkin <kirill.malkin at starboardstorage.com> and
fixed by Volker for vfs_aio_fork as ref 0aacdbfada46329e0ad9dacfa90041a1c7dbf3e8.
From that change:
aio_suspend does not signal the main process with a signal, it just waits. The
aio_fork module does not use the signal at all, it directly calls back into the
main smbd by calling smbd_aio_complete_aio_ex. This is an abstraction
violation, but the alternative would have been to use signals where they are
not needed. However, in wait_for_aio_completion this bites us: With aio_fork we
call handle_aio_completed twice on the same aio_ex struct: Once from the call
to handle_aio_completion within the aio_fork module and once from the code in
wait_for_aio_completion.
Fix this differently here by not calling directly back into smbd,
but using a new function aio_linux_setup_returns() to setup the
return values that wait_for_aio_completion() in the main smbd
will pick up by calling handle_aio_completd().
-----------------------------------------------------------------------
Summary of changes:
source3/modules/vfs_aio_fork.c | 5 +++
source3/modules/vfs_aio_linux.c | 33 ++++++++++++++++--
source3/modules/vfs_aio_pthread.c | 5 +++
source3/registry/reg_parse.c | 4 +-
source3/smbd/aio.c | 66 ++++++++++---------------------------
source3/smbd/close.c | 21 +++++-------
source3/smbd/proto.h | 1 -
7 files changed, 68 insertions(+), 67 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
index d10cc9f..f3e8f7f 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_linux.c b/source3/modules/vfs_aio_linux.c
index d49dc49..d152f35 100644
--- a/source3/modules/vfs_aio_linux.c
+++ b/source3/modules/vfs_aio_linux.c
@@ -280,12 +280,12 @@ static int aio_linux_write(struct vfs_handle_struct *handle,
}
/************************************************************************
- Handle a single finished io.
+ Save off the error / success conditions from the io_event.
+ Is idempotent (can be called multiple times given the same ioev).
***********************************************************************/
-static void aio_linux_handle_io_finished(struct io_event *ioev)
+static void aio_linux_setup_returns(struct io_event *ioev)
{
- struct aio_extra *aio_ex = NULL;
struct aio_private_data *pd = (struct aio_private_data *)ioev->data;
/* ioev->res2 contains the -errno if error. */
@@ -297,6 +297,18 @@ static void aio_linux_handle_io_finished(struct io_event *ioev)
pd->ret_size = ioev->res;
pd->ret_errno = 0;
}
+}
+
+/************************************************************************
+ Handle a single finished io.
+***********************************************************************/
+
+static void aio_linux_handle_io_finished(struct io_event *ioev)
+{
+ struct aio_extra *aio_ex = NULL;
+ struct aio_private_data *pd = (struct aio_private_data *)ioev->data;
+
+ aio_linux_setup_returns(ioev);
aio_ex = (struct aio_extra *)pd->aiocb->aio_sigevent.sigev_value.sival_ptr;
smbd_aio_complete_aio_ex(aio_ex);
@@ -406,6 +418,11 @@ static ssize_t aio_linux_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;
}
@@ -512,7 +529,15 @@ static void aio_linux_handle_suspend_io_finished(struct suspend_private *sp,
for (i = 0; i < sp->num_entries; i++) {
if (sp->aiocb_array[i] == pd->aiocb) {
sp->num_finished++;
- aio_linux_handle_io_finished(ioev);
+ /*
+ * We don't call aio_linux_handle_io_finished()
+ * here, but only the function that sets up the
+ * return values. This allows
+ * aio_linux_handle_io_finished() to be successfully
+ * called from smbd/aio.c:wait_for_aio_completion()
+ * once we return from here with all io's done.
+ */
+ aio_linux_setup_returns(ioev);
return;
}
}
diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c
index cb441bf..36ce9ab 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;
}
diff --git a/source3/registry/reg_parse.c b/source3/registry/reg_parse.c
index a112572..5a22fd7 100644
--- a/source3/registry/reg_parse.c
+++ b/source3/registry/reg_parse.c
@@ -791,12 +791,12 @@ handle_iconv_errno(int err, const char* obuf, size_t linenum,
DEBUG(0, ("Illegal multibyte sequence at line %lu: %s",
(long unsigned)(linenum+1), pos));
- assert(ilen > 0);
+ assert((*ilen) > 0);
do {
size_t il = 1;
DEBUGADD(0, ("<%02x>", (unsigned char)**iptr));
- if (olen > 0) {
+ if ((*olen) > 0) {
*(*optr)++ = '\?';
(*iptr)++;
/* Todo: parametrize, e.g. skip: *optr++ = *iptr++; */
diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index e5347a4..aa8aee0 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -49,7 +49,6 @@ struct aio_extra {
DATA_BLOB outbuf;
struct lock_struct lock;
bool write_through;
- bool pass_cancel;
int (*handle_completion)(struct aio_extra *ex, int errcode);
};
@@ -106,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;
}
@@ -138,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;
}
@@ -231,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, "
@@ -337,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. */
@@ -454,7 +453,6 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
return NT_STATUS_NO_MEMORY;
}
aio_ex->handle_completion = handle_aio_smb2_read_complete;
- aio_ex->pass_cancel = true;
init_strict_lock_struct(fsp, (uint64_t)smbreq->smbpid,
(uint64_t)startpos, (uint64_t)smb_maxcnt, READ_LOCK,
@@ -487,7 +485,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;
@@ -551,7 +548,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
aio_ex->handle_completion = handle_aio_smb2_write_complete;
aio_ex->write_through = write_through;
- aio_ex->pass_cancel = true;
init_strict_lock_struct(fsp, (uint64_t)smbreq->smbpid,
in_offset, (uint64_t)in_data.length, WRITE_LOCK,
@@ -584,7 +580,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;
@@ -865,18 +860,16 @@ static bool handle_aio_completed(struct aio_extra *aio_ex, int *perr)
return False;
}
+ if (err == ECANCELED) {
+ DEBUG(10,( "handle_aio_completed: operation mid %llu 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);
- if (!aio_ex->pass_cancel && 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;
- }
-
err = aio_ex->handle_completion(aio_ex, err);
if (err) {
*perr = err; /* Only save non-zero errors. */
@@ -894,8 +887,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));
@@ -915,12 +906,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)
{
@@ -984,8 +975,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;
}
@@ -1014,29 +1011,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)
@@ -1087,10 +1061,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 dc6df47..c87b1a0 100644
--- a/source3/smbd/close.c
+++ b/source3/smbd/close.c
@@ -667,19 +667,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 f2040cd..a770c3a 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -89,7 +89,6 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
bool write_through);
bool cancel_smb2_aio(struct smb_request *smbreq);
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 */
--
Samba Shared Repository
More information about the samba-cvs
mailing list