s3:smbd: make use of share_mode_entry_prepare_{lock_add,unlock}() in open_{file_ntcre...
authorStefan Metzmacher <metze@samba.org>
Mon, 29 Aug 2022 14:48:04 +0000 (16:48 +0200)
committerJeremy Allison <jra@samba.org>
Tue, 20 Sep 2022 00:34:36 +0000 (00:34 +0000)
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 something 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]

(only this commit with the close part reverted) to:

    open[num/s=12722,avslat=0.009548,minlat=0.000051,maxlat=0.054338]
    close[num/s=12720,avslat=0.010701,minlat=0.000033,maxlat=0.054372]

(with both patches) to:

    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]

So we are finally perform similar like we did in Samba 4.12,
which resulted in:

    open[num/s=36846,avslat=0.003574,minlat=0.000043,maxlat=0.020378]
    close[num/s=36844,avslat=0.003552,minlat=0.000026,maxlat=0.020321]

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>
source3/smbd/open.c

index 9ecd9194375fbc0a5a9347452967390c47f1feba..9d85b0d8ef866bc666cd273a969910555eaccc3a 100644 (file)
@@ -3730,6 +3730,88 @@ static int calculate_open_access_flags(uint32_t access_mask,
        return O_RDWR;
 }
 
+struct open_ntcreate_lock_state {
+       struct share_mode_entry_prepare_state prepare_state;
+       struct files_struct *fsp;
+       const char *object_type;
+       struct smb_request *req;
+       uint32_t create_disposition;
+       uint32_t access_mask;
+       uint32_t share_access;
+       int oplock_request;
+       const struct smb2_lease *lease;
+       bool first_open_attempt;
+       bool keep_locked;
+       NTSTATUS status;
+       struct timespec write_time;
+       share_mode_entry_prepare_unlock_fn_t cleanup_fn;
+};
+
+static void open_ntcreate_lock_add_entry(struct share_mode_lock *lck,
+                                        bool *keep_locked,
+                                        void *private_data)
+{
+       struct open_ntcreate_lock_state *state =
+               (struct open_ntcreate_lock_state *)private_data;
+
+       /*
+        * By default drop the g_lock again if we leave the
+        * tdb chainlock.
+        */
+       *keep_locked = false;
+
+       state->status = check_and_store_share_mode(state->fsp,
+                                                  state->req,
+                                                  lck,
+                                                  state->create_disposition,
+                                                  state->access_mask,
+                                                  state->share_access,
+                                                  state->oplock_request,
+                                                  state->lease,
+                                                  state->first_open_attempt);
+       if (!NT_STATUS_IS_OK(state->status)) {
+               return;
+       }
+
+       state->write_time = get_share_mode_write_time(lck);
+
+       /*
+        * keep the g_lock while existing the tdb chainlock,
+        * we we're asked to, which mean we'll keep
+        * the share_mode_lock during object creation,
+        * or setting delete on close.
+        */
+       *keep_locked = state->keep_locked;
+}
+
+static void open_ntcreate_lock_cleanup_oplock(struct share_mode_lock *lck,
+                                             void *private_data)
+{
+       struct open_ntcreate_lock_state *state =
+               (struct open_ntcreate_lock_state *)private_data;
+       bool ok;
+
+       ok = remove_share_oplock(lck, state->fsp);
+       if (!ok) {
+               DBG_ERR("Could not remove oplock for %s %s\n",
+                       state->object_type, fsp_str_dbg(state->fsp));
+       }
+}
+
+static void open_ntcreate_lock_cleanup_entry(struct share_mode_lock *lck,
+                                            void *private_data)
+{
+       struct open_ntcreate_lock_state *state =
+               (struct open_ntcreate_lock_state *)private_data;
+       bool ok;
+
+       ok = del_share_mode(lck, state->fsp);
+       if (!ok) {
+               DBG_ERR("Could not delete share entry for %s %s\n",
+                       state->object_type, fsp_str_dbg(state->fsp));
+       }
+}
+
 /****************************************************************************
  Open a file with a share mode. Passed in an already created files_struct *.
 ****************************************************************************/
@@ -3763,12 +3845,14 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
        mode_t unx_mode = (mode_t)0;
        int info;
        uint32_t existing_dos_attributes = 0;
-       struct share_mode_lock *lck = NULL;
+       struct open_ntcreate_lock_state lck_state = {};
+       bool keep_locked = false;
        uint32_t open_access_mask = access_mask;
        NTSTATUS status;
        SMB_STRUCT_STAT saved_stat = smb_fname->st;
        struct timespec old_write_time;
        bool setup_poll = false;
+       NTSTATUS ulstatus;
 
        if (conn->printer) {
                /*
@@ -4230,34 +4314,55 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
                }
        }
 
-       lck = get_share_mode_lock(talloc_tos(), fsp->file_id,
-                                 conn->connectpath,
-                                 smb_fname, &old_write_time);
+       /*
+        * If we created a new file, overwrite an existing one
+        * or going to delete it later, we should keep
+        * the share_mode_lock (g_lock) until we call
+        * share_mode_entry_prepare_unlock()
+        */
+       if (info != FILE_WAS_OPENED) {
+               keep_locked = true;
+       } else if (create_options & FILE_DELETE_ON_CLOSE) {
+               keep_locked = true;
+       }
+
+       lck_state = (struct open_ntcreate_lock_state) {
+               .fsp                    = fsp,
+               .object_type            = "file",
+               .req                    = req,
+               .create_disposition     = create_disposition,
+               .access_mask            = access_mask,
+               .share_access           = share_access,
+               .oplock_request         = oplock_request,
+               .lease                  = lease,
+               .first_open_attempt     = first_open_attempt,
+               .keep_locked            = keep_locked,
+       };
 
-       if (lck == NULL) {
-               DEBUG(0, ("open_file_ntcreate: Could not get share "
-                         "mode lock for %s\n",
-                         smb_fname_str_dbg(smb_fname)));
+       status = share_mode_entry_prepare_lock_add(&lck_state.prepare_state,
+                                                  fsp->file_id,
+                                                  conn->connectpath,
+                                                  smb_fname,
+                                                  &old_write_time,
+                                                  open_ntcreate_lock_add_entry,
+                                                  &lck_state);
+       if (!NT_STATUS_IS_OK(status)) {
+               DBG_ERR("share_mode_entry_prepare_lock_add() failed for %s - %s\n",
+                       smb_fname_str_dbg(smb_fname), nt_errstr(status));
                fd_close(fsp);
-               return NT_STATUS_SHARING_VIOLATION;
+               return status;
        }
 
-       status = check_and_store_share_mode(
-               fsp,
-               req,
-               lck,
-               create_disposition,
-               access_mask,
-               share_access,
-               oplock_request,
-               lease,
-               first_open_attempt);
+       status = lck_state.status;
        if (!NT_STATUS_IS_OK(status)) {
-               TALLOC_FREE(lck);
                fd_close(fsp);
                return status;
        }
 
+       /*
+        * From here we need to use 'goto unlock;' instead of return !!!
+        */
+
        if (fsp->oplock_type != NO_OPLOCK && fsp->oplock_type != LEASE_OPLOCK) {
                /*
                 * Now ask for kernel oplocks
@@ -4268,7 +4373,8 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
                        /*
                         * Could not get the kernel oplock
                         */
-                       remove_share_oplock(lck, fsp);
+                       lck_state.cleanup_fn =
+                               open_ntcreate_lock_cleanup_oplock;
                        fsp->oplock_type = NO_OPLOCK;
                }
        }
@@ -4282,10 +4388,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
                ret = SMB_VFS_FTRUNCATE(fsp, 0);
                if (ret != 0) {
                        status = map_nt_error_from_unix(errno);
-                       del_share_mode(lck, fsp);
-                       TALLOC_FREE(lck);
-                       fd_close(fsp);
-                       return status;
+                       lck_state.cleanup_fn =
+                               open_ntcreate_lock_cleanup_entry;
+                       goto unlock;
                }
                notify_fname(fsp->conn, NOTIFY_ACTION_MODIFIED,
                             FILE_NOTIFY_CHANGE_SIZE
@@ -4303,10 +4408,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
            !fsp_is_alternate_stream(fsp)) {
                status = delete_all_streams(conn, smb_fname);
                if (!NT_STATUS_IS_OK(status)) {
-                       del_share_mode(lck, fsp);
-                       TALLOC_FREE(lck);
-                       fd_close(fsp);
-                       return status;
+                       lck_state.cleanup_fn =
+                               open_ntcreate_lock_cleanup_entry;
+                       goto unlock;
                }
        }
 
@@ -4328,11 +4432,10 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
                                                   share_access,
                                                   access_mask);
                if (ret == -1){
-                       del_share_mode(lck, fsp);
-                       TALLOC_FREE(lck);
-                       fd_close(fsp);
-
-                       return NT_STATUS_SHARING_VIOLATION;
+                       status = NT_STATUS_SHARING_VIOLATION;
+                       lck_state.cleanup_fn =
+                               open_ntcreate_lock_cleanup_entry;
+                       goto unlock;
                }
 
                fsp->fsp_flags.kernel_share_modes_taken = true;
@@ -4373,10 +4476,9 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
 
                        if (!NT_STATUS_IS_OK(status)) {
                                /* Remember to delete the mode we just added. */
-                               del_share_mode(lck, fsp);
-                               TALLOC_FREE(lck);
-                               fd_close(fsp);
-                               return status;
+                               lck_state.cleanup_fn =
+                                       open_ntcreate_lock_cleanup_entry;
+                               goto unlock;
                        }
                }
                /* Note that here we set the *initial* delete on close flag,
@@ -4456,18 +4558,29 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
                }
        }
 
-       {
-               /*
-                * Deal with other opens having a modified write time.
-                */
-               struct timespec write_time = get_share_mode_write_time(lck);
+       /*
+        * Deal with other opens having a modified write time.
+        */
+       if (!is_omit_timespec(&lck_state.write_time)) {
+               update_stat_ex_mtime(&fsp->fsp_name->st, lck_state.write_time);
+       }
 
-               if (!is_omit_timespec(&write_time)) {
-                       update_stat_ex_mtime(&fsp->fsp_name->st, write_time);
-               }
+       status = NT_STATUS_OK;
+
+unlock:
+       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",
+                       smb_fname_str_dbg(smb_fname), nt_errstr(ulstatus));
+               smb_panic("share_mode_entry_prepare_unlock() failed!");
        }
 
-       TALLOC_FREE(lck);
+       if (!NT_STATUS_IS_OK(status)) {
+               fd_close(fsp);
+               return status;
+       }
 
        return NT_STATUS_OK;
 }
@@ -4626,11 +4739,13 @@ static NTSTATUS open_directory(connection_struct *conn,
 {
        struct smb_filename *smb_dname = fsp->fsp_name;
        bool dir_existed = VALID_STAT(smb_dname->st);
-       struct share_mode_lock *lck = NULL;
+       struct open_ntcreate_lock_state lck_state = {};
+       bool keep_locked = false;
        NTSTATUS status;
        struct timespec mtimespec;
        int info = 0;
        uint32_t need_fd_access;
+       NTSTATUS ulstatus;
 
        if (is_ntfs_stream_smb_fname(smb_dname)) {
                DEBUG(2, ("open_directory: %s is a stream name!\n",
@@ -4889,42 +5004,62 @@ static NTSTATUS open_directory(connection_struct *conn,
                }
        }
 
-       lck = get_share_mode_lock(talloc_tos(), fsp->file_id,
-                                 conn->connectpath, smb_dname,
-                                 &mtimespec);
+       /*
+        * If we created a new directory or going to delete it later,
+        * we should keep * the share_mode_lock (g_lock) until we call
+        * share_mode_entry_prepare_unlock()
+        */
+       if (info != FILE_WAS_OPENED) {
+               keep_locked = true;
+       } else if (create_options & FILE_DELETE_ON_CLOSE) {
+               keep_locked = true;
+       }
+
+       lck_state = (struct open_ntcreate_lock_state) {
+               .fsp                    = fsp,
+               .object_type            = "directory",
+               .req                    = req,
+               .create_disposition     = create_disposition,
+               .access_mask            = access_mask,
+               .share_access           = share_access,
+               .oplock_request         = NO_OPLOCK,
+               .lease                  = NULL,
+               .first_open_attempt     = true,
+               .keep_locked            = keep_locked,
+       };
 
-       if (lck == NULL) {
-               DEBUG(0, ("open_directory: Could not get share mode lock for "
-                         "%s\n", smb_fname_str_dbg(smb_dname)));
+       status = share_mode_entry_prepare_lock_add(&lck_state.prepare_state,
+                                                  fsp->file_id,
+                                                  conn->connectpath,
+                                                  smb_dname,
+                                                  &mtimespec,
+                                                  open_ntcreate_lock_add_entry,
+                                                  &lck_state);
+       if (!NT_STATUS_IS_OK(status)) {
+               DBG_ERR("share_mode_entry_prepare_lock_add() failed for %s - %s\n",
+                       smb_fname_str_dbg(smb_dname), nt_errstr(status));
                fd_close(fsp);
-               return NT_STATUS_SHARING_VIOLATION;
+               return status;
        }
 
-       status = check_and_store_share_mode(
-               fsp,
-               req,
-               lck,
-               create_disposition,
-               access_mask,
-               share_access,
-               NO_OPLOCK,
-               NULL,  /* lease */
-               true); /* first_open_attempt */
+       status = lck_state.status;
        if (!NT_STATUS_IS_OK(status)) {
-               TALLOC_FREE(lck);
                fd_close(fsp);
                return status;
        }
 
+       /*
+        * From here we need to use 'goto unlock;' instead of return !!!
+        */
+
        /* For directories the delete on close bit at open time seems
           always to be honored on close... See test 19 in Samba4 BASE-DELETE. */
        if (create_options & FILE_DELETE_ON_CLOSE) {
                status = can_set_delete_on_close(fsp, 0);
                if (!NT_STATUS_IS_OK(status) && !NT_STATUS_EQUAL(status, NT_STATUS_DIRECTORY_NOT_EMPTY)) {
-                       del_share_mode(lck, fsp);
-                       TALLOC_FREE(lck);
-                       fd_close(fsp);
-                       return status;
+                       lck_state.cleanup_fn =
+                               open_ntcreate_lock_cleanup_entry;
+                       goto unlock;
                }
 
                if (NT_STATUS_IS_OK(status)) {
@@ -4934,24 +5069,34 @@ static NTSTATUS open_directory(connection_struct *conn,
                }
        }
 
-       {
-               /*
-                * Deal with other opens having a modified write time. Is this
-                * possible for directories?
-                */
-               struct timespec write_time = get_share_mode_write_time(lck);
-
-               if (!is_omit_timespec(&write_time)) {
-                       update_stat_ex_mtime(&fsp->fsp_name->st, write_time);
-               }
+       /*
+        * Deal with other opens having a modified write time.
+        */
+       if (!is_omit_timespec(&lck_state.write_time)) {
+               update_stat_ex_mtime(&fsp->fsp_name->st, lck_state.write_time);
        }
 
-       TALLOC_FREE(lck);
-
        if (pinfo) {
                *pinfo = info;
        }
 
+       status = NT_STATUS_OK;
+
+unlock:
+       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",
+                       smb_fname_str_dbg(smb_dname), nt_errstr(ulstatus));
+               smb_panic("share_mode_entry_prepare_unlock() failed!");
+       }
+
+       if (!NT_STATUS_IS_OK(status)) {
+               fd_close(fsp);
+               return status;
+       }
+
        return NT_STATUS_OK;
 }