5404 smbfs needs mmap support

Review Request #915 — Created Feb. 24, 2018 and submitted

gwr
illumos-gate
5404
general

5404 smbfs needs mmap support

This is (mostly) work from Jilin Xpd jilinxpd@gmail.com
done during the 2012 Google Summer of Code program.
I finished it up, did some debug and testing etc.
Along the way, I tried to make it "closer" to the
NFS code from which this was derived.

Latest version is on github here:
https://github.com/gwr/illumos-gate/tree/smbcl-mmap5b

Compile things in an smbfs mount (the linker is among the long list of
evil tools that don't have a fall-back when mmap(2) is unsupported).
There are also some STC-derived tests coming (see other review).

wr@oi-test:/$ SRV=orion opt/smbclient-tests/bin/smbclienttest -c opt/smbclient-tests/runfiles/just-mmap.run 
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_001 (run as root) [00:19] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_002 (run as root) [00:29] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_003 (run as root) [00:41] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_004 (run as root) [00:42] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_005 (run as root) [00:12] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_006 (run as root) [00:10] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_007 (run as root) [00:07] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_008 (run as root) [00:02] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_009 (run as root) [00:02] [PASS]
Test: /opt/smbclient-tests/tests/smbfs/mmap/tp_mmap_010 (run as root) [00:52] [PASS]

Results Summary
PASS      10

Running Time:   00:03:40
Percent passed: 100.0%
  • 0
  • 0
  • 10
  • 3
  • 13
Description From Last Updated
seeemef@mac.com
  1. The patch from Download Diff does not apply with git-apply. What patch command should one use?

    1. The latest version of this is on github.
      repo: ssh://github.com/gwr/illumos-gate.git
      branch: smbcl-rti3f

      Note that I make new branches as I make review changes
      so that later stuff (not yet out for review) can be
      easily rebased from one branch to the next.
      (So that will not "forever" be the latest.)

    1. I left comments like this one several places in this code to facilitate comparison with the NFS code from which this was derived.
      Generally the comments describe things the NFS code did that we're not doing here, or doing differently.
      Are you finding these comments objectionable? I was hoping they'd be helpful.

    2. Oh, I didn't realize it was referring to code at another location. Perhaps rewording "here" as "at this point in nfs3_map()"?

  2. This seems like a comment TODO that was not finished.

    1. forgot these in this round. maybe next time.
      BTW, this code block was just moved...

    2. OK

  3. This and below are unhelpful comments. Something more explanatory would be welcome.

  4. 
      
jbk
  1. 
      
  2. A flags argument was added, but it doesn't seem to be used in this function

    1. Yeah, the NFS code used that flags arg for an optional NFS_NOPURGE_DNLC etc.
      This code is not currently using the DNLC, so the arg ended up unused.

      I have mixed feelings about whether to keep the arg or not.
      I like having the code stay similar to NFS, but I also like
      avoiding unused args. This is called 3 places in 2 files,
      so if you want me to kill the arg, I guess that's OK.
      Is that your preference?

    2. I'm fine with keeping the form the same, but maybe just add __unused and a comment explaining why it's there?

  3. This gets called by the directio(3C) function. I'm not sure if directio support is in scope or not (it seems like there's bits here for it), but just wanted to mention that in case it was.

    1. Yes, directio should work. I just wasn't sure which of these ioctls it needed to handle.
      I'll go check and get back to you. Thanks for having a look.

    2. IIRC, there's a couple of different ways an application can get the directio behavior -- you can set it globally on the mount for every open(2) or per fd. This I believe is for the latter. It can be handy so an app that wants directio (e.g. Oracle) can get it while other things (e.g. backup utilities) can get the normal buffered behavior.

  4. 
      
gwr
gwr
seeemef@mac.com
  1. 
      
  2. I didn't see anyplace in master...gwr_smbcl-rti3f.patch where NFS_EOF (-98) was set as an error. (Unlike e.g. in nfs_bio(), nfs3_bio(), or nfs4_bio().)

    1. Yeah, now that you mention it, that probably can go away.
      Need to look a little more at why that was there.

    2. Actually, it looks like the logic to set that error was missing and should be there.
      Fixed. Now need to re-test and figure out why that didn't break tests...

  3. I didn't see anyplace in master...gwr_smbcl-rti3f.patch where NFS_EOF (-98) was set as an error. (Unlike e.g. in nfs_bio(), nfs3_bio(), or nfs4_bio().)

  4. 
      
gwr
gwr
seeemef@mac.com
  1. 
      
  2. I'm wondering if it's suspect to ASSERT on variable whose access is guarded by mutex in the subsequent lines.

    1. I think we're OK testing that flag outside the lock. We see the same in nfs_client.c
      The lock is to make sure np->r_flags |= ... does the right thing.

  3. 
      
seeemef@mac.com
  1. Ship It!
  2. 
      
tsoome
  1. 
      
  2. could you clear the spaces while there?

  3. clear the ugliness while there?:)

  4. and more spaces...

  5. 
      
jbk
  1. With the fixing the whitespace nits that Toomas pointed out (as Dan would say, while the patient is open, might as well fix those), I'm good with this (so a conditional 'Ship It').

    1. Yeah I haven't done a cstyle cleanup yet. I usually hold those out until reviews are mostly done so those changes don't dilute the focus of reviewers.

  2. 
      
gwr
tsoome
  1. Ship It!
  2. 
      
jbk
  1. Ship It!
  2. 
      
andy_js
  1. Ship It!
  2. 
      
gwr
gwr
gwr
Review request changed

Status: Closed (submitted)

Loading...