broken mandatory locking behavior

broken mandatory locking behavior

[Pavel Shilovsky]

Hi all!

Now I have been working on lock cache capability for cifs module and
simplification of lock code. So, I faced with a such an interesting
place - mandatory style unlock code.

The problem is that mandatory semantic doesn't allow to specify a
range and remove all locks from there. But cifs module allows it. The
reason why it is so can be that cifs module tries to follow posix
semantic for mandatory locks. I think it's not what we want at all.

We have two lock mode for cifs module: posix and mandatory. And as for
me, we should follow posix semantic for posix locks and mandatory for
mandatory locks. If we mix these two semantic for mandatory lock mode,
we reach nothing: no posix (because mandatory locks doesn't allow to
perform io ops on the locked region by other processes) and no
mandatory (because we can setup unlock from 0 to infinity and remove
all locks at once).

The another problem is that this 'broken' code lives on kernel for a
long time. So, I suggest to discuss it now - while I am here I can
include this fix into my patchset as well.

Thoughts?

-- 
Best regards,
Pavel Shilovsky.

Re: broken mandatory locking behavior

[Jeff Layton]

On Sun, 25 Sep 2011 15:39:34 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Hi all!
> 
> Now I have been working on lock cache capability for cifs module and
> simplification of lock code. So, I faced with a such an interesting
> place - mandatory style unlock code.
> 
> The problem is that mandatory semantic doesn't allow to specify a
> range and remove all locks from there. But cifs module allows it. The
> reason why it is so can be that cifs module tries to follow posix
> semantic for mandatory locks. I think it's not what we want at all.
> 
> We have two lock mode for cifs module: posix and mandatory. And as for
> me, we should follow posix semantic for posix locks and mandatory for
> mandatory locks. If we mix these two semantic for mandatory lock mode,
> we reach nothing: no posix (because mandatory locks doesn't allow to
> perform io ops on the locked region by other processes) and no
> mandatory (because we can setup unlock from 0 to infinity and remove
> all locks at once).
> 

I'm not sure I understand what you're saying here. IIUC, windows does
not allow the merging of lock ranges. 

> The another problem is that this 'broken' code lives on kernel for a
> long time. So, I suggest to discuss it now - while I am here I can
> include this fix into my patchset as well.
> 

While I'd agree that the situation with locking is far from ideal, I'm
not sure that the above really adequately describes the situation...

With cifs.ko, our goal is generally do attempt to map POSIX behavior on
top of what the CIFS/SMB protocol allows. Locking is one of those
unfortunate places where this mapping really falls short.

Windows locks are always mandatory -- full stop. POSIX locks are
usually advisory (it is possible to do mandatory locking too, but
applications rarely do). POSIX allows the merging of lock ranges.
Windows does not. There are other differences too...

Now, most of our users don't care about the intricacies of what the
protocol allows. They just want their applications to work. So, we
attempt to accomodate that by mapping POSIX advisory locking on top of
windows locks. It's imperfect, but we do the best we can.

Your email above is rather vague -- I get that you want to change
*something*, but I'm not sure specifically what it is. Perhaps you can
clarify? What changes will users see with your proposal?

We haven't discussed locking on the list in a while -- the last time
was here:

http://lists.samba.org/archive/linux-cifs-client/2007-July/002088.html
http://lists.samba.org/archive/linux-cifs-client/2007-August/002167.html

I'd make sure what you're proposing jives with the limitations that
Jeremy outlined. 
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: broken mandatory locking behavior

[Pavel Shilovsky]

2011/10/1 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> I'm not sure I understand what you're saying here. IIUC, windows does
> not allow the merging of lock ranges.

Yes, in CIFS we allow to unlock all locks that appears in unlock
region for mandatory locks (by unlocking each one in an order), but
Windows doesn't.

>
> While I'd agree that the situation with locking is far from ideal, I'm
> not sure that the above really adequately describes the situation...
>
> With cifs.ko, our goal is generally do attempt to map POSIX behavior on
> top of what the CIFS/SMB protocol allows. Locking is one of those
> unfortunate places where this mapping really falls short.
>
> Windows locks are always mandatory -- full stop. POSIX locks are
> usually advisory (it is possible to do mandatory locking too, but
> applications rarely do). POSIX allows the merging of lock ranges.
> Windows does not. There are other differences too...
>
> Now, most of our users don't care about the intricacies of what the
> protocol allows. They just want their applications to work. So, we
> attempt to accomodate that by mapping POSIX advisory locking on top of
> windows locks. It's imperfect, but we do the best we can.
>
> Your email above is rather vague -- I get that you want to change
> *something*, but I'm not sure specifically what it is. Perhaps you can
> clarify? What changes will users see with your proposal?

Ok, sorry for wasn't clear. I suggest to follow Windows semantic
fluently for Windows locks - to unlock exactly the lock (one lock)
that is requested and no others. So, the end users will need to unlock
every lock they set before. On close, Windows remove locks itself -
so, we don't need to unlock them explicitly.

As we can't follow POSIX semantic Windows locks fluently, I don't
think we need to support it partly. So, as the results, we end up with
two opposite mode for brlocks: Windows and POSIX. If someone needs
such sort mixing as we have now, we can add extra mount option for
this further (or vice versa - a mount option for the new mode).

Thoughts?

-- 
Best regards,
Pavel Shilovsky.

Re: broken mandatory locking behavior

[Jeff Layton]

On Tue, 4 Oct 2011 13:42:24 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2011/10/1 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > I'm not sure I understand what you're saying here. IIUC, windows does
> > not allow the merging of lock ranges.
> 
> Yes, in CIFS we allow to unlock all locks that appears in unlock
> region for mandatory locks (by unlocking each one in an order), but
> Windows doesn't.
> 
> >
> > While I'd agree that the situation with locking is far from ideal, I'm
> > not sure that the above really adequately describes the situation...
> >
> > With cifs.ko, our goal is generally do attempt to map POSIX behavior on
> > top of what the CIFS/SMB protocol allows. Locking is one of those
> > unfortunate places where this mapping really falls short.
> >
> > Windows locks are always mandatory -- full stop. POSIX locks are
> > usually advisory (it is possible to do mandatory locking too, but
> > applications rarely do). POSIX allows the merging of lock ranges.
> > Windows does not. There are other differences too...
> >
> > Now, most of our users don't care about the intricacies of what the
> > protocol allows. They just want their applications to work. So, we
> > attempt to accomodate that by mapping POSIX advisory locking on top of
> > windows locks. It's imperfect, but we do the best we can.
> >
> > Your email above is rather vague -- I get that you want to change
> > *something*, but I'm not sure specifically what it is. Perhaps you can
> > clarify? What changes will users see with your proposal?
> 
> Ok, sorry for wasn't clear. I suggest to follow Windows semantic
> fluently for Windows locks - to unlock exactly the lock (one lock)
> that is requested and no others. So, the end users will need to unlock
> every lock they set before. On close, Windows remove locks itself -
> so, we don't need to unlock them explicitly.
> 

The VFS calls down to the filesystem to unlock all of the locks prior
to ->release. How do you propose to tell the difference?

All cifs.ko will see is an unlock from the VFS -- it has no way to know
whether it's a "normal" unlock or an unlock that's occurring due to a
close() syscall (and therefore can be skipped).

IOW, doing what you propose here will require VFS-level changes.

> As we can't follow POSIX semantic Windows locks fluently, I don't
> think we need to support it partly. So, as the results, we end up with
> two opposite mode for brlocks: Windows and POSIX. If someone needs
> such sort mixing as we have now, we can add extra mount option for
> this further (or vice versa - a mount option for the new mode).
> 
> Thoughts?
> 

I think this is tantamount to insisting that applications be specially
written for cifs.ko if they want to do locking.

The larger goal for cifs.ko (as I see it) is to allow applications to
mostly run unchanged on top of windows servers. Naturally, this is
impossible since windows doesn't follow POSIX semantics, so the best we
can do is closely approximate it.

There's certainly room for improvement here, but what you're proposing
sounds like a step backward from that goal. It will break a lot of
existing applications that run successfully on top of cifs.ko today.

I'm also not sure I understand the benefit here either. What do you
think that such a change would gain us?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: broken mandatory locking behavior

[J. Bruce Fields]

On Tue, Oct 04, 2011 at 06:54:51AM -0400, Jeff Layton wrote:
> I think this is tantamount to insisting that applications be specially
> written for cifs.ko if they want to do locking.
> 
> The larger goal for cifs.ko (as I see it) is to allow applications to
> mostly run unchanged on top of windows servers. Naturally, this is
> impossible since windows doesn't follow POSIX semantics, so the best we
> can do is closely approximate it.
> 
> There's certainly room for improvement here, but what you're proposing
> sounds like a step backward from that goal. It will break a lot of
> existing applications that run successfully on top of cifs.ko today.

Right.  You can tell which kind of locking the application wants by the
interface it's using.  If the application is using fcntl F_GETLK,
F_SETLK, F_SETLKW, then chances are it was written assuming posix
semantics, so all we can do is emulate those as best as possible.

If there are applications that need want the Windows semantics, then
we'd need to provide a new lock interface which promises them exactly
that.

--b.

Re: broken mandatory locking behavior

[Pavel Shilovsky]

2011/10/4 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 4 Oct 2011 13:42:24 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2011/10/1 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
>> > I'm not sure I understand what you're saying here. IIUC, windows does
>> > not allow the merging of lock ranges.
>>
>> Yes, in CIFS we allow to unlock all locks that appears in unlock
>> region for mandatory locks (by unlocking each one in an order), but
>> Windows doesn't.
>>
>> >
>> > While I'd agree that the situation with locking is far from ideal, I'm
>> > not sure that the above really adequately describes the situation...
>> >
>> > With cifs.ko, our goal is generally do attempt to map POSIX behavior on
>> > top of what the CIFS/SMB protocol allows. Locking is one of those
>> > unfortunate places where this mapping really falls short.
>> >
>> > Windows locks are always mandatory -- full stop. POSIX locks are
>> > usually advisory (it is possible to do mandatory locking too, but
>> > applications rarely do). POSIX allows the merging of lock ranges.
>> > Windows does not. There are other differences too...
>> >
>> > Now, most of our users don't care about the intricacies of what the
>> > protocol allows. They just want their applications to work. So, we
>> > attempt to accomodate that by mapping POSIX advisory locking on top of
>> > windows locks. It's imperfect, but we do the best we can.
>> >
>> > Your email above is rather vague -- I get that you want to change
>> > *something*, but I'm not sure specifically what it is. Perhaps you can
>> > clarify? What changes will users see with your proposal?
>>
>> Ok, sorry for wasn't clear. I suggest to follow Windows semantic
>> fluently for Windows locks - to unlock exactly the lock (one lock)
>> that is requested and no others. So, the end users will need to unlock
>> every lock they set before. On close, Windows remove locks itself -
>> so, we don't need to unlock them explicitly.
>>
>
> The VFS calls down to the filesystem to unlock all of the locks prior
> to ->release. How do you propose to tell the difference?
>
> All cifs.ko will see is an unlock from the VFS -- it has no way to know
> whether it's a "normal" unlock or an unlock that's occurring due to a
> close() syscall (and therefore can be skipped).
>
> IOW, doing what you propose here will require VFS-level changes.

This correct. But this is not what I meant. I don't think we need to
call posix_file_lock_wait for mandatory locks at all. In this way, VFS
doesn't know anything about brlocks on a file - so, only cifs.ko
processes them. In this way, on close we simply doesn't do anything
with brlocks we have - when we close a file, server removes brlocks
itself.

>
>> As we can't follow POSIX semantic Windows locks fluently, I don't
>> think we need to support it partly. So, as the results, we end up with
>> two opposite mode for brlocks: Windows and POSIX. If someone needs
>> such sort mixing as we have now, we can add extra mount option for
>> this further (or vice versa - a mount option for the new mode).
>>
>> Thoughts?
>>
>
> I think this is tantamount to insisting that applications be specially
> written for cifs.ko if they want to do locking.
>
> The larger goal for cifs.ko (as I see it) is to allow applications to
> mostly run unchanged on top of windows servers. Naturally, this is
> impossible since windows doesn't follow POSIX semantics, so the best we
> can do is closely approximate it.
>
> There's certainly room for improvement here, but what you're proposing
> sounds like a step backward from that goal. It will break a lot of
> existing applications that run successfully on top of cifs.ko today.
>
> I'm also not sure I understand the benefit here either. What do you
> think that such a change would gain us?
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>

WINE applications that runs over cifs shares need such a change.

-- 
Best regards,
Pavel Shilovsky.

Re: broken mandatory locking behavior

[Pavel Shilovsky]

2011/10/4 J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>:
> On Tue, Oct 04, 2011 at 06:54:51AM -0400, Jeff Layton wrote:
>> I think this is tantamount to insisting that applications be specially
>> written for cifs.ko if they want to do locking.
>>
>> The larger goal for cifs.ko (as I see it) is to allow applications to
>> mostly run unchanged on top of windows servers. Naturally, this is
>> impossible since windows doesn't follow POSIX semantics, so the best we
>> can do is closely approximate it.
>>
>> There's certainly room for improvement here, but what you're proposing
>> sounds like a step backward from that goal. It will break a lot of
>> existing applications that run successfully on top of cifs.ko today.
>
> Right.  You can tell which kind of locking the application wants by the
> interface it's using.  If the application is using fcntl F_GETLK,
> F_SETLK, F_SETLKW, then chances are it was written assuming posix
> semantics, so all we can do is emulate those as best as possible.
>
> If there are applications that need want the Windows semantics, then
> we'd need to provide a new lock interface which promises them exactly
> that.
>
> --b.
>

Ok, I understand your and Jeff's idea - it's better to support POSIX
at least partly than doesn't support it at all.

I don't think that we need VFS changes for the new mode, because it is
cifs feature only. All that we need is to support this for cifs - so,
it can be a new mount option or the existing one - now we have
'forcemand' mount option (that switch on mandatory locking even if the
server support POSIX one) that can includes such a change. What do you
think about it?

-- 
Best regards,
Pavel Shilovsky.

Re: broken mandatory locking behavior

[Jeff Layton]

On Wed, 5 Oct 2011 13:24:59 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2011/10/4 J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>:
> > On Tue, Oct 04, 2011 at 06:54:51AM -0400, Jeff Layton wrote:
> >> I think this is tantamount to insisting that applications be specially
> >> written for cifs.ko if they want to do locking.
> >>
> >> The larger goal for cifs.ko (as I see it) is to allow applications to
> >> mostly run unchanged on top of windows servers. Naturally, this is
> >> impossible since windows doesn't follow POSIX semantics, so the best we
> >> can do is closely approximate it.
> >>
> >> There's certainly room for improvement here, but what you're proposing
> >> sounds like a step backward from that goal. It will break a lot of
> >> existing applications that run successfully on top of cifs.ko today.
> >
> > Right.  You can tell which kind of locking the application wants by the
> > interface it's using.  If the application is using fcntl F_GETLK,
> > F_SETLK, F_SETLKW, then chances are it was written assuming posix
> > semantics, so all we can do is emulate those as best as possible.
> >
> > If there are applications that need want the Windows semantics, then
> > we'd need to provide a new lock interface which promises them exactly
> > that.
> >
> > --b.
> >
> 
> Ok, I understand your and Jeff's idea - it's better to support POSIX
> at least partly than doesn't support it at all.
> 
> I don't think that we need VFS changes for the new mode, because it is
> cifs feature only. All that we need is to support this for cifs - so,
> it can be a new mount option or the existing one - now we have
> 'forcemand' mount option (that switch on mandatory locking even if the
> server support POSIX one) that can includes such a change. What do you
> think about it?
> 

I'm not a fan of adding more mount-option enabled hacks to cifs to
support wine. If wine or other applications need windows semantics,
then I would prefer to see you add proper vfs-level interfaces to
provide them.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: broken mandatory locking behavior

[J. Bruce Fields]

On Wed, Oct 05, 2011 at 06:09:36AM -0400, Jeff Layton wrote:
> On Wed, 5 Oct 2011 13:24:59 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > 2011/10/4 J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>:
> > > On Tue, Oct 04, 2011 at 06:54:51AM -0400, Jeff Layton wrote:
> > >> I think this is tantamount to insisting that applications be specially
> > >> written for cifs.ko if they want to do locking.
> > >>
> > >> The larger goal for cifs.ko (as I see it) is to allow applications to
> > >> mostly run unchanged on top of windows servers. Naturally, this is
> > >> impossible since windows doesn't follow POSIX semantics, so the best we
> > >> can do is closely approximate it.
> > >>
> > >> There's certainly room for improvement here, but what you're proposing
> > >> sounds like a step backward from that goal. It will break a lot of
> > >> existing applications that run successfully on top of cifs.ko today.
> > >
> > > Right.  You can tell which kind of locking the application wants by the
> > > interface it's using.  If the application is using fcntl F_GETLK,
> > > F_SETLK, F_SETLKW, then chances are it was written assuming posix
> > > semantics, so all we can do is emulate those as best as possible.
> > >
> > > If there are applications that need want the Windows semantics, then
> > > we'd need to provide a new lock interface which promises them exactly
> > > that.
> > >
> > > --b.
> > >
> > 
> > Ok, I understand your and Jeff's idea - it's better to support POSIX
> > at least partly than doesn't support it at all.
> > 
> > I don't think that we need VFS changes for the new mode, because it is
> > cifs feature only. All that we need is to support this for cifs - so,
> > it can be a new mount option or the existing one - now we have
> > 'forcemand' mount option (that switch on mandatory locking even if the
> > server support POSIX one) that can includes such a change. What do you
> > think about it?
> > 
> 
> I'm not a fan of adding more mount-option enabled hacks to cifs to
> support wine. If wine or other applications need windows semantics,
> then I would prefer to see you add proper vfs-level interfaces to
> provide them.

Agreed.  A mount option that changes semantics in subtle ways will cause
applications to fail in sutble and unpredictable ways when it's set
wrong.  And doesn't have any hope of handling the case of a mixture of
POSIX and Windows applications on the same filesystem.

Whereas a new interface, when it's unavailable, will cause a clean
predictable failure right at the start, allowing the application to give
a helpful error message, or fall back on different behavior if it
chooses to.

Also, before building anything more on top of Linux mandatory locking,
somebody should really fix the existing bugs--the current implementation
has races.

--b.

Re: broken mandatory locking behavior

[Pavel Shilovsky]

2011/10/5 J. Bruce Fields <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>:
> On Wed, Oct 05, 2011 at 06:09:36AM -0400, Jeff Layton wrote:
>> I'm not a fan of adding more mount-option enabled hacks to cifs to
>> support wine. If wine or other applications need windows semantics,
>> then I would prefer to see you add proper vfs-level interfaces to
>> provide them.
>
> Agreed.  A mount option that changes semantics in subtle ways will cause
> applications to fail in sutble and unpredictable ways when it's set
> wrong.  And doesn't have any hope of handling the case of a mixture of
> POSIX and Windows applications on the same filesystem.
>
> Whereas a new interface, when it's unavailable, will cause a clean
> predictable failure right at the start, allowing the application to give
> a helpful error message, or fall back on different behavior if it
> chooses to.
>
> Also, before building anything more on top of Linux mandatory locking,
> somebody should really fix the existing bugs--the current implementation
> has races.
>
> --b.
>

Ok, I will think about how to do this kind of things better.

So, I will send my locking patchset without unlock behavior change.

-- 
Best regards,
Pavel Shilovsky.