s3:smbd: make use of share_mode_entry_prepare_{lock_del,unlock}() in close_{remove_share_mode,directory}()

This gives a nice speed up...

The following test with 256 commections all looping with open/close
on the same inode (share root) is improved drastically:

  smbtorture //127.0.0.1/m -Uroot%test smb2.bench.path-contention-shared \
         --option='torture:bench_path=' \
         --option="torture:timelimit=60" \
         --option="torture:nprocs=256" \
         --option="torture:qdepth=1"

From some like this:

    open[num/s=11536,avslat=0.011450,minlat=0.000039,maxlat=0.052707]
    close[num/s=11534,avslat=0.010878,minlat=0.000022,maxlat=0.052342]

to:
    open[num/s=13225,avslat=0.010504,minlat=0.000042,maxlat=0.054023]
    close[num/s=13223,avslat=0.008971,minlat=0.000022,maxlat=0.053838]

But this is only half of the solution, the next commits will
add a similar optimization to the open code, at the end we'll
perform like we did in Samba 4.12:

    open[num/s=37680,avslat=0.003471,minlat=0.000040,maxlat=0.061411]
    close[num/s=37678,avslat=0.003440,minlat=0.000022,maxlat=0.051536]

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15125

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

1 files changed, 56 insertions, 30 deletions

diff --git a/source3/smbd/close.c b/source3/smbd/close.c
index 510fefc58f0..94678b5b8db 100644
--- a/source3/smbd/close.c
+++ b/source3/smbd/close.c
@@ -284,17 +284,25 @@ struct close_share_mode_lock_state {
 	const struct security_unix_token *del_token;
 	const struct security_token *del_nt_token;
 	bool reset_delete_on_close;
-	void (*cleanup_fn)(struct share_mode_lock *lck,
-			   struct close_share_mode_lock_state *state);
+	share_mode_entry_prepare_unlock_fn_t cleanup_fn;
 };
 
 static void close_share_mode_lock_prepare(struct share_mode_lock *lck,
-					  struct close_share_mode_lock_state *state)
+					  bool *keep_locked,
+					  void *private_data)
 {
+	struct close_share_mode_lock_state *state =
+		(struct close_share_mode_lock_state *)private_data;
 	struct files_struct *fsp = state->fsp;
 	bool normal_close;
 	bool ok;
 
+	/*
+	 * By default drop the g_lock again if we leave the
+	 * tdb chainlock.
+	 */
+	*keep_locked = false;
+
 	if (fsp->oplock_type != NO_OPLOCK) {
 		ok = remove_share_oplock(lck, fsp);
 		if (!ok) {
@@ -371,6 +379,14 @@ static void close_share_mode_lock_prepare(struct share_mode_lock *lck,
 		return;
 	}
 
+	/*
+	 * We're going to remove the file/directory
+	 * so keep the g_lock after the tdb chainlock
+	 * is left, so we hold the share_mode_lock
+	 * also during the deletion
+	 */
+	*keep_locked = true;
+
 	state->got_tokens = get_delete_on_close_token(lck, fsp->name_hash,
 					&state->del_nt_token, &state->del_token);
 	if (state->close_type != ERROR_CLOSE) {
@@ -379,8 +395,10 @@ static void close_share_mode_lock_prepare(struct share_mode_lock *lck,
 }
 
 static void close_share_mode_lock_cleanup(struct share_mode_lock *lck,
-					  struct close_share_mode_lock_state *state)
+					  void *private_data)
 {
+	struct close_share_mode_lock_state *state =
+		(struct close_share_mode_lock_state *)private_data;
 	struct files_struct *fsp = state->fsp;
 	bool ok;
 
@@ -405,9 +423,9 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp,
 	connection_struct *conn = fsp->conn;
 	struct close_share_mode_lock_state lck_state = {};
 	bool changed_user = false;
-	struct share_mode_lock *lck = NULL;
 	NTSTATUS status = NT_STATUS_OK;
 	NTSTATUS tmp_status;
+	NTSTATUS ulstatus;
 	struct file_id id;
 	struct smb_filename *parent_fname = NULL;
 	struct smb_filename *base_fname = NULL;
@@ -424,20 +442,21 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp,
 	 * This prevents race conditions with the file being created. JRA.
 	 */
 
-	lck = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);
-	if (lck == NULL) {
-		DEBUG(0, ("close_remove_share_mode: Could not get share mode "
-			  "lock for file %s\n", fsp_str_dbg(fsp)));
-		return NT_STATUS_INVALID_PARAMETER;
-	}
-
 	lck_state = (struct close_share_mode_lock_state) {
 		.fsp			= fsp,
 		.object_type		= "file",
 		.close_type		= close_type,
 	};
 
-	close_share_mode_lock_prepare(lck, &lck_state);
+	status = share_mode_entry_prepare_lock_del(&lck_state.prepare_state,
+						   fsp->file_id,
+						   close_share_mode_lock_prepare,
+						   &lck_state);
+	if (!NT_STATUS_IS_OK(status)) {
+		DBG_ERR("share_mode_entry_prepare_lock_del() failed for %s - %s\n",
+			fsp_str_dbg(fsp), nt_errstr(status));
+		return status;
+	}
 
 	/* Remove the oplock before potentially deleting the file. */
 	if (fsp->oplock_type != NO_OPLOCK) {
@@ -608,12 +627,15 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp,
 		}
 	}
 
-	if (lck_state.cleanup_fn != NULL) {
-		lck_state.cleanup_fn(lck, &lck_state);
+	ulstatus = share_mode_entry_prepare_unlock(&lck_state.prepare_state,
+						   lck_state.cleanup_fn,
+						   &lck_state);
+	if (!NT_STATUS_IS_OK(ulstatus)) {
+		DBG_ERR("share_mode_entry_prepare_unlock() failed for %s - %s\n",
+			fsp_str_dbg(fsp), nt_errstr(ulstatus));
+		smb_panic("share_mode_entry_prepare_unlock() failed!");
 	}
 
-	TALLOC_FREE(lck);
-
 	if (lck_state.delete_object) {
 		/*
 		 * Do the notification after we released the share
@@ -1457,12 +1479,12 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp,
 				enum file_close_type close_type)
 {
 	connection_struct *conn = fsp->conn;
-	struct share_mode_lock *lck = NULL;
 	struct close_share_mode_lock_state lck_state = {};
 	bool changed_user = false;
 	NTSTATUS status = NT_STATUS_OK;
 	NTSTATUS status1 = NT_STATUS_OK;
 	NTSTATUS notify_status;
+	NTSTATUS ulstatus;
 
 	SMB_ASSERT(fsp->fsp_flags.is_fsa);
 
@@ -1479,20 +1501,21 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp,
 	 * reference to a directory also.
 	 */
 
-	lck = get_existing_share_mode_lock(talloc_tos(), fsp->file_id);
-	if (lck == NULL) {
-		DEBUG(0, ("close_directory: Could not get share mode lock for "
-			  "%s\n", fsp_str_dbg(fsp)));
-		return NT_STATUS_INVALID_PARAMETER;
-	}
-
 	lck_state = (struct close_share_mode_lock_state) {
 		.fsp			= fsp,
 		.object_type		= "directory",
 		.close_type		= close_type,
 	};
 
-	close_share_mode_lock_prepare(lck, &lck_state);
+	status = share_mode_entry_prepare_lock_del(&lck_state.prepare_state,
+						   fsp->file_id,
+						   close_share_mode_lock_prepare,
+						   &lck_state);
+	if (!NT_STATUS_IS_OK(status)) {
+		DBG_ERR("share_mode_entry_prepare_lock_del() failed for %s - %s\n",
+			fsp_str_dbg(fsp), nt_errstr(status));
+		return status;
+	}
 
 	/*
 	 * We don't have directory leases yet, so assert it in order
@@ -1569,12 +1592,15 @@ done:
 		pop_sec_ctx();
 	}
 
-	if (lck_state.cleanup_fn != NULL) {
-		lck_state.cleanup_fn(lck, &lck_state);
+	ulstatus = share_mode_entry_prepare_unlock(&lck_state.prepare_state,
+						   lck_state.cleanup_fn,
+						   &lck_state);
+	if (!NT_STATUS_IS_OK(ulstatus)) {
+		DBG_ERR("share_mode_entry_prepare_unlock() failed for %s - %s\n",
+			fsp_str_dbg(fsp), nt_errstr(ulstatus));
+		smb_panic("share_mode_entry_prepare_unlock() failed!");
 	}
 
-	TALLOC_FREE(lck);
-
 	remove_pending_change_notify_requests_by_fid(fsp, notify_status);
 
 	status1 = fd_close(fsp);