Project

General

Profile

Actions

Bug #13227

closed

SMB server serializes writes where it should not

Added by Gordon Ross 7 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
-
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

During some performance testing, we discovered that the SMB server is serializing writes on a given file handle where it does not need to do so. This has fairly severe impact in performance tests that issue multiple writes on one file handle. (Sorry, don't have the details handy. Latency reduced about 50%)

The reason for this serialization is simply that some prior work accidentally used RW_WRITER where RW_READER was more suitable.
The fix is a one-liner:

diff -r be29f39333c9 usr/src/uts/common/fs/smbsrv/smb_fsops.c
--- a/usr/src/uts/common/fs/smbsrv/smb_fsops.c    Thu Jul 02 21:48:55 2020 +0000
+++ b/usr/src/uts/common/fs/smbsrv/smb_fsops.c    Fri Jul 03 13:01:50 2020 +0000
@@ -1539,7 +1539,7 @@ smb_fsop_write(
         cr = kcr;
     }

-    smb_node_start_crit(snode, RW_WRITER);
+    smb_node_start_crit(snode, RW_READER);
     rc = nbl_svmand(vp, kcr, &svmand);
     if (rc) {
         smb_node_end_crit(snode);

As per the nbmand locking design, the caller needs to acquire lock on the nbmand state in 'RW_WRITER' mode only if needs to obtain a byte-range lock or share reservation/share mode. For e.g. fs_shrlock() or fs_frlock(). In all other cases including writes/setattr/remove operations, the caller should acquire the lock in 'RW_READER' mode as it is not changing the the byte-range lock/share reservations state.

Actions #1

Updated by Gordon Ross 7 months ago

  • Description updated (diff)
Actions #2

Updated by Adam Stylinski 7 months ago

Maybe this is at least indirectly related to incident #13138?

Actions #3

Updated by Toomas Soome 7 months ago

Gordon Ross wrote:

During some performance testing, we discovered that the SMB server is serializing writes on a given file handle where it does not need to do so. This has fairly severe impact in performance tests that issue multiple writes on one file handle. (Sorry, don't have the details handy. Latency reduced about 50%)

The reason for this serialization is simply that some prior work accidentally used RW_WRITER where RW_READER was more suitable.
The fix is a one-liner:
[...]

As per the nbmand locking design, the caller needs to acquire lock on the nbmand state in 'RW_WRITER' mode only if needs to obtain a byte-range lock or share reservation/share mode. For e.g. fs_shrlock() or fs_frlock(). In all other cases including writes/setattr/remove operations, the caller should acquire the lock in 'RW_READER' mode as it is not changing the the byte-range lock/share reservations state.

Appears to work ok for me:)

Actions #4

Updated by Gordon Ross 3 months ago

Testing:
iometer with four workers, 100% write, on a VM with disks over NFS.
Before got about 35MB/sec, and after shows about 48MB/sec.

Previous tests on real hardware show nearly 2:1 improvement with parallel writes to the same file.

Actions #5

Updated by Gordon Ross 3 months ago

  • Status changed from In Progress to Pending RTI
Actions #6

Updated by Electric Monk 3 months ago

  • Status changed from Pending RTI to Closed
  • % Done changed from 0 to 100

git commit f74a127feb6111d3b6890ae374a142eda1fd4e3e

commit  f74a127feb6111d3b6890ae374a142eda1fd4e3e
Author: Gordon Ross <gordon.ross@tintri.com>
Date:   2021-02-08T20:21:58.000Z

    13227 SMB server serializes writes where it should not
    Reviewed by: Suresh Jayaraman <suresh.jayaraman@tegile.com>
    Reviewed by: Prashanth Badari <prbadari@tintri.com>
    Reviewed by: Andy Giles <andy@tegile.com>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF