Design change for oplock/open code?

Volker Lendecke Volker.Lendecke at SerNet.DE
Thu Apr 11 03:06:03 MDT 2013


Hi!

Attached find a patch that is NOT for upstream yet, but what
I would like to receive some comments for.

It changes our oplock and open retry handling logic. The
first of the patches adds a dbwrap_record_watch_send when we
defer an open. For those who haven't seen this API yet: It
calls us back whenever a record we're interested in changes.
It is right now used in the g_lock implementation for
persistent ctdb transactions as well as the smb2 session
reconnect code.

With current code, for oplock breaks we rely on the code
that handles the client oplock break to get back to us when
the oplock break is done. Likewise for the SHARING_VIOLATION
retry when a file is closed. The new code will retry the
open whenever the locking.tdb record changes, independently
from the oplock breaker or conflicting share mode holder. It
might lead to some false wakeups, because someone unrelated
modifies the record, but I right now I can't think of a
scenario where this could become a problem.

Two nice aspects: First, the patchset just survived an
autobuild. Second, it has the potential to decouple
responsibilities and remove quite some code, simplifying our
oplock implentation somewhat.

Comments?

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From a841175740364a7fdfe80ae5f766e5457368fc19 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 29 Nov 2012 16:10:22 +0100
Subject: [PATCH 1/2] s3: Use dbwrap_record_watch_send for defer_open

---
 source3/locking/share_mode_lock.c |    3 ++
 source3/smbd/open.c               |   68 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
index 4f26099..dbee60f 100644
--- a/source3/locking/share_mode_lock.c
+++ b/source3/locking/share_mode_lock.c
@@ -46,6 +46,7 @@
 #include "messages.h"
 #include "util_tdb.h"
 #include "../librpc/gen_ndr/ndr_open_files.h"
+#include "source3/lib/dbwrap/dbwrap_watch.h"
 
 #undef DBGC_CLASS
 #define DBGC_CLASS DBGC_LOCKING
@@ -76,6 +77,8 @@ static bool locking_init_internal(bool read_only)
 	if (!posix_locking_init(read_only))
 		return False;
 
+	dbwrap_watch_db(lock_db, server_messaging_context());
+
 	return True;
 }
 
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index be8d31b..6a2b474 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -33,6 +33,7 @@
 #include "auth.h"
 #include "serverid.h"
 #include "messages.h"
+#include "source3/lib/dbwrap/dbwrap_watch.h"
 
 extern const struct generic_mapping file_generic_mapping;
 
@@ -1458,6 +1459,13 @@ static bool request_timed_out(struct timeval request_time,
 	return (timeval_compare(&end_time, &now) < 0);
 }
 
+struct defer_open_state {
+	struct smbd_server_connection *sconn;
+	uint64_t mid;
+};
+
+static void defer_open_done(struct tevent_req *req);
+
 /****************************************************************************
  Handle the 1 second delay in returning a SHARING_VIOLATION error.
 ****************************************************************************/
@@ -1504,8 +1512,66 @@ static void defer_open(struct share_mode_lock *lck,
 		exit_server("push_deferred_open_message_smb failed");
 	}
 	if (lck) {
-		add_deferred_open(lck, req->mid, request_time, self, state->id);
+		struct defer_open_state *watch_state;
+		struct tevent_req *watch_req;
+		bool ret;
+
+		watch_state = talloc(req->sconn, struct defer_open_state);
+		if (watch_state == NULL) {
+			exit_server("talloc failed");
+		}
+		watch_state->sconn = req->sconn;
+		watch_state->mid = req->mid;
+
+		DEBUG(10, ("defering mid %llu\n",
+			   (unsigned long long)req->mid));
+
+		watch_req = dbwrap_record_watch_send(
+			watch_state, req->sconn->ev_ctx, lck->data->record,
+			req->sconn->msg_ctx);
+		if (watch_req == NULL) {
+			exit_server("Could not watch share mode record");
+		}
+		tevent_req_set_callback(watch_req, defer_open_done,
+					watch_state);
+
+		ret = tevent_req_set_endtime(
+			watch_req, req->sconn->ev_ctx,
+			timeval_sum(&request_time, &timeout));
+		SMB_ASSERT(ret);
+	}
+}
+
+static void defer_open_done(struct tevent_req *req)
+{
+	struct defer_open_state *state = tevent_req_callback_data(
+		req, struct defer_open_state);
+	struct db_record *rec = NULL;
+	NTSTATUS status;
+	bool ret;
+
+	status = dbwrap_record_watch_recv(req, talloc_tos(), &rec);
+	TALLOC_FREE(req);
+	if (!NT_STATUS_IS_OK(status)) {
+		DEBUG(1, ("dbwrap_record_watch_recv returned %s\n",
+			  nt_errstr(status)));
+		/*
+		 * Even if it failed, retry anyway. TODO: We need a way to
+		 * tell a re-scheduled open about that error.
+		 */
 	}
+
+	/*
+	 * TODO: We need a version of dbwrap_record_watch_recv that does not
+	 * fetch_lock the record.
+	 */
+	TALLOC_FREE(rec);
+
+	DEBUG(10, ("scheduling mid %llu\n", (unsigned long long)state->mid));
+
+	ret = schedule_deferred_open_message_smb(state->sconn, state->mid);
+	SMB_ASSERT(ret);
+	TALLOC_FREE(state);
 }
 
 
-- 
1.7.9.5


From 546c2846d4733505cd1bcd123095430a4ecbca4d Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Wed, 10 Apr 2013 16:57:32 +0200
Subject: [PATCH 2/2] Ignore OPEN_RETRY and BREAK_RESPONSE

---
 source3/smbd/oplock.c      |    8 ++++----
 source3/smbd/process.c     |   20 ++++++++++----------
 source3/smbd/smb2_create.c |   16 ++++++++--------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
index 1fc7c83..1ea8175 100644
--- a/source3/smbd/oplock.c
+++ b/source3/smbd/oplock.c
@@ -988,12 +988,12 @@ bool init_oplocks(struct smbd_server_connection *sconn)
 			   process_oplock_break_message);
 	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_ASYNC_LEVEL2_BREAK,
 			   process_oplock_async_level2_break_message);
-	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_BREAK_RESPONSE,
-			   process_oplock_break_response);
+//	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_BREAK_RESPONSE,
+//			   process_oplock_break_response);
 	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_KERNEL_BREAK,
 			   process_kernel_oplock_break);
-	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_OPEN_RETRY,
-			   process_open_retry_message);
+//	messaging_register(sconn->msg_ctx, sconn, MSG_SMB_OPEN_RETRY,
+//			   process_open_retry_message);
 
 	return true;
 }
diff --git a/source3/smbd/process.c b/source3/smbd/process.c
index 1ebda79..a286702 100644
--- a/source3/smbd/process.c
+++ b/source3/smbd/process.c
@@ -664,16 +664,16 @@ static bool push_queued_message(struct smb_request *req,
 		}
 	}
 
-	msg->te = tevent_add_timer(msg->sconn->ev_ctx,
-				   msg,
-				   end_time,
-				   smbd_deferred_open_timer,
-				   msg);
-	if (!msg->te) {
-		DEBUG(0,("push_message: event_add_timed failed\n"));
-		TALLOC_FREE(msg);
-		return false;
-	}
+//	msg->te = tevent_add_timer(msg->sconn->ev_ctx,
+//				   msg,
+//				   end_time,
+//				   smbd_deferred_open_timer,
+//				   msg);
+//	if (!msg->te) {
+//		DEBUG(0,("push_message: event_add_timed failed\n"));
+//		TALLOC_FREE(msg);
+//		return false;
+//	}
 
 	DLIST_ADD_END(req->sconn->deferred_open_queue, msg,
 		      struct pending_message_list *);
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index c239ccb..404c93d 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -1460,14 +1460,14 @@ bool push_deferred_open_message_smb2(struct smbd_smb2_request *smb2req,
 				true) ));
 
 	state->open_was_deferred = true;
-	state->te = tevent_add_timer(smb2req->sconn->ev_ctx,
-				state,
-				end_time,
-				smb2_deferred_open_timer,
-				smb2req);
-        if (!state->te) {
-		return false;
-	}
+//	state->te = tevent_add_timer(smb2req->sconn->ev_ctx,
+//				state,
+//				end_time,
+//				smb2_deferred_open_timer,
+//				smb2req);
+//        if (!state->te) {
+//		return false;
+//	}
 
 	/* allow this request to be canceled */
 	tevent_req_set_cancel_fn(req, smbd_smb2_create_cancel);
-- 
1.7.9.5



More information about the samba-technical mailing list