Bug 10911 - SMB2 leases are not yet supported.
SMB2 leases are not yet supported.
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
4.2.0rc2
All All
: P5 enhancement
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on: 10921
Blocks: 10077
  Show dependency treegraph
 
Reported: 2014-11-01 04:43 UTC by Jeremy Allison
Modified: 2014-12-18 17:20 UTC (History)
6 users (show)

See Also:


Attachments
Current patchset on top of master that supports leases. (128.88 KB, patch)
2014-11-01 04:47 UTC, Jeremy Allison
no flags Details
Leases prerequisite patchset (part1) cherry-picked from master. (already pushed) (36.42 KB, patch)
2014-11-03 18:24 UTC, Jeremy Allison
metze: review+
Details
git-am patch for 4.2.0 WHATSNEW (1.88 KB, patch)
2014-11-03 22:56 UTC, Jeremy Allison
kseeger: review+
Details
Current patchset on top of master that supports leases. (128.90 KB, text/plain)
2014-11-04 18:05 UTC, Jeremy Allison
no flags Details
Original (working) patch (159.77 KB, patch)
2014-11-04 18:33 UTC, Jeremy Allison
no flags Details
Leases patch on top of master. (128.86 KB, patch)
2014-11-04 19:21 UTC, Jeremy Allison
no flags Details
Found one more case where d->num_leases was indexed by uint16_t, not uint32_t (128.85 KB, patch)
2014-11-05 00:33 UTC, Jeremy Allison
no flags Details
Latest version. (137.19 KB, patch)
2014-11-05 06:00 UTC, Jeremy Allison
no flags Details
Leases patch on top of master. (149.92 KB, patch)
2014-11-05 18:32 UTC, Jeremy Allison
no flags Details
Backport of leases patch on top of v4-2-test. (149.58 KB, patch)
2014-11-06 01:11 UTC, Jeremy Allison
no flags Details
Leases patch on top of master. (149.89 KB, patch)
2014-11-06 17:13 UTC, Jeremy Allison
no flags Details
WHATSNEW announcement for leases. (1.21 KB, patch)
2014-11-06 20:14 UTC, Jeremy Allison
kseeger: review+
Details
Leases rerequisite patch (part2) for master (720 bytes, patch)
2014-11-07 07:59 UTC, Stefan Metzmacher
jra: review+
Details
Leases rerequisite patch (part2) for master (2.06 KB, patch)
2014-11-07 08:44 UTC, Stefan Metzmacher
no flags Details
WHATSNEW announcement for leases v2 (1.18 KB, patch)
2014-11-09 20:34 UTC, Karolin Seeger
jra: review+
metze: review+
Details
Back-port from master of current leases prerequisites and tests. (172.67 KB, patch)
2014-12-04 01:40 UTC, Jeremy Allison
no flags Details
Backports for v4-2-test part1 (192.77 KB, patch)
2014-12-04 01:55 UTC, Stefan Metzmacher
jra: review+
Details
Backports for v4-2-test part2 (148.68 KB, patch)
2014-12-04 07:29 UTC, Stefan Metzmacher
jra: review+
vl: review+
Details
Additional rename test and patch for master. (7.01 KB, patch)
2014-12-04 18:21 UTC, Jeremy Allison
no flags Details
Fixed rename patch for master :-). (7.22 KB, patch)
2014-12-04 20:00 UTC, Jeremy Allison
no flags Details
Version 3 of rename patch. (17.68 KB, patch)
2014-12-05 05:59 UTC, Jeremy Allison
no flags Details
Backports for v4-2-test part3 (19.55 KB, patch)
2014-12-05 21:51 UTC, Jeremy Allison
metze: review+
Details
New data model to support rename leases on dynamic shares. (18.74 KB, patch)
2014-12-05 21:53 UTC, Jeremy Allison
no flags Details
Backports for v4-2-test part4 (21.61 KB, patch)
2014-12-09 02:57 UTC, Jeremy Allison
metze: review+
Details
Backports for v4-2-test part5 (1.16 KB, patch)
2014-12-18 07:38 UTC, Stefan Metzmacher
jra: review+
vl: review+
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2014-11-01 04:43:43 UTC
This is a placeholder bug - as a feature request/blocker for 4.2.0 until Volker's leases patch is accepted into master and back-ported.
Comment 1 Jeremy Allison 2014-11-01 04:47:46 UTC
Created attachment 10394 [details]
Current patchset on top of master that supports leases.

Adding metze to review list just to remind him :-).

Jeremy.
Comment 2 Jeremy Allison 2014-11-03 18:24:28 UTC
Created attachment 10396 [details]
Leases prerequisite patchset (part1) cherry-picked from master. (already pushed)

Metze, here is the current leases prerequisite patchset that applies to v4-2-test. I'd like to ensure this gets into the v4-2-test branch now so we can then apply the finished leases patchset on top.

If you can +1 this I'll ask Karolin in import this for v4-2-test.

Thanks,

Jeremy.
Comment 3 Stefan Metzmacher 2014-11-03 20:20:51 UTC
Comment on attachment 10396 [details]
Leases prerequisite patchset (part1) cherry-picked from master. (already pushed)

Looks, Jeremy please add a WHATSNEW.txt update for the new option.
Comment 4 Jeremy Allison 2014-11-03 20:31:01 UTC
Karolin, can you add the:

Leases prerequisite patchset cherry-picked from master.

changes to v4-2-test please ?

Once you're done re-assign it back to me to get the rest of the leases code in.

I'll upload a WHATSNEW change to mention the new option as well.

Thanks !

Jeremy.
Comment 5 Jeremy Allison 2014-11-03 22:56:59 UTC
Created attachment 10397 [details]
git-am patch for 4.2.0 WHATSNEW

Document "strict rename" in the WHATSNEW.
Comment 6 Jeremy Allison 2014-11-04 18:05:23 UTC
Created attachment 10399 [details]
Current patchset on top of master that supports leases.

Fixed compile error when have_read_oplocks got renamed introduced in the patchset that has already gone into master.
Comment 7 Jeremy Allison 2014-11-04 18:33:57 UTC
Created attachment 10400 [details]
Original (working) patch

So this is very frustrating.

With this original patch, the one that got tested at Mathworks, we pass

make test TESTS=samba3.smb2.lease

However, with the approved changes into master, and the modified leases patch that removes the already approved changes on top (that is):

https://attachments.samba.org/attachment.cgi?id=10399

we get errors in:

make test TESTS=samba3.smb2.lease

THAT WASN'T SUPPOSED TO HAPPEN ! The idea was that the logic wasn't being changed in the rewrites.

Now I have to figure out what got broken...
Comment 8 Jeremy Allison 2014-11-04 18:43:54 UTC
Of course there are no changes to source4/torture/smb2/leases.c, I checked that :-).
Comment 9 Jeremy Allison 2014-11-04 19:13:30 UTC
Comment on attachment 10399 [details]
Current patchset on top of master that supports leases.

Oh hang on a minute, I think I messed up the compile fix for:

https://attachments.samba.org/attachment.cgi?id=10399

(that's why I didn't ask for review on it :-).

Marking obsolute...
Comment 10 Jeremy Allison 2014-11-04 19:21:35 UTC
Created attachment 10401 [details]
Leases patch on top of master.

OK - when updating the additional leases patch for master to fix the compile, I removed an additional "br_lock->num_locks = 0;" statement that caused the open code logic to think all files had outstanding locks. Fixed now, phew :-).

Back to passing:

make test TESTS=samba3.smb2.lease
Comment 11 Karolin Seeger 2014-11-04 20:18:53 UTC
(In reply to Jeremy Allison from comment #4)
Done, re-assigning.
Comment 12 Karolin Seeger 2014-11-04 20:19:27 UTC
(In reply to Karolin Seeger from comment #11)
Done = pushed to autobuild-v4-2-test.
Comment 13 Jeremy Allison 2014-11-04 23:08:46 UTC
(In reply to Karolin Seeger from comment #12)

Thanks Karolin, now all we need is to get the code into master, and I'll prepare a back-port for 4.2.0.

Cheers !

Jeremy.
Comment 14 Jeremy Allison 2014-11-05 00:33:09 UTC
Created attachment 10406 [details]
Found one more case where d->num_leases was indexed by uint16_t, not uint32_t
Comment 15 Jeremy Allison 2014-11-05 00:34:28 UTC
Comment on attachment 10396 [details]
Leases prerequisite patchset (part1) cherry-picked from master. (already pushed)

Patch now applied to v4-2-test - remove from default attachments list so we can see what remains to be done.
Comment 16 Jeremy Allison 2014-11-05 00:35:06 UTC
Comment on attachment 10397 [details]
git-am patch for 4.2.0 WHATSNEW

Patch now applied to v4-2-test. Mark as obsolete so it remains clear what remains left to be done.
Comment 17 Jeremy Allison 2014-11-05 06:00:48 UTC
Created attachment 10407 [details]
Latest version.

Adds 4 new patches to the set:

s3: leases : Add smb2_lease_equal() which compares client_guids and keys.
s3: leases: Add fsp_client_guid() utility function to return the connected client guid.
s3: leases: Ensure we check client_guid as well as lease key to ensure lease identity (using smb2_lease_equal()).
s3: leases: Ensure the client guids match when doing a durable reconnect.

Should deal with some more of the issues raised in metze's WIP git branch.
Comment 18 Jeremy Allison 2014-11-05 18:32:44 UTC
Created attachment 10411 [details]
Leases patch on top of master.

OK - here is the latest patchset.

Changes include:

(1). Restoring the logic in open_file_ntcreate()
around setting lease/oplock state. Code now
looks like:

        if (file_existed) {
                /*
                 * stat opens on existing files don't get oplocks or leases.
                 *
                 * Note that we check for stat open on the *open_access_mask*,
                 * i.e. the access mask we actually used to do the open,
                 * not the one the client asked for (which is in
                 * fsp->access_mask). This is due to the fact that
                 * FILE_OVERWRITE and FILE_OVERWRITE_IF add in O_TRUNC,
                 * which adds FILE_WRITE_DATA to open_access_mask.
                 */
                if (is_stat_open(open_access_mask)) {
                        if (lease) {
                                lease->lease_state = SMB2_LEASE_NONE;
                        } else {
                                oplock_request = NO_OPLOCK;
                        }
                }
        }

and I removed the code inside grant_fsp_oplock_type()
that was doing the same thing based on create_disposition.

This logic is now *identical* (modulo leases change) to
the logic that is currently in master.

(2). Addition of 2 new tests, smb2.oplock test batch9a and
raw.oplock test batch9a that are expansion of your
test hacks into new test functions. This patchset passes them.

(3) Change the error code in downgrade_share_lease()
from NT_STATUS_INVALID_PARAMETER to NT_STATUS_REQUEST_NOT_ACCEPTED
if the client is responding with a superset of what we
allow. Matches Windows return code.
Comment 19 Jeremy Allison 2014-11-06 01:11:29 UTC
Created attachment 10412 [details]
Backport of leases patch on top of v4-2-test.
Comment 20 Jeremy Allison 2014-11-06 17:13:47 UTC
Created attachment 10415 [details]
Leases patch on top of master.

Fix compile after commit fbe40d21c8e0f5bc87635e71fb828dfc5479a1ff s3:smbd: fix file corruption using "write cache size != 0" went in.
Comment 21 Jeremy Allison 2014-11-06 17:14:40 UTC
I will update the v4-2-test back-port once Karolin has merged the previous data corruption patch to v4.2.
Comment 22 Jeremy Allison 2014-11-06 20:14:14 UTC
Created attachment 10416 [details]
WHATSNEW announcement for leases.

Karolin, feel free to review but don't push to v4-2-test until the leases code has gone in.

Thanks !

Jeremy.
Comment 23 Stefan Metzmacher 2014-11-07 07:59:20 UTC
Created attachment 10417 [details]
Leases rerequisite patch (part2) for master
Comment 24 Stefan Metzmacher 2014-11-07 08:44:51 UTC
Created attachment 10418 [details]
Leases rerequisite patch (part2) for master

This adds -Werror=return-type to avoid errors like this:
    
     ../source3/utils/status.c: In function ‘print_share_mode’:
     ../source3/utils/status.c:126:3: error: ‘return’ with no value, in function
     returning non-void [-Werror=return-type]
       return;
Comment 25 Stefan Metzmacher 2014-11-07 08:47:44 UTC
Comment on attachment 10417 [details]
Leases rerequisite patch (part2) for master

Ok, -Werror=return-type breaks other cases too. We'll add that later...
Comment 26 Jeremy Allison 2014-11-07 16:42:44 UTC
Comment on attachment 10417 [details]
Leases rerequisite patch (part2) for master

LGTM. Pushed to master.
Comment 27 Jeremy Allison 2014-11-07 16:50:16 UTC
Karolin, bouncing this over to you to add attachment 10417 [details] - Leases rerequisite patch (part2) for master to v4-2-test. Once you've done bounce it back to me so I can continue to hound Metze to review the rest of the code :-).

Thanks !

Jeremy.
Comment 28 Stefan Metzmacher 2014-11-07 17:10:44 UTC
(In reply to Jeremy Allison from comment #27)

Karolin, please wait until the individual patches are and master
and use git cherry-pick -x for the release branch, thanks!
Comment 29 Karolin Seeger 2014-11-09 20:28:09 UTC
(In reply to Stefan (metze) Metzmacher from comment #28)
I cherry-picked and pushed the Leases prerequisite patches (part2).
That's only one patch. So I am not 100% certain if I missed anything.
Please check!

(All other patches don't apply/don't have a review flag).
Comment 30 Karolin Seeger 2014-11-09 20:34:57 UTC
Created attachment 10421 [details]
WHATSNEW announcement for leases v2

-Move after "Larger IO sizes for SMB2/3 by default" (original patch did not apply (context))
-Change headline (remove "New feature", because it's in the section "NEW FEATURES")
Comment 31 Jeremy Allison 2014-11-10 04:44:02 UTC
Comment on attachment 10421 [details]
WHATSNEW announcement for leases v2

LGTM.
Comment 32 Jeremy Allison 2014-12-04 01:40:59 UTC
Created attachment 10478 [details]
Back-port from master of current leases prerequisites and tests.
Comment 33 Stefan Metzmacher 2014-12-04 01:55:17 UTC
Created attachment 10479 [details]
Backports for v4-2-test part1
Comment 34 Stefan Metzmacher 2014-12-04 01:59:13 UTC
Comment on attachment 10421 [details]
WHATSNEW announcement for leases v2

I think we should make it more clear that this is an experimental feature,
as we did with SMB2_02 for v3-6
Comment 35 Jeremy Allison 2014-12-04 05:12:31 UTC
OK, I'll look at the text I used for 3.6 and see if I can adapt it for 4.2.0.
Comment 36 Jeremy Allison 2014-12-04 05:33:24 UTC
Comment on attachment 10479 [details]
Backports for v4-2-test part1

LGTM.
Comment 37 Jeremy Allison 2014-12-04 05:35:35 UTC
Karolin, can you apply the attachment:

"Backports for v4-2-test part1"

https://attachments.samba.org/attachment.cgi?id=10479

to the v4-2-test tree please which will sync us up with pre-leases code master ? Once that's done, we'll backport the next (nearly final) part of the leases code if it goes successfully into master via autobuild.

Thanks !

Jeremy.
Comment 38 Stefan Metzmacher 2014-12-04 07:29:15 UTC
Created attachment 10481 [details]
Backports for v4-2-test part2
Comment 39 Jeremy Allison 2014-12-04 16:57:37 UTC
Comment on attachment 10481 [details]
Backports for v4-2-test part2

LGTM. Karolin please push to v4-2-test.
Comment 40 Jeremy Allison 2014-12-04 18:21:09 UTC
Created attachment 10484 [details]
Additional rename test and patch for master.

Please review and push to master, then I'll do the cherry-pick for 4.2.
Comment 41 Jeremy Allison 2014-12-04 20:00:46 UTC
Created attachment 10486 [details]
Fixed rename patch for master :-).

Fixed version (iterates over leases
array, not share mode entry array as we
have multiple share mode entries pointing
to one lease array entry).
Comment 42 Karolin Seeger 2014-12-04 20:13:23 UTC
(In reply to Jeremy Allison from comment #39)
Pushed Backports for v4-2-test part1 and 2 to autobuild-v4-2-test.
Comment 43 Jeremy Allison 2014-12-05 05:59:40 UTC
Created attachment 10488 [details]
Version 3 of rename patch.

Makes SMB2 setinfo async for the SMB2_FILE_RENAME_INFORMATION_INTERNAL call and includes a test that shows why we need it.
Comment 44 Jeremy Allison 2014-12-05 21:51:35 UTC
Created attachment 10492 [details]
Backports for v4-2-test part3

Cherry-pick of code that went into master.
Comment 45 Jeremy Allison 2014-12-05 21:53:53 UTC
Created attachment 10493 [details]
New data model to support rename leases on dynamic shares.

What I proposed for master. Implements Metze's new data model for leases.db

Please take a look - thanks !
Comment 46 Jeremy Allison 2014-12-07 16:52:17 UTC
(In reply to Jeremy Allison from comment #44)

Karolin, can you push attachment 10492 [details] ( Backports for v4-2-test part3 ) to v4-2-test please ?

Then we only need Metze's approval on the new data model patch going into master (or his version of it :-) and a back-port of that code and I think we're good for 4.2.0rc3 !

Cheers,

Jeremy.
Comment 47 Karolin Seeger 2014-12-07 19:22:47 UTC
(In reply to Jeremy Allison from comment #46)
Pushed to autobuild-v4-2-test.
Comment 48 Jeremy Allison 2014-12-09 02:57:16 UTC
Created attachment 10507 [details]
Backports for v4-2-test part4

Last chunk that went into master, cheery-picked for 4.2.

Once this goes in I think we can close this out !
Comment 49 Stefan Metzmacher 2014-12-09 09:54:22 UTC
Karolin, please pick part4 and close the bug...
Comment 50 Stefan Metzmacher 2014-12-09 10:27:57 UTC
Comment on attachment 10421 [details]
WHATSNEW announcement for leases v2

I think the WHATSNEW.txt is ok for 4.2.0rc3...
Comment 51 Karolin Seeger 2014-12-09 20:40:34 UTC
(In reply to Stefan (metze) Metzmacher from comment #49)
Pushed to autobuild-v4-2-test.
Comment 52 Karolin Seeger 2014-12-15 19:36:44 UTC
(In reply to Karolin Seeger from comment #51)
Pushed to v4-2-test.
Closing out bug report!

Great work, guys! Thanks! :)
Comment 53 Stefan Metzmacher 2014-12-17 09:58:58 UTC
There's a regression in the cluster case where brl_get_locks_readonly_parser()
returns uninitialized data.
Comment 54 Stefan Metzmacher 2014-12-18 07:38:29 UTC
Created attachment 10546 [details]
Backports for v4-2-test part5
Comment 55 Stefan Metzmacher 2014-12-18 09:50:27 UTC
Pushed to autobuild-v4-2-test.
Comment 56 Stefan Metzmacher 2014-12-18 15:38:32 UTC
(In reply to Stefan (metze) Metzmacher from comment #55)

Pushed to v4-2-test