Pavel's Lock cleanup

Pavel's Lock cleanup

[Steve French]

I like Pavel's lock cleanup (1st patch of series) so far, am
continuing to review it, but I also noticed a few things that we need
to do in the future. This line of code:

	if (flock->fl_flags & FL_LEASE)
		cFYI(1, "Lease on file - not implemented yet");

reminded me that we should check if we need to implement this (in cifs
we can return yes if we have a lease already (oplock), but in smb2.1
and later we can request a lease or an upgrade to an existing one, on
the fly).

-- 
Thanks,

Steve

Re: Pavel's Lock cleanup

[J. Bruce Fields]

On Thu, Oct 13, 2011 at 05:23:52PM -0500, Steve French wrote:
> I like Pavel's lock cleanup (1st patch of series) so far, am
> continuing to review it, but I also noticed a few things that we need
> to do in the future. This line of code:
> 
> 	if (flock->fl_flags & FL_LEASE)
> 		cFYI(1, "Lease on file - not implemented yet");
> 
> reminded me that we should check if we need to implement this (in cifs
> we can return yes if we have a lease already (oplock), but in smb2.1
> and later we can request a lease or an upgrade to an existing one, on
> the fly).

And you'll also need to make sure you call break_lease() when you find
out it's broken.

A description of the smb (1/2/2.1) oplock and lease semantics would be
useful--I'm curious whether they're really a good fit for the linux
vfs's lease semantics.

--b.

Re: Pavel's Lock cleanup

[Steve French]

On Fri, Oct 14, 2011 at 4:12 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Thu, Oct 13, 2011 at 05:23:52PM -0500, Steve French wrote:
>> I like Pavel's lock cleanup (1st patch of series) so far, am
>> continuing to review it, but I also noticed a few things that we need
>> to do in the future. This line of code:
>>
>>       if (flock->fl_flags & FL_LEASE)
>>               cFYI(1, "Lease on file - not implemented yet");
>>
>> reminded me that we should check if we need to implement this (in cifs
>> we can return yes if we have a lease already (oplock), but in smb2.1
>> and later we can request a lease or an upgrade to an existing one, on
>> the fly).
>
> And you'll also need to make sure you call break_lease() when you find
> out it's broken.
>
> A description of the smb (1/2/2.1) oplock and lease semantics would be
> useful--I'm curious whether they're really a good fit for the linux
> vfs's lease semantics.

I can probably dig up the exact references in MS-CIFS.pdf and MS-SMB2.pdf
but basically SMB2 (original dialect) and CIFS have similar semantics (and
Samba's need to implement CIFS oplocks drove the original implementation
of leases in the kernel, although it is requested at open time via a flag,
not after open as a handle based call).  The problem comes in with smb2.1
which adds the ability to reaquire leases (which may be ok) and upgrade
leases (from read only to readwrite) and to reuse an existing lease
(so the 2nd open from the same client doesn't have to break oplock)
which helps Linux which has a common cache manager.

smb2.2 adds the ability to do directory leases (to cache directory
entries)


-- 
Thanks,

Steve

Re: Pavel's Lock cleanup

[J. Bruce Fields]

On Fri, Oct 14, 2011 at 05:38:59PM -0500, Steve French wrote:
> On Fri, Oct 14, 2011 at 4:12 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Thu, Oct 13, 2011 at 05:23:52PM -0500, Steve French wrote:
> >> I like Pavel's lock cleanup (1st patch of series) so far, am
> >> continuing to review it, but I also noticed a few things that we need
> >> to do in the future. This line of code:
> >>
> >>       if (flock->fl_flags & FL_LEASE)
> >>               cFYI(1, "Lease on file - not implemented yet");
> >>
> >> reminded me that we should check if we need to implement this (in cifs
> >> we can return yes if we have a lease already (oplock), but in smb2.1
> >> and later we can request a lease or an upgrade to an existing one, on
> >> the fly).
> >
> > And you'll also need to make sure you call break_lease() when you find
> > out it's broken.
> >
> > A description of the smb (1/2/2.1) oplock and lease semantics would be
> > useful--I'm curious whether they're really a good fit for the linux
> > vfs's lease semantics.
> 
> I can probably dig up the exact references in MS-CIFS.pdf and MS-SMB2.pdf
> but basically SMB2 (original dialect) and CIFS have similar semantics (and
> Samba's need to implement CIFS oplocks drove the original implementation
> of leases in the kernel, although it is requested at open time via a flag,
> not after open as a handle based call).  The problem comes in with smb2.1
> which adds the ability to reaquire leases (which may be ok) and upgrade
> leases (from read only to readwrite) and to reuse an existing lease
> (so the 2nd open from the same client doesn't have to break oplock)
> which helps Linux which has a common cache manager.
> 
> smb2.2 adds the ability to do directory leases (to cache directory
> entries)

I know all that, but what I'd like to know would be:
	- What's the difference between a lease and an oplock?
	- What does each give you the right to cache?  For example:
		- The Samba folks tell me, not filename, only data:
			http://marc.info/?l=samba-technical&m=131688944117833&w=2
		- Based on these patches, I'm assuming they also give
		  the right to cache locks.
	  But it would be nice to have references (either
	  specifications, or test results) to support those statements.

Microsoft's documents don't spell this out, as far as I can tell.

--b.

Re: Pavel's Lock cleanup

[Steve French]

On Fri, Oct 14, 2011 at 6:29 PM, J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> wrote:
> On Fri, Oct 14, 2011 at 05:38:59PM -0500, Steve French wrote:
>> On Fri, Oct 14, 2011 at 4:12 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
>> > On Thu, Oct 13, 2011 at 05:23:52PM -0500, Steve French wrote:
>> >> I like Pavel's lock cleanup (1st patch of series) so far, am
>> >> continuing to review it, but I also noticed a few things that we need
>> >> to do in the future. This line of code:
>> >>
>> >>       if (flock->fl_flags & FL_LEASE)
>> >>               cFYI(1, "Lease on file - not implemented yet");
>> >>
>> >> reminded me that we should check if we need to implement this (in cifs
>> >> we can return yes if we have a lease already (oplock), but in smb2.1
>> >> and later we can request a lease or an upgrade to an existing one, on
>> >> the fly).
>> >
>> > And you'll also need to make sure you call break_lease() when you find
>> > out it's broken.
>> >
>> > A description of the smb (1/2/2.1) oplock and lease semantics would be
>> > useful--I'm curious whether they're really a good fit for the linux
>> > vfs's lease semantics.
>>
>> I can probably dig up the exact references in MS-CIFS.pdf and MS-SMB2.pdf
>> but basically SMB2 (original dialect) and CIFS have similar semantics (and
>> Samba's need to implement CIFS oplocks drove the original implementation
>> of leases in the kernel, although it is requested at open time via a flag,
>> not after open as a handle based call).  The problem comes in with smb2.1
>> which adds the ability to reaquire leases (which may be ok) and upgrade
>> leases (from read only to readwrite) and to reuse an existing lease
>> (so the 2nd open from the same client doesn't have to break oplock)
>> which helps Linux which has a common cache manager.
>>
>> smb2.2 adds the ability to do directory leases (to cache directory
>> entries)
>
> I know all that, but what I'd like to know would be:
>        - What's the difference between a lease and an oplock?
>        - What does each give you the right to cache?  For example:
>                - The Samba folks tell me, not filename, only data:
>                        http://marc.info/?l=samba-technical&m=131688944117833&w=2
>                - Based on these patches, I'm assuming they also give
>                  the right to cache locks.
>          But it would be nice to have references (either
>          specifications, or test results) to support those statements.

Interesting questions.  The MS-FSA document (in section 3.1.4.12)
seems to agree with that
(that querying attributes, and some metadata, timestamps, does not
break oplock):

(see http://msdn.microsoft.com/en-us/library/860b1516-c452-47b4-bdbc-625d344e2041.aspx)

"If OpParams.DesiredAccess contains no flags other than FILE_READ_ATTRIBUTES,
FILE_WRITE_ATTRIBUTES, or SYNCHRONIZE, the algorithm returns at this point."
(ie doesn't break oplock if these are the only flags set).


On oplock vs. leases (in MS usage, oplock is the older term).  An oplock was -
1) requested on open (presumably what the "op" part of the name meant)
2) had only a few types:  "batch" (could cache data, and cache open/close)
and "read write" (could cache read or writes of data) and later a
"read only" type was
added (allowing you to cache writes)

Leases (a later term in Microsoft documents) had more function (they
included for
example a "key" which identifies the owner of the lease).

-- 
Thanks,

Steve

Re: Pavel's Lock cleanup

[Steve French]

On Fri, Oct 14, 2011 at 7:02 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On oplock vs. leases (in MS usage, oplock is the older term).  An oplock was -
> 1) requested on open (presumably what the "op" part of the name meant)
> 2) had only a few types:  "batch" (could cache data, and cache open/close)
> and "read write" (could cache read or writes of data) and later a
> "read only" type was
> added (allowing you to cache writes)

oops - I meant "allowing you to cache reads" (obviously)


-- 
Thanks,

Steve

Re: Pavel's Lock cleanup

[Pavel Shilovsky]

2011/10/15 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> On Fri, Oct 14, 2011 at 7:02 PM, Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On oplock vs. leases (in MS usage, oplock is the older term).  An oplock was -
>> 1) requested on open (presumably what the "op" part of the name meant)
>> 2) had only a few types:  "batch" (could cache data, and cache open/close)
>> and "read write" (could cache read or writes of data) and later a
>> "read only" type was
>> added (allowing you to cache writes)
>
> oops - I meant "allowing you to cache reads" (obviously)
>

Also, note that batch and read write oplocks allow to cache byte-range
locks on the client.

-- 
Best regards,
Pavel Shilovsky.

Re: Pavel's Lock cleanup

[J. Bruce Fields]

On Fri, Oct 14, 2011 at 07:02:55PM -0500, Steve French wrote:
> On Fri, Oct 14, 2011 at 6:29 PM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > On Fri, Oct 14, 2011 at 05:38:59PM -0500, Steve French wrote:
> >> On Fri, Oct 14, 2011 at 4:12 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> >> > On Thu, Oct 13, 2011 at 05:23:52PM -0500, Steve French wrote:
> >> >> I like Pavel's lock cleanup (1st patch of series) so far, am
> >> >> continuing to review it, but I also noticed a few things that we need
> >> >> to do in the future. This line of code:
> >> >>
> >> >>       if (flock->fl_flags & FL_LEASE)
> >> >>               cFYI(1, "Lease on file - not implemented yet");
> >> >>
> >> >> reminded me that we should check if we need to implement this (in cifs
> >> >> we can return yes if we have a lease already (oplock), but in smb2.1
> >> >> and later we can request a lease or an upgrade to an existing one, on
> >> >> the fly).
> >> >
> >> > And you'll also need to make sure you call break_lease() when you find
> >> > out it's broken.
> >> >
> >> > A description of the smb (1/2/2.1) oplock and lease semantics would be
> >> > useful--I'm curious whether they're really a good fit for the linux
> >> > vfs's lease semantics.
> >>
> >> I can probably dig up the exact references in MS-CIFS.pdf and MS-SMB2.pdf
> >> but basically SMB2 (original dialect) and CIFS have similar semantics (and
> >> Samba's need to implement CIFS oplocks drove the original implementation
> >> of leases in the kernel, although it is requested at open time via a flag,
> >> not after open as a handle based call).  The problem comes in with smb2.1
> >> which adds the ability to reaquire leases (which may be ok) and upgrade
> >> leases (from read only to readwrite) and to reuse an existing lease
> >> (so the 2nd open from the same client doesn't have to break oplock)
> >> which helps Linux which has a common cache manager.
> >>
> >> smb2.2 adds the ability to do directory leases (to cache directory
> >> entries)
> >
> > I know all that, but what I'd like to know would be:
> >        - What's the difference between a lease and an oplock?
> >        - What does each give you the right to cache?  For example:
> >                - The Samba folks tell me, not filename, only data:
> >                        http://marc.info/?l=samba-technical&m=131688944117833&w=2
> >                - Based on these patches, I'm assuming they also give
> >                  the right to cache locks.
> >          But it would be nice to have references (either
> >          specifications, or test results) to support those statements.
> 
> Interesting questions.  The MS-FSA document (in section 3.1.4.12)

Woah, I totally missed that section on my previous reading.  Thanks!

> seems to agree with that
> (that querying attributes, and some metadata, timestamps, does not
> break oplock):
> 
> (see http://msdn.microsoft.com/en-us/library/860b1516-c452-47b4-bdbc-625d344e2041.aspx)
> 
> "If OpParams.DesiredAccess contains no flags other than FILE_READ_ATTRIBUTES,
> FILE_WRITE_ATTRIBUTES, or SYNCHRONIZE, the algorithm returns at this point."
> (ie doesn't break oplock if these are the only flags set).
> 
> 
> On oplock vs. leases (in MS usage, oplock is the older term).  An oplock was -
> 1) requested on open (presumably what the "op" part of the name meant)
> 2) had only a few types:  "batch" (could cache data, and cache open/close)
> and "read write" (could cache read or writes of data) and later a
> "read only" type was
> added (allowing you to cache writes)
> 
> Leases (a later term in Microsoft documents) had more function (they
> included for
> example a "key" which identifies the owner of the lease).

OK, thanks again, that's helpful.

--b.