Samba oplock level II problem

[Pavel Shilovsky]

2011/2/4 Jeremy Allison <jra at samba.org>:
> On Tue, Feb 01, 2011 at 03:10:16PM +0300, Pavel Shilovsky wrote:
>> 2011/2/1 Jeremy Allison <jra at samba.org>:
>> > On Tue, Feb 01, 2011 at 12:21:29AM +0300, Pavel Shilovsky wrote:
>> >> >
>> >>
>> >> Please, look at the second variant of the patch - it has extra debug
>> >> message if initial_break_processing returns NULL:
>> >> http://git.etersoft.ru/people/piastry/packages/?p=samba4.git;a=commitdiff;h=569ee7e3553b4e2fb0b098cca9d1a32ddc7afee6
>> >
>> > Got it - pushed (a similar patch) thanks !
>> >
>>
>> Today, I noticed that now we can have several fid with different
>> oplock types in the same time. That's why we need to process
>> conted_level2_oplocks_begin_default not only for level II current
>> oplock type but for none oplock too (because another clients can have
>> level II oplock to be broken in this call).
>>
>> So, I created the patch to fix it:
>> http://git.etersoft.ru/people/piastry/packages/?p=samba4.git;a=commitdiff;h=0f1a9d80fc28b2e9f8ace21819bf9af4d9da0580
>
> I don't think this patch is needed.
>
> If a client asks for NO_OPLOCK but the file already has
> a level2 oplock granted we silently and internally grant
> an oplock type of FAKE_LEVEL_II_OPLOCK.
>
> So all entries in the open file table can be in one of
> 3 states:
>
> 1). Exclusive or batch oplock - only one allowed.
> 2). Level2 or fake level2 oplocks - multiple allowed.
> 3). no oplock - multiple allowed.
>
> Note the only way we can have a combination of
> Level2 or fake level2 and no-oplocks is in the
> case where we're in transition from state (2) to
> state (3) (and we haven't finished yet).
>
> In that case someone (an active smbd) has already
> called contend_level2_oplocks_begin_default() and
> we're in the process of transitioning from state (2)
> to state (3) - and we don't need to send out another
> (duplicate) set of messages.
>
> In any other case we're either in state (1) where
> we dont' need to send messages or in state (3) where
> we also don't need to send messages.
>
> The FAKE_LEVEL_II_OPLOCK which is granted when a
> client asks for NO_OPLOCK saves us from needing
> to make this change:
>
> -       if (!LEVEL_II_OPLOCK_TYPE(fsp->oplock_type))
> +       if (EXCLUSIVE_OPLOCK_TYPE(fsp->oplock_type) ||
> +           BATCH_OPLOCK_TYPE(fsp->oplock_type))
>                return;
>
> - the single check for LEVEL_II_OPLOCK_TYPE (which
> covers both LEVEL_II_OPLOCK and FAKE_LEVEL_II_OPLOCK
> types is correct here I think.
>
> If you disagree please explain your reasoning further.
>
> Jeremy.
>

I think you missed new byte-range locking part. Now we may have the
following situation:
1) f1 = open(file) # exclusive oplock is granted to f1
2) lock(f1) # set byte-range lock via f1
3) f2 = open(file) # f1 gets oplock break with level2 but no oplock is
granted to f2 (because lock count more 0) - we have 'no oplock' for f2
and 'level2' oplock for f1 in share mode entries
4) lock(f2) # set byte-range lock via f2 - call
contend_level2_oplocks_begin_default but don't proccess it because
current code doesn't do anything for no-oplock fids in this function;
but we still have level2 oplock for f1 that needs to be broken after
f2 set a lock!

So, in general, byte-range locks bring complexity and now we may don't
have the same level between all entries of the file (Windows 7 server
grants the same oplock levels for the situation described above).
That's why I think such a patch is needed.

-- 
Best regards,
Pavel Shilovsky.