[PATCHES] Issue fsync for SMB2 FLUSH asynchronously
Christof Schmitt
cs at samba.org
Thu Nov 12 21:41:36 UTC 2015
On Thu, Nov 12, 2015 at 11:56:47AM -0800, Jeremy Allison wrote:
> On Wed, Nov 11, 2015 at 06:05:57PM -0800, Jeremy Allison wrote:
> > > TEST SMB2-BASIC FAILED!
> > >
> > > Let me look at this some more..
> >
> > Found the bug. It's my fault of old..
> >
> > There is a *HIDEOUS* global variable that counts
> > the number of outstanding aio calls:
> >
> > extern int outstanding_aio_calls;
> >
> > and when source3/modules/vfs_default.c
> > wants to read the results of an async
> > call in vfswrap_asys_finished() it
> > uses a variable length array defined
> > as:
> >
> > struct asys_result results[outstanding_aio_calls];
> >
> > ret = asys_results(asys_ctx, results, outstanding_aio_calls);
> >
> > to work out how many to read. The vfswrap_fsync_send()
> > doesn't increment outstanding_aio_calls so when
> > the fflush finishes it calls asys_results(asys_ctx, results, 0);
> > which doesn't work too well :-).
> >
> > I need to ensure outstanding_aio_calls is kept up
> > to date everywhere.
> >
> > Going to clock off now - I'll post a fixed
> > patch (that passes make test) tomorrow.
> >
> > Sorry for the problem !
>
> OK, here is the revised and working patch.
>
> I removed the global access to the problematic
> variables, created wrapper functions for them, and
> ensure that the number of outstanding
> aio calls is incremented and decremented
> correctly on the async fsync calls.
>
> Passes local make test ! Please review and
> push if happy.
Looks good, pushed to autobuild.
Thanks for the patches, it would have taken me some time to figure out
the details.
Christof
>
> Cheers,
>
> Jeremy.
> From 40471bfcff873baff22b12980699b18129d457f7 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 12 Nov 2015 09:20:05 -0800
> Subject: [PATCH 1/4] s3: smbd: Remove aio_pending_size from globals.
>
> Access via functions only.
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/modules/vfs_aio_fork.c | 2 +-
> source3/modules/vfs_aio_linux.c | 8 ++++----
> source3/modules/vfs_aio_pthread.c | 4 ++--
> source3/modules/vfs_default.c | 2 +-
> source3/smbd/aio.c | 24 ++++++++++++++++++++----
> source3/smbd/globals.c | 1 -
> source3/smbd/globals.h | 1 -
> source3/smbd/proto.h | 2 ++
> 8 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/source3/modules/vfs_aio_fork.c b/source3/modules/vfs_aio_fork.c
> index 7fca3d1..25a72c6 100644
> --- a/source3/modules/vfs_aio_fork.c
> +++ b/source3/modules/vfs_aio_fork.c
> @@ -909,7 +909,7 @@ static int aio_fork_connect(vfs_handle_struct *handle, const char *service,
> * Essentially we want this to be unlimited unless smb.conf
> * says different.
> *********************************************************************/
> - aio_pending_size = 100;
> + set_aio_pending_size(100);
> return 0;
> }
>
> diff --git a/source3/modules/vfs_aio_linux.c b/source3/modules/vfs_aio_linux.c
> index 74ebb3c..599272e 100644
> --- a/source3/modules/vfs_aio_linux.c
> +++ b/source3/modules/vfs_aio_linux.c
> @@ -113,12 +113,12 @@ static bool init_aio_linux(struct vfs_handle_struct *handle)
> goto fail;
> }
>
> - if (io_queue_init(aio_pending_size, &io_ctx)) {
> + if (io_queue_init(get_aio_pending_size(), &io_ctx)) {
> goto fail;
> }
>
> DEBUG(10,("init_aio_linux: initialized with up to %d events\n",
> - aio_pending_size));
> + get_aio_pending_size()));
>
> return true;
>
> @@ -333,8 +333,8 @@ static int aio_linux_connect(vfs_handle_struct *handle, const char *service,
> * Essentially we want this to be unlimited unless smb.conf
> * says different.
> *********************************************************************/
> - aio_pending_size = lp_parm_int(
> - SNUM(handle->conn), "aio_linux", "aio num events", 128);
> + set_aio_pending_size(lp_parm_int(
> + SNUM(handle->conn), "aio_linux", "aio num events", 128));
> return SMB_VFS_NEXT_CONNECT(handle, service, user);
> }
>
> diff --git a/source3/modules/vfs_aio_pthread.c b/source3/modules/vfs_aio_pthread.c
> index 059d745..72c812f 100644
> --- a/source3/modules/vfs_aio_pthread.c
> +++ b/source3/modules/vfs_aio_pthread.c
> @@ -51,7 +51,7 @@ static bool init_aio_threadpool(struct tevent_context *ev_ctx,
> return true;
> }
>
> - ret = pthreadpool_init(aio_pending_size, pp_pool);
> + ret = pthreadpool_init(get_aio_pending_size(), pp_pool);
> if (ret) {
> errno = ret;
> return false;
> @@ -69,7 +69,7 @@ static bool init_aio_threadpool(struct tevent_context *ev_ctx,
> }
>
> DEBUG(10,("init_aio_threadpool: initialized with up to %d threads\n",
> - aio_pending_size));
> + get_aio_pending_size()));
>
> return true;
> }
> diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
> index bbe8cca..5d0966e 100644
> --- a/source3/modules/vfs_default.c
> +++ b/source3/modules/vfs_default.c
> @@ -716,7 +716,7 @@ static bool vfswrap_init_asys_ctx(struct smbd_server_connection *conn)
> return true;
> }
>
> - ret = asys_context_init(&ctx, aio_pending_size);
> + ret = asys_context_init(&ctx, get_aio_pending_size());
> if (ret != 0) {
> DEBUG(1, ("asys_context_init failed: %s\n", strerror(ret)));
> return false;
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index 253782b..a750a1a 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -26,6 +26,22 @@
> #include "lib/tevent_wait.h"
>
> /****************************************************************************
> + Statics plus accessor functions.
> +*****************************************************************************/
> +
> +static int aio_pending_size = 100; /* tevent supports 100 signals SA_SIGINFO */
> +
> +int get_aio_pending_size(void)
> +{
> + return aio_pending_size;
> +}
> +
> +void set_aio_pending_size(int newsize)
> +{
> + aio_pending_size = newsize;
> +}
> +
> +/****************************************************************************
> The buffer we keep around whilst an aio request is in process.
> *****************************************************************************/
>
> @@ -186,7 +202,7 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= aio_pending_size) {
> + if (outstanding_aio_calls >= get_aio_pending_size()) {
> DEBUG(10,("schedule_aio_read_and_X: Already have %d aio "
> "activities outstanding.\n",
> outstanding_aio_calls ));
> @@ -452,7 +468,7 @@ NTSTATUS schedule_aio_write_and_X(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= aio_pending_size) {
> + if (outstanding_aio_calls >= get_aio_pending_size()) {
> DEBUG(3,("schedule_aio_write_and_X: Already have %d aio "
> "activities outstanding.\n",
> outstanding_aio_calls ));
> @@ -711,7 +727,7 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= aio_pending_size) {
> + if (outstanding_aio_calls >= get_aio_pending_size()) {
> DEBUG(10,("smb2: Already have %d aio "
> "activities outstanding.\n",
> outstanding_aio_calls ));
> @@ -867,7 +883,7 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= aio_pending_size) {
> + if (outstanding_aio_calls >= get_aio_pending_size()) {
> DEBUG(3,("smb2: Already have %d aio "
> "activities outstanding.\n",
> outstanding_aio_calls ));
> diff --git a/source3/smbd/globals.c b/source3/smbd/globals.c
> index a501234..f552bc8 100644
> --- a/source3/smbd/globals.c
> +++ b/source3/smbd/globals.c
> @@ -24,7 +24,6 @@
> #include "messages.h"
> #include <tdb.h>
>
> -int aio_pending_size = 100; /* tevent supports 100 signals SA_SIGINFO */
> int outstanding_aio_calls = 0;
>
> #ifdef USE_DMAPI
> diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
> index b567a58..86774cf 100644
> --- a/source3/smbd/globals.h
> +++ b/source3/smbd/globals.h
> @@ -22,7 +22,6 @@
> #include "librpc/gen_ndr/smbXsrv.h"
> #include "smbprofile.h"
>
> -extern int aio_pending_size;
> extern int outstanding_aio_calls;
>
> #ifdef USE_DMAPI
> diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
> index 2eac3ec..caeecc7 100644
> --- a/source3/smbd/proto.h
> +++ b/source3/smbd/proto.h
> @@ -66,6 +66,8 @@ void srv_set_signing(struct smbXsrv_connection *conn,
>
> /* The following definitions come from smbd/aio.c */
>
> +int get_aio_pending_size(void);
> +void set_aio_pending_size(int newsize);
> struct aio_extra;
> bool aio_write_through_requested(struct aio_extra *aio_ex);
> NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
> From 84bdfa6f54fdf7b11aa84758158accc312ef88ba Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Thu, 12 Nov 2015 09:25:41 -0800
> Subject: [PATCH 2/4] s3: smbd: Remove outstanding_aio_calls from globals.
>
> Access via functions only.
>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/modules/vfs_default.c | 4 ++--
> source3/smbd/aio.c | 41 +++++++++++++++++++++++++++++------------
> source3/smbd/globals.c | 2 --
> source3/smbd/globals.h | 2 --
> source3/smbd/proto.h | 3 +++
> 5 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
> index 5d0966e..f3ebb89 100644
> --- a/source3/modules/vfs_default.c
> +++ b/source3/modules/vfs_default.c
> @@ -866,14 +866,14 @@ static void vfswrap_asys_finished(struct tevent_context *ev,
> uint16_t flags, void *p)
> {
> struct asys_context *asys_ctx = (struct asys_context *)p;
> - struct asys_result results[outstanding_aio_calls];
> + struct asys_result results[get_outstanding_aio_calls()];
> int i, ret;
>
> if ((flags & TEVENT_FD_READ) == 0) {
> return;
> }
>
> - ret = asys_results(asys_ctx, results, outstanding_aio_calls);
> + ret = asys_results(asys_ctx, results, get_outstanding_aio_calls());
> if (ret < 0) {
> DEBUG(1, ("asys_results returned %s\n", strerror(-ret)));
> return;
> diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
> index a750a1a..1216a0d 100644
> --- a/source3/smbd/aio.c
> +++ b/source3/smbd/aio.c
> @@ -30,6 +30,7 @@
> *****************************************************************************/
>
> static int aio_pending_size = 100; /* tevent supports 100 signals SA_SIGINFO */
> +static int outstanding_aio_calls;
>
> int get_aio_pending_size(void)
> {
> @@ -41,6 +42,21 @@ void set_aio_pending_size(int newsize)
> aio_pending_size = newsize;
> }
>
> +int get_outstanding_aio_calls(void)
> +{
> + return outstanding_aio_calls;
> +}
> +
> +void increment_outstanding_aio_calls(void)
> +{
> + outstanding_aio_calls++;
> +}
> +
> +void decrement_outstanding_aio_calls(void)
> +{
> + outstanding_aio_calls--;
> +}
> +
> /****************************************************************************
> The buffer we keep around whilst an aio request is in process.
> *****************************************************************************/
> @@ -66,7 +82,7 @@ bool aio_write_through_requested(struct aio_extra *aio_ex)
>
> static int aio_extra_destructor(struct aio_extra *aio_ex)
> {
> - outstanding_aio_calls--;
> + decrement_outstanding_aio_calls();
> return 0;
> }
>
> @@ -98,7 +114,7 @@ static struct aio_extra *create_aio_extra(TALLOC_CTX *mem_ctx,
> }
> talloc_set_destructor(aio_ex, aio_extra_destructor);
> aio_ex->fsp = fsp;
> - outstanding_aio_calls++;
> + increment_outstanding_aio_calls();
> return aio_ex;
> }
>
> @@ -202,10 +218,10 @@ NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= get_aio_pending_size()) {
> + if (get_outstanding_aio_calls() >= get_aio_pending_size()) {
> DEBUG(10,("schedule_aio_read_and_X: Already have %d aio "
> "activities outstanding.\n",
> - outstanding_aio_calls ));
> + get_outstanding_aio_calls() ));
> return NT_STATUS_RETRY;
> }
>
> @@ -468,10 +484,10 @@ NTSTATUS schedule_aio_write_and_X(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= get_aio_pending_size()) {
> + if (get_outstanding_aio_calls() >= get_aio_pending_size()) {
> DEBUG(3,("schedule_aio_write_and_X: Already have %d aio "
> "activities outstanding.\n",
> - outstanding_aio_calls ));
> + get_outstanding_aio_calls() ));
> DEBUG(10,("schedule_aio_write_and_X: failed to schedule "
> "aio_write for file %s, offset %.0f, len = %u "
> "(mid = %u)\n",
> @@ -554,7 +570,8 @@ NTSTATUS schedule_aio_write_and_X(connection_struct *conn,
> "%s, offset %.0f, len = %u (mid = %u) "
> "outstanding_aio_calls = %d\n",
> fsp_str_dbg(fsp), (double)startpos, (unsigned int)numtowrite,
> - (unsigned int)aio_ex->smbreq->mid, outstanding_aio_calls ));
> + (unsigned int)aio_ex->smbreq->mid,
> + get_outstanding_aio_calls() ));
>
> return NT_STATUS_OK;
> }
> @@ -727,10 +744,10 @@ NTSTATUS schedule_smb2_aio_read(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= get_aio_pending_size()) {
> + if (get_outstanding_aio_calls() >= get_aio_pending_size()) {
> DEBUG(10,("smb2: Already have %d aio "
> "activities outstanding.\n",
> - outstanding_aio_calls ));
> + get_outstanding_aio_calls() ));
> return NT_STATUS_RETRY;
> }
>
> @@ -883,10 +900,10 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
> return NT_STATUS_RETRY;
> }
>
> - if (outstanding_aio_calls >= get_aio_pending_size()) {
> + if (get_outstanding_aio_calls() >= get_aio_pending_size()) {
> DEBUG(3,("smb2: Already have %d aio "
> "activities outstanding.\n",
> - outstanding_aio_calls ));
> + get_outstanding_aio_calls() ));
> return NT_STATUS_RETRY;
> }
>
> @@ -955,7 +972,7 @@ NTSTATUS schedule_aio_smb2_write(connection_struct *conn,
> (double)in_offset,
> (unsigned int)in_data.length,
> (unsigned int)aio_ex->smbreq->mid,
> - outstanding_aio_calls ));
> + get_outstanding_aio_calls() ));
>
> return NT_STATUS_OK;
> }
> diff --git a/source3/smbd/globals.c b/source3/smbd/globals.c
> index f552bc8..70805a1 100644
> --- a/source3/smbd/globals.c
> +++ b/source3/smbd/globals.c
> @@ -24,8 +24,6 @@
> #include "messages.h"
> #include <tdb.h>
>
> -int outstanding_aio_calls = 0;
> -
> #ifdef USE_DMAPI
> struct smbd_dmapi_context *dmapi_ctx = NULL;
> #endif
> diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h
> index 86774cf..0422cbe 100644
> --- a/source3/smbd/globals.h
> +++ b/source3/smbd/globals.h
> @@ -22,8 +22,6 @@
> #include "librpc/gen_ndr/smbXsrv.h"
> #include "smbprofile.h"
>
> -extern int outstanding_aio_calls;
> -
> #ifdef USE_DMAPI
> struct smbd_dmapi_context;
> extern struct smbd_dmapi_context *dmapi_ctx;
> diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
> index caeecc7..95414e6 100644
> --- a/source3/smbd/proto.h
> +++ b/source3/smbd/proto.h
> @@ -68,6 +68,9 @@ void srv_set_signing(struct smbXsrv_connection *conn,
>
> int get_aio_pending_size(void);
> void set_aio_pending_size(int newsize);
> +int get_outstanding_aio_calls(void);
> +void increment_outstanding_aio_calls(void);
> +void decrement_outstanding_aio_calls(void);
> struct aio_extra;
> bool aio_write_through_requested(struct aio_extra *aio_ex);
> NTSTATUS schedule_aio_read_and_X(connection_struct *conn,
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
> From 5a356b7a58a636541b98cf3e6759b0b8ffe8eb7a Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 11 Nov 2015 13:31:15 -0700
> Subject: [PATCH 3/4] smbd: Issue fsync for SMB2 FLUSH asynchronously
>
> SMB2 FLUSH mainly calls fsync and there is already code in place to
> handle fsync asynchronously, so use the asynchronous code path for SMB2
> FLUSH. This avoids a SMB2 FLUSH stalling other requests processing.
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
> source3/smbd/smb2_flush.c | 74 ++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 67 insertions(+), 7 deletions(-)
>
> diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c
> index 04a8710..d26707a 100644
> --- a/source3/smbd/smb2_flush.c
> +++ b/source3/smbd/smb2_flush.c
> @@ -110,15 +110,18 @@ struct smbd_smb2_flush_state {
> struct smbd_smb2_request *smb2req;
> };
>
> +static void smbd_smb2_flush_done(struct tevent_req *subreq);
> +
> static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx,
> struct tevent_context *ev,
> struct smbd_smb2_request *smb2req,
> struct files_struct *fsp)
> {
> struct tevent_req *req;
> + struct tevent_req *subreq;
> struct smbd_smb2_flush_state *state;
> - NTSTATUS status;
> struct smb_request *smbreq;
> + int ret;
>
> req = tevent_req_create(mem_ctx, &state,
> struct smbd_smb2_flush_state);
> @@ -145,16 +148,73 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx,
> return tevent_req_post(req, ev);
> }
>
> - status = sync_file(smbreq->conn, fsp, true);
> - if (!NT_STATUS_IS_OK(status)) {
> - DEBUG(5,("smbd_smb2_flush: sync_file for %s returned %s\n",
> - fsp_str_dbg(fsp), nt_errstr(status)));
> - tevent_req_nterror(req, status);
> + if (fsp->fh->fd == -1) {
> + tevent_req_nterror(req, NT_STATUS_INVALID_HANDLE);
> + return tevent_req_post(req, ev);
> + }
> +
> + if (!lp_strict_sync(SNUM(smbreq->conn))) {
> + /*
> + * No strict sync. Don't really do
> + * anything here.
> + */
> + tevent_req_done(req);
> + return tevent_req_post(req, ev);
> + }
> +
> + if (get_outstanding_aio_calls() >= get_aio_pending_size()) {
> + /* No more allowed aio. Synchronous flush. */
> + NTSTATUS status = sync_file(smbreq->conn, fsp, true);
> + if (!NT_STATUS_IS_OK(status)) {
> + DEBUG(5,("sync_file for %s returned %s\n",
> + fsp_str_dbg(fsp),
> + nt_errstr(status)));
> + tevent_req_nterror(req, status);
> + return tevent_req_post(req, ev);
> + }
> + tevent_req_done(req);
> + return tevent_req_post(req, ev);
> + }
> +
> + ret = flush_write_cache(fsp, SAMBA_SYNC_FLUSH);
> + if (ret == -1) {
> + tevent_req_nterror(req, map_nt_error_from_unix(errno));
> + return tevent_req_post(req, ev);
> + }
> +
> + subreq = SMB_VFS_FSYNC_SEND(state, ev, fsp);
> + if (tevent_req_nomem(subreq, req)) {
> + return tevent_req_post(req, ev);
> + }
> +
> + tevent_req_set_callback(subreq, smbd_smb2_flush_done, req);
> +
> + /* Ensure any close request knows about this outstanding IO. */
> + if (!aio_add_req_to_fsp(fsp, req)) {
> + tevent_req_nterror(req, NT_STATUS_NO_MEMORY);
> return tevent_req_post(req, ev);
> }
>
> + increment_outstanding_aio_calls();
> + return req;
> +
> +}
> +
> +static void smbd_smb2_flush_done(struct tevent_req *subreq)
> +{
> + struct tevent_req *req = tevent_req_callback_data(
> + subreq, struct tevent_req);
> + int ret, err;
> +
> + decrement_outstanding_aio_calls();
> +
> + ret = SMB_VFS_FSYNC_RECV(subreq, &err);
> + TALLOC_FREE(subreq);
> + if (ret == -1) {
> + tevent_req_error(req, err);
> + return;
> + }
> tevent_req_done(req);
> - return tevent_req_post(req, ev);
> }
>
> static NTSTATUS smbd_smb2_flush_recv(struct tevent_req *req)
> --
> 2.6.0.rc2.230.g3dd15c0
>
>
> From 374f2ca5fe619e8b3d4a9872d2ab0f49715380c2 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Wed, 11 Nov 2015 13:28:09 -0700
> Subject: [PATCH 4/4] selftest: Use strict sync = yes
>
> This enables the codepaths calling fsync for FLUSH requests during
> selftest.
>
> Signed-off-by: Christof Schmitt <cs at samba.org>
> Reviewed-by: Jeremy Allison <jra at samba.org>
> ---
> selftest/target/Samba3.pm | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 4b7882b..1c54dae 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -1331,6 +1331,7 @@ sub provision($$$$$$$$)
> create mask = 755
> dos filemode = yes
> strict rename = yes
> + strict sync = yes
> vfs objects = acl_xattr fake_acls xattr_tdb streams_depot
>
> printing = vlp
> --
> 2.6.0.rc2.230.g3dd15c0
>
More information about the samba-technical
mailing list