Latest leases patchset - getting there !
Jeremy Allison
jra at samba.org
Thu Nov 13 00:12:01 MST 2014
On Thu, Nov 13, 2014 at 01:17:43AM +0100, Stefan (metze) Metzmacher wrote:
>
> I've integrated this with a few more tests we fail (sorry...:-)
>
> ontop17.diff.txt replaces the older ontop13.diff.txt
> and also includes your changes.
No problem. Attached are some new patches to fix the new
tests we fail :-). They apply on top of ontop17.diff.txt.
on-top-of-ontop17.patchset
The current state of play with this new code are:
request PASS
break_twice PASS
nobreakself PASS
upgrade PASS
upgrade2 PASS
upgrade3 PASS
break PASS
oplock PASS
multibreak PASS
breaking1 --- PASS WITH smbtorture MODIFICATION (*)
breaking2 --- TEST CRASHES in smbtorture.(**)
complex1 PASS
v2_epoch1 PASS
v2_epoch2 insane test - see below
v2_epoch3 insane test - see below
v2_complex1 FAIL (***) - Should be pass - irrelevent difference with Windows.
--------------------------------------------------------------------
(*) smb2.lease.breaking1 only passes for me if I comment
out the following :
diff --git a/source4/torture/smb2/lease.c b/source4/torture/smb2/lease.c
index 8dde085..a6c44e3 100644
--- a/source4/torture/smb2/lease.c
+++ b/source4/torture/smb2/lease.c
@@ -1874,8 +1874,8 @@ static bool test_lease_breaking1(struct torture_context *tctx,
CHECK_STATUS(status, NT_STATUS_OK);
CHECK_LEASE_BREAK_ACK(&ack, "RH", LEASE1);
- torture_assert(tctx, req2->cancel.can_cancel,
- "req2 can_cancel");
+// torture_assert(tctx, req2->cancel.can_cancel,
+// "req2 can_cancel");
Still investigating (but we pass so it can't be all bad :-).
--------------------------------------------------------------------
(**) smb2.lease.breaking2 just crashes inside smbtorture for me in:
test_lease_breaking2()
torture_assert(tctx, tevent_req_is_in_progress(req2->subreq),
"req2 pending");
as req2->subreq == NULL (state is DONE).
Isn't this an indication of different ordering of returns from
the server ? This is the only failure I don't understand, and
I'm still investigating.
--------------------------------------------------------------------
(***) smb2.lease.v2_complex1 passes all the lease state
tests, but returns a FAIL due to the fact that we send
the lease break on a different transport to what Windows
does.
Given TCP transports Tree1 with handle1 with key LEASE1 (RH) and Tree2 with
handle2 with key LEASE2 (RH), a write on handle1 issues a lease break
for key LEASE2 on transport Tree1 - (which is where the write came
from), whereas we issue the lease break for key LEASE2 on transport
Tree2 (the one it was actually opened on).
This is merely different from what Windows does, not wrong - as
both LEASES have the same client guid and the TCP transports are going
to the same place it doesn't actually matter.
We should note this as an implementation difference and move
on - not a blocker IMHO.
--------------------------------------------------------------------
v2_epoch2 and v2_epoch3
I have a problem with these tests. They're interesting to
work out how the Windows internal implementation works, but
they are not credible as a correct functionality test
for code we're trying hard to get into master and v4-2-test.
As far as I can tell v2_epoch2 creates a file with
a v2 lease key(R), then opens the file with an identical
v1 lease key(RH) and checks that the reply is a v2
lease reply with correct epoch !
v3_epoch2 does the reverse by creating with a v1
lease, and then doing a second open with an identical
v2 lease key on the same connection and checks the
server replies with a v1 lease response !
That might be how Windows server works internally if
there's an existing v2 or v1 lease and the client does
something silly and sends a different lease type, but
it's an insane semantic to test.
NO CLIENT WILL EVER MIX V1 AND V2 LEASE TYPES ON THE SAME
TCP CONNECTION.
If they do, then they richly deserve anything they
get back...
Congratulations for working out that Windows server
does this, but this is really make-work to implement.
It simply makes no sense to match this behavior
unless you can show me a client other than a torture
tester doing this, or give me one reason why a client
implementor might even consider doing this ?
I say we keep the tests, mark them as knownfail for
Samba and move on.
Later, if we have lots of free time, we can make
our internal implementation match Windows frame
for frame and have a party :-).
But not now we're trying to get this code DONE !
--------------------------------------------------------------------
> I'll try to add a samba only regression test for the homes share case
> next.
Great - thanks !
> BTW: I think the leasedb would be the correct place to store if
> the current lease is v1 or v2 (see the v2_epoch[2|3] tests).
See above, I really don't think we should be doing this unless
you can show me a client where this actually matters.
Jeremy.
-------------- next part --------------
From 92680d9f548c8450243fd18a4ca33926fe5b1fd1 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 12 Nov 2014 22:47:13 -0800
Subject: [PATCH 1/3] s3: leases: When processing an oplock break message, if
we need to increment the epoch remember to write it out again.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/oplock.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
index 5523de9..3e2b951 100644
--- a/source3/smbd/oplock.c
+++ b/source3/smbd/oplock.c
@@ -618,6 +618,8 @@ static void process_oplock_break_message(struct messaging_context *msg_ctx,
/* Need to increment the epoch */
o->epoch += 1;
fsp->lease->lease.lease_epoch = o->epoch;
+ /* And remember to write it out again.. */
+ lck->data->modified = true;
}
TALLOC_FREE(lck);
--
1.9.1
From b8153ea37444b0178c49c4e2daf22d69d6d53a71 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 12 Nov 2014 22:56:38 -0800
Subject: [PATCH 2/3] s3: leases : Ensure we set the lease_version correctly
from the requesting client (where we can).
It matters when sending lease break requests - the epoch
field must be correctly sent for v2 leases.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/durable.c | 15 ++++++++++++++-
source3/smbd/open.c | 12 +++++++++---
source3/smbd/proto.h | 1 +
source3/smbd/smb2_break.c | 2 +-
4 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/source3/smbd/durable.c b/source3/smbd/durable.c
index 6e668ce..28e8a42 100644
--- a/source3/smbd/durable.c
+++ b/source3/smbd/durable.c
@@ -727,11 +727,24 @@ NTSTATUS vfs_default_durable_reconnect(struct connection_struct *conn,
if (fsp->oplock_type == LEASE_OPLOCK) {
struct share_mode_oplock *o = &lck->data->leases[e->lease_idx];
struct smb2_lease_key key;
+ uint16_t lease_version;
key.data[0] = o->lease_key.data[0];
key.data[1] = o->lease_key.data[1];
- fsp->lease = find_fsp_lease(fsp, &key, o);
+ /*
+ * We don't pass down the reqesting lease
+ * version - we should. For now infer from
+ * the protocol.
+ */
+
+ if (fsp->conn->sconn->client->connections->protocol >= PROTOCOL_SMB3_00) {
+ lease_version = 2;
+ } else {
+ lease_version = 1;
+ }
+
+ fsp->lease = find_fsp_lease(fsp, &key, lease_version, o);
if (fsp->lease == NULL) {
TALLOC_FREE(lck);
fsp_free(fsp);
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index 3f576d8..a2033b2 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1560,6 +1560,7 @@ static bool is_same_lease(const files_struct *fsp,
struct fsp_lease *find_fsp_lease(files_struct *new_fsp,
const struct smb2_lease_key *key,
+ uint16_t lease_version,
const struct share_mode_oplock *o)
{
files_struct *fsp;
@@ -1593,8 +1594,12 @@ struct fsp_lease *find_fsp_lease(files_struct *new_fsp,
new_fsp->lease->ref_count = 1;
new_fsp->lease->lease.lease_key = *key;
new_fsp->lease->lease.lease_state = o->current_state;
- /* Internally treat all leases as version 2 with epoch. */
- new_fsp->lease->lease.lease_version = 2;
+ /*
+ * We internally treat all leases as V2 and update
+ * the epoch, but when sending breaks it matters if
+ * the requesting lease was v1 or v2.
+ */
+ new_fsp->lease->lease.lease_version = lease_version;
new_fsp->lease->lease.lease_epoch = o->epoch;
return new_fsp->lease;
}
@@ -1616,7 +1621,8 @@ static NTSTATUS grant_fsp_lease(files_struct *fsp, struct share_mode_data *d,
bool do_upgrade;
uint32_t existing, requested;
- fsp->lease = find_fsp_lease(fsp, &lease->lease_key, o);
+ fsp->lease = find_fsp_lease(fsp, &lease->lease_key,
+ lease->lease_version, o);
if (fsp->lease == NULL) {
DEBUG(1, ("Did not find existing lease for file %s\n",
fsp_str_dbg(fsp)));
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index ecf89a7..22dddf4 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -617,6 +617,7 @@ NTSTATUS change_dir_owner_to_parent(connection_struct *conn,
struct share_mode_oplock;
struct fsp_lease *find_fsp_lease(files_struct *new_fsp,
const struct smb2_lease_key *key,
+ uint16_t lease_version,
const struct share_mode_oplock *o);
bool is_stat_open(uint32 access_mask);
struct deferred_open_record;
diff --git a/source3/smbd/smb2_break.c b/source3/smbd/smb2_break.c
index abe6709..9ab118d 100644
--- a/source3/smbd/smb2_break.c
+++ b/source3/smbd/smb2_break.c
@@ -431,7 +431,7 @@ void send_break_message_smb2(files_struct *fsp, uint32_t break_to)
no_ack = ((fsp->lease->lease.lease_state == SMB2_LEASE_READ) &&
(break_to == SMB2_LEASE_NONE));
- if (xconn->protocol >= PROTOCOL_SMB3_00) {
+ if (fsp->lease->lease.lease_version > 1) {
new_epoch = fsp->lease->lease.lease_epoch;
} else {
new_epoch = 0;
--
1.9.1
From 994c6c45892ad641c6d6f342558361e1aa7142b5 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Wed, 12 Nov 2014 22:59:45 -0800
Subject: [PATCH 3/3] s3: leases - correctly send back
SMB2_LEASE_FLAG_BREAK_IN_PROGRESS if we're waiting for a lease break
response.
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/oplock.c | 17 +++++++++++++++++
source3/smbd/proto.h | 1 +
source3/smbd/smb2_create.c | 6 ++++++
3 files changed, 24 insertions(+)
diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
index 3e2b951..4755ca6 100644
--- a/source3/smbd/oplock.c
+++ b/source3/smbd/oplock.c
@@ -937,6 +937,23 @@ done:
return;
}
+bool break_in_progress(const files_struct *fsp)
+{
+ files_struct *new_fsp;
+
+ for (new_fsp = file_find_di_first(fsp->conn->sconn, fsp->file_id);
+ new_fsp != NULL;
+ new_fsp = file_find_di_next(new_fsp)) {
+ if (new_fsp == fsp) {
+ continue;
+ }
+ if (new_fsp->sent_oplock_break != NO_BREAK_SENT) {
+ return true;
+ }
+ }
+ return false;
+}
+
void smbd_contend_level2_oplocks_begin(files_struct *fsp,
enum level2_contention_type type)
{
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 22dddf4..f507696 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -672,6 +672,7 @@ NTSTATUS downgrade_lease(struct smbd_server_connection *sconn,
const struct file_id id,
const struct smb2_lease_key *key,
uint32_t lease_state);
+bool break_in_progress(const files_struct *fsp);
void contend_level2_oplocks_begin(files_struct *fsp,
enum level2_contention_type type);
void contend_level2_oplocks_end(files_struct *fsp,
diff --git a/source3/smbd/smb2_create.c b/source3/smbd/smb2_create.c
index 0c49c70..b076596 100644
--- a/source3/smbd/smb2_create.c
+++ b/source3/smbd/smb2_create.c
@@ -1213,6 +1213,12 @@ static struct tevent_req *smbd_smb2_create_send(TALLOC_CTX *mem_ctx,
lease.lease_flags &=
SMB2_LEASE_FLAG_PARENT_LEASE_KEY_SET;
+ /* Is there a break in progress ? */
+ if (break_in_progress(result)) {
+ lease.lease_flags |=
+ SMB2_LEASE_FLAG_BREAK_IN_PROGRESS;
+ }
+
if (!smb2_lease_push(&lease, buf, lease_len)) {
tevent_req_nterror(
req, NT_STATUS_INTERNAL_ERROR);
--
1.9.1
More information about the samba-technical
mailing list