[PATCHES] Issue fsync for SMB2 FLUSH asynchronously

Jeremy Allison jra at samba.org
Thu Nov 12 19:56:47 UTC 2015


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.

Cheers,

	Jeremy.
-------------- next part --------------
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