Currently smbd tries a non-blocking open for a kernel oplock if there is no smbd open record the *first* time it gets a EWOULDBLOCK error from the kernel. Subsequent opens try non-blocking. This is incorrect. Without inter-process communication from the non-smbd process the best we can do is to try non-blocking opens every <poll> time-periods (where my heuristic is poll=1 second) until either the break timeout period expires of we get success. Patch to follow.
Arg. The above comment should say: "Subsequent opens try a blocking open" of course.
Created attachment 13763 [details] git-am fix for master. Includes regression test case. NB. The patch attached to bug: https://bugzilla.samba.org/show_bug.cgi?id=13058 must be applied first.
Generally looks good. This is not a formal review yet, I'd like to take a closer look tomorrow. One thing that itches me is: anyone actually bothered testing whether we call poll that damn oplocked fd?
My regression test patch tests that case doesn't it ? If you run at debug level 10 you can certainly see the smbd polling for an allowed open every 1 second. Or am I not understanding your test case ?
(In reply to Jeremy Allison from comment #4) poll as in poll()? I don't see that in the test, but maybe I'm missing something. I'll take a closer look tomorrow.
I don't understand. Who would call poll() ? The test code forks, then the child process takes the oplock locally and just waits for a signal from the kernel using pause(). The parent client process uses smb2 calls to drive the smbd server to request an open on the oplocked file. That then causes pause() to return (got a signal) and we check it was a RT_SIGNAL meaning the oplock was requested to be broken. The child process waits 3 seconds to allow the parent to check that the requesting smbd isn't blocked by doing another SMB2_CREATE request, then just removes the lease and exits. Who needs to poll() ?
Are you confused by my initial comment where I say: "the best we can do is to try non-blocking opens every <poll> time-periods (where my heuristic is poll=1 second)" ? I don't mean poll() the system call here, but poll in the generic English "polling for a change" sense. And yeah, I tested that the smbd does do the check every 1 second whilst the non-smbd process has the file oplocked. I did this by doing a debug level 10 and counting the number of: DBG_DEBUG("kernel_oplock_poll_open_timer fired. Retying open !\n"); message I got from the timer firing. And yeah, it was correct :-).
I meant what I asked: can we poll() a file fd such that when poll() indicates readability the kernel oplock was released by the other process. That would give us a signalling mechanism and we don't have to do a sleep/retry dance. This is not about the test, if that worked we could use it in smbd. I guess it does not work, but I was wondering if anyone ever actually tested this.
Sadly no, I don't believe that works. I think a periodic wakeup and retry is the best we can do. I'm happy to write test code and check though once I'm dug out of my current work hole :-). In the meantime, I still think my patch is good :-).
(In reply to Jeremy Allison from comment #9) Thinking about it some, that *can't* work. Remember, we don't even get an fd to poll on until the open completes. We just get -1, EWOULDBLOCK back when trying to open the leased file. So yep, doing a 1 second check is the best we can do without cooperation from the holding process. Cheer up, I've done worse things in Samba in the past :-).
(In reply to Jeremy Allison from comment #10) Wuahahaha, I shouldn't come up with clever ideas late at night! :)))
Comment on attachment 13763 [details] git-am fix for master. Lgtm & pushed.
Created attachment 13775 [details] git-am fix for 4.7.next. Cherry-picked from master.
Created attachment 13776 [details] git-am fix for 4.6.next. Back-port from master.
Reassigning to Karolin for inclusion in 4.6 and 4.7.
(In reply to Ralph Böhme from comment #15) Pushed to autobuild-v4-{7,6}-test.
(In reply to Karolin Seeger from comment #16) Pushed to both branches. Closing out bug report. Thanks!