Patchset: Remove FAKE_LEVEL_II_OPLOCK type

[Stefan (metze) Metzmacher]

Hi Volker,

this looks really great, thanks! This looks like a big step in the direction
of supporting leases:-)

> Without clustering, fsp->brlock_rec will never be set anyway. In the
> clustering case we can't use the seqnum trick, so this is slow enough
> that the additional if-statement does not matter in this case anyway. In
> the non-clustered case it might. Have not measured it, but every little
> bit helps I guess.

What is the exact reason why the seqnum trick doesn't work in cluster mode.
I see that there might be cases where we would reload to often
because the seqnum on the local tdb also changes if a record get migrated
or a recovery happened. But wouldn't the exact same seqnum doesn't mean
we could also change in cluster mode?

> Subject: [PATCH 09/13] smbd: Put "have_level2_oplocks" into brlock.tdb

If possible I'd prefer the name 'read_oplocks' instead of 'level2_oplocks'
for the new functions variable, so that we don't have to rename them
when we get leases.

> The format for this change is to add one byte to the end of the brlock.tdb
> record with value 1 if we have level2 oplocks around. Thus this patch
> effectively reverts 8f41142 which I discovered while writing this
> change. We now legally have unaligned records.
>
> We can certainly talk about the format, but I'm not yet convinced we
> need an idl for this yet. This is a potentially very hot code path,
> and ndr marshalling has a cost.

What about adding a uint32_t and use it as flags field, we may want to
store more information
in future.

> @@ -630,6 +662,7 @@ static void contend_level2_oplocks_begin_default(files_struct *fsp,
>  	struct smbd_server_connection *sconn = fsp->conn->sconn;
>  	struct tevent_immediate *im;
>  	struct break_to_none_state *state;
> +	struct byte_range_lock *brl;
>  
>  	/*
>  	 * If this file is level II oplocked then we need
> @@ -639,8 +672,14 @@ static void contend_level2_oplocks_begin_default(files_struct *fsp,
>  	 * the shared memory area whilst doing this.
>  	 */
>  
> -	if (!LEVEL_II_OPLOCK_TYPE(fsp->oplock_type))

I'd keep the optimization by using

if (EXCLUSIVE_OPLOCK_TYPE(fsp->ofsp->oplock_type)) {
     return;
}

> +	brl = brl_get_locks_readonly(fsp);
> +	if (brl == NULL) {
> +		/* Can't tell if we have to downgrade level2 oplocks */

If we can't figure this out shouldn't we better downgrade the oplock?

>  		return;
> +	}
> +	if (!brl_have_level2_oplocks(brl)) {
> +		return;
> +	}

I'd use

if ((brl ! NULL) && !brl_have_level2_oplocks(brl)) {
        return;
}

As optimization we could also think about relying on
brl_get_locks_readonly() being called by strict_lock_default()
so that we don't need it twice...

metze

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