Latest leases patchset - getting there !

[Stefan (metze) Metzmacher]

Am 18.11.2014 um 08:12 schrieb Jeremy Allison:
> On Mon, Nov 17, 2014 at 10:20:40PM -0800, Jeremy Allison wrote:
>> On Tue, Nov 18, 2014 at 02:48:26AM +0100, Stefan (metze) Metzmacher wrote:
>>>>
>>>> OK, I don't understand what you're asking for here (sorry).
>>>>
>>>> Right now the test does:
>>>>
>>>> open LEASE1 -> RWH
>>>> 	        <- RWH (H1)
>>>> open LEASE2 -> RWH
>>>> 		<- Break to RH on LEASE1
>>>> ... timout...
>>>> 		<- RH (H2)
>>>> Ack break to RH ->
>>>> 		<- NT_STATUS_UNSUCCESSFUL
>>>> Write on H1 ->
>>>> 		<- Break to NONE on LEASE2
>>>>
>>>
>>> The following in addition:
>>
>> OK - here is the updated timeout test that does the
>> changes you requested:
>>
>>> Write on H2 ->
>>>
>>>                 <- Break to NONE on LEASE1?
>>
>> Problem - Windows (W2K8) does NOT break to none on LEASE1
>> here. That just seems wrong to me. I'll test
>> with W2K12 when I get into work tomorrow.
>>
>> Currently I've added code that does the following
>> to cope with the difference.
>>
>>         if (TARGET_IS_SAMBA3(tctx)) {
>>                 CHECK_BREAK_INFO("RH", "", LEASE1);
>>         } else {
>>                 CHECK_NO_BREAK(tctx);
>>         }
>>
>> But not breaking H1,LEASE1 to none on a write
>> on H2,LEASE2 seems like it could lead to data
>> corruption on Windows to me.
>>
>> My understanding is that H1,LEASE1 should be in a RH state
>> if the reply to H2,LEASE2 came back with RH.
> 
> Hmmm. Maybe not. With the following additional
> change on top I can get us to behave like W2K8.
> And with this we pass smb2.lease and smb2.oplock
> as well.
> 
> It forces a break to SMB2_LEASE_NONE on lease
> break timeout, and removes the (untested)
> optimization that prevents us from breaking
> to none if we write on our own lease where we're
> the only read lease holder.
> 
> diff --git a/source3/smbd/oplock.c b/source3/smbd/oplock.c
> index 8a9e021..efecc98 100644
> --- a/source3/smbd/oplock.c
> +++ b/source3/smbd/oplock.c
> @@ -528,7 +528,8 @@ static void lease_timeout_handler(struct tevent_context *ctx,
>         (void)downgrade_lease(lts->fsp->conn->sconn->client->connections,
>                         lts->fsp->file_id,
>                         &lts->fsp->lease->lease,
> -                       lts->break_to);
> +                       SMB2_LEASE_NONE);
> +       //              lts->break_to);
>         /*
>          * Remove the timed event handler.
>          * Must do this last as lts is hung off it.
> @@ -948,10 +949,12 @@ static void contend_level2_oplocks_begin_default(files_struct *fsp,
>                 DEBUG(10, ("No read oplocks around\n"));
>                 return;
>         }
> +#if 0
>         if ((num_read_oplocks == 1) && (fsp->oplock_type == LEASE_OPLOCK)) {
>                 DEBUG(10, ("We're the only reader, don't break\n"));
>                 return;
>         }
> +#endif

This is just for performance, we'll still skip breaking
our own lease within do_break_to_none().

I think we need to use smb2_lease_equal() there instead of doing:

                if ((l->lease_key.data[0] == state->lease_key.data[0]) &&
                    (l->lease_key.data[1] == state->lease_key.data[1])) {
                        DEBUG(10, ("Don't break our own lease\n"));
                        continue;
                }

>         /*
>          * When we get here we might have a brlock entry locked. Also
> 
> Updated lp3-2 attached. If we decide to keep this we
> can remove the struct lease_timeout_state I introduced
> for the lease_timeout_handler(), as we only need to
> pass the fsp, not a struct.

Why do we need the fsp at all, can't we hang the timeout on fsp->lease?

I'm also wondering, if you should remember the exiration time in
the 'breaking' field instead of just a bool.

If the process trying to send the lease break to the client gets
disconnected,
we'll never downgrade when it times out and the lease is still valid
over another connection.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20141118/3d677a5d/attachment.pgp>