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

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>

diff --git a/source3/smbd/close.c b/source3/smbd/close.c
index 510fefc58f0acdba346e90a8933ab1e89cad190d..94678b5b8db67fb518f31e8da630ea2ed24725d5 100644 (file)
--- 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);