fcntl: F_SETLEASE/F_RDLCK question

fcntl: F_SETLEASE/F_RDLCK question

[Heiko Carstens]

Hi,

the semantics of fnctl used together with F_SETLEASE and
argument F_RDLCK have been changed with bk changeset
1.1938.185.141 (sometime between 2.6.9 and 2.6.10).
Since then it's only possible to get a read lease when the
file in question does not have _any_ writers.
This is at least inconsistent with the man page of fcntl
and looks pretty much like this is a bug in the kernel.

Any comments?

Thanks,
Heiko

Re: fcntl: F_SETLEASE/F_RDLCK question

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

Hi Heiko,

On Mon, 2 May 2005 11:15:24 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
>
> the semantics of fnctl used together with F_SETLEASE and
> argument F_RDLCK have been changed with bk changeset
> 1.1938.185.141 (sometime between 2.6.9 and 2.6.10).
> Since then it's only possible to get a read lease when the
> file in question does not have _any_ writers.
> This is at least inconsistent with the man page of fcntl
> and looks pretty much like this is a bug in the kernel.
> 
> Any comments?

The previous behaviour was a bug that occurred because at the time the
original lease code was written, it was not possible to tell if there were
writers when the read lease was being taken. Further improvements in the
kernel have since made this possible.

The intention of a read lease is to let the holder know is anyone tries to
modify the file.

The current behaviour does not conflict with the man pages on Debian
(although the previous behaviour did not either :-))

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: fcntl: F_SETLEASE/F_RDLCK question

[Heiko Carstens]

> The intention of a read lease is to let the holder know is anyone tries 
to
> modify the file.
> 
> The current behaviour does not conflict with the man pages on Debian
> (although the previous behaviour did not either :-))

Now that I know what the intention of the read lease is, I agree that 
there
is no conflict :). Thanks for clarifying!

Heiko

Re: fcntl: F_SETLEASE/F_RDLCK question

[Michael Kerrisk]

Hi Stephen,

> On Mon, 2 May 2005 11:15:24 +0200 Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> >
> > the semantics of fnctl used together with F_SETLEASE and
> > argument F_RDLCK have been changed with bk changeset
> > 1.1938.185.141 (sometime between 2.6.9 and 2.6.10).
> > Since then it's only possible to get a read lease when the
> > file in question does not have _any_ writers.
> > This is at least inconsistent with the man page of fcntl
> > and looks pretty much like this is a bug in the kernel.
> > 
> > Any comments?
> 
> The previous behaviour was a bug that occurred because at the time the
> original lease code was written, it was not possible to tell if there 
> were
> writers when the read lease was being taken. Further improvements in the
> kernel have since made this possible.
> 
> The intention of a read lease is to let the holder know is anyone tries 
> to modify the file.
> 
> The current behaviour does not conflict with the man pages on Debian
> (although the previous behaviour did not either :-))

Indeed the problem referred to is fixed, but it looks like another 
one may have been introduced.

It now appears (I tested on 2.6.11.6) that if a process opens 
a file O_RDWR and then tries to place a read lease via that
file descriptor, then the F_SETLEASE fails with EAGAIN, 
even though no other process has the file open for writing.  
(On the other hand, if the process opens the file 
O_WRONLY, then it can place either a read or a write lease.  
This is how I think things always worked, but it seems 
inconsistent with the aforementioned behaviour.)  

Some further testing showed the following (both open() 
and fcntl(F_SETLEASE) from same process):

 open()  |  lease requested
  flag   | F_RDLCK  | F_WRLCK
---------+----------+----------
O_RDONLY | okay     |  okay
O_WRONLY | EAGAIN   |  okay
O_RDWR   | EAGAIN   |  okay

This seems strange (I imagine the caller should be excluded 
from the list of processes being checked to see if the file 
is opened for writing), and differs from earlier kernel
versions.  What is the intended behaviour here?

Cheers,

Michael

-- 
+++ Lassen Sie Ihren Gedanken freien Lauf... z.B. per FreeSMS +++
GMX bietet bis zu 100 FreeSMS/Monat: http://www.gmx.net/de/go/mail

Re: fcntl: F_SETLEASE/F_RDLCK question

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 2211 bytes --]

Hi Michael,

On Tue, 3 May 2005 12:00:18 +0200 (MEST) "Michael Kerrisk" <mtk-lkml@gmx.net> wrote:
>
> Indeed the problem referred to is fixed, but it looks like another 
> one may have been introduced.
> 
> It now appears (I tested on 2.6.11.6) that if a process opens 
> a file O_RDWR and then tries to place a read lease via that
> file descriptor, then the F_SETLEASE fails with EAGAIN, 
> even though no other process has the file open for writing.  
> (On the other hand, if the process opens the file 
> O_WRONLY, then it can place either a read or a write lease.  
> This is how I think things always worked, but it seems 
> inconsistent with the aforementioned behaviour.)  
> 
> Some further testing showed the following (both open() 
> and fcntl(F_SETLEASE) from same process):
> 
>  open()  |  lease requested
>   flag   | F_RDLCK  | F_WRLCK
> ---------+----------+----------
> O_RDONLY | okay     |  okay
> O_WRONLY | EAGAIN   |  okay
> O_RDWR   | EAGAIN   |  okay
> 
> This seems strange (I imagine the caller should be excluded 
> from the list of processes being checked to see if the file 
> is opened for writing), and differs from earlier kernel
> versions.  What is the intended behaviour here?

Thanks for the testing.  My expectation is that it shouldn't matter how
the current process opened the file for either type of lease.  However,
you are right (IMHO) that the current process should *not* be counted as a
writer in the case of trying to obtain a F_RDLCK lease.

How does this (completely untested, not even compiled) patch look?

diff -ruNP linus/fs/locks.c linus-leases.1/fs/locks.c
--- linus/fs/locks.c	2005-04-26 15:38:00.000000000 +1000
+++ linus-leases.1/fs/locks.c	2005-05-03 23:00:14.000000000 +1000
@@ -1288,7 +1288,8 @@
 		goto out;
 
 	error = -EAGAIN;
-	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
+	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount)
+				 > ((filp->f_mode & FMODE_WRITE) ? 1 : 0)))
 		goto out;
 	if ((arg == F_WRLCK)
 	    && ((atomic_read(&dentry->d_count) > 1)

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: fcntl: F_SETLEASE/F_RDLCK question

[William A.(Andy) Adamson]

> Hi Michael,
> 
> On Tue, 3 May 2005 12:00:18 +0200 (MEST) "Michael Kerrisk" <mtk-lkml@gmx.net> wrote:
> >
> > Indeed the problem referred to is fixed, but it looks like another 
> > one may have been introduced.
> > 
> > It now appears (I tested on 2.6.11.6) that if a process opens 
> > a file O_RDWR and then tries to place a read lease via that
> > file descriptor, then the F_SETLEASE fails with EAGAIN, 
> > even though no other process has the file open for writing.  
> > (On the other hand, if the process opens the file 
> > O_WRONLY, then it can place either a read or a write lease.  
> > This is how I think things always worked, but it seems 
> > inconsistent with the aforementioned behaviour.)  
> > 
> > Some further testing showed the following (both open() 
> > and fcntl(F_SETLEASE) from same process):
> > 
> >  open()  |  lease requested
> >   flag   | F_RDLCK  | F_WRLCK
> > ---------+----------+----------
> > O_RDONLY | okay     |  okay
> > O_WRONLY | EAGAIN   |  okay
> > O_RDWR   | EAGAIN   |  okay
> > 
> > This seems strange (I imagine the caller should be excluded 
> > from the list of processes being checked to see if the file 
> > is opened for writing), and differs from earlier kernel
> > versions.  What is the intended behaviour here?
> 
> Thanks for the testing.  My expectation is that it shouldn't matter how
> the current process opened the file for either type of lease.  However,
> you are right (IMHO) that the current process should *not* be counted as a
> writer in the case of trying to obtain a F_RDLCK lease.

i believe the current implementation is correct. opening a file for write 
means that you can not have a read lease, caller included.


-->Andy


> 
> How does this (completely untested, not even compiled) patch look?
> 
> diff -ruNP linus/fs/locks.c linus-leases.1/fs/locks.c
> --- linus/fs/locks.c	2005-04-26 15:38:00.000000000 +1000
> +++ linus-leases.1/fs/locks.c	2005-05-03 23:00:14.000000000 +1000
> @@ -1288,7 +1288,8 @@
>  		goto out;
>  
>  	error = -EAGAIN;
> -	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> +	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount)
> +				 > ((filp->f_mode & FMODE_WRITE) ? 1 : 0)))
>  		goto out;
>  	if ((arg == F_WRLCK)
>  	    && ((atomic_read(&dentry->d_count) > 1)
> 
> -- 
> Cheers,
> Stephen Rothwell                    sfr@canb.auug.org.au
> http://www.canb.auug.org.au/~sfr/

Re: fcntl: F_SETLEASE/F_RDLCK question

[Matthew Wilcox]

On Tue, May 03, 2005 at 09:55:42AM -0400, William A.(Andy) Adamson wrote:
> i believe the current implementation is correct. opening a file for write 
> means that you can not have a read lease, caller included.

Why not?  Certainly, others will not be able to take out a read lease,
so there's very little point to only having a read lease, but I don't
see why we should deny it.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: fcntl: F_SETLEASE/F_RDLCK question

[William A.(Andy) Adamson]

> On Tue, May 03, 2005 at 09:55:42AM -0400, William A.(Andy) Adamson wrote:
> > i believe the current implementation is correct. opening a file for write 
> > means that you can not have a read lease, caller included.
> 
> Why not?  Certainly, others will not be able to take out a read lease,
> so there's very little point to only having a read lease, but I don't
> see why we should deny it.
> 

by definition: a read lease means there are no writers. so, the question is 
not 'why not', the question is why? why hand out a read lease to an open for 
write?

-->Andy

Re: fcntl: F_SETLEASE/F_RDLCK question

[Michael Kerrisk]

> > On Tue, May 03, 2005 at 09:55:42AM -0400, William A.(Andy) Adamson
> > wrote:
> > > i believe the current implementation is correct. opening a file for
> > > write 
> > > means that you can not have a read lease, caller included.
> > 
> > Why not?  Certainly, others will not be able to take out a read lease,
> > so there's very little point to only having a read lease, but I don't
> > see why we should deny it.
> > 
> 
> by definition: a read lease means there are no writers. so, the question
> is 
> not 'why not', the question is why? why hand out a read lease to an open
> for write?

Andy,

Look more closely at my earlier table. 

Regardless of what the answer to your question is, the 
current semantics are bizarre.  As things stand, a process
can open a file O_RDWR, and and can place a WRITE lease 
but not a READ lease.  That can't be right.

FWIW it's worth, I think the read lease should be allowed.
Oplocks are concerned with what other processes are doing, 
not what the caller is doing.  Also, the current semantcis
break backward compatibility.

Cheers,

Michael

-- 
+++ Neu: Echte DSL-Flatrates von GMX - Surfen ohne Limits +++
Always online ab 4,99 Euro/Monat: http://www.gmx.net/de/go/dsl

Re: fcntl: F_SETLEASE/F_RDLCK question

[William A.(Andy) Adamson]

> > > On Tue, May 03, 2005 at 09:55:42AM -0400, William A.(Andy) Adamson
> > > wrote:
> > > > i believe the current implementation is correct. opening a file for
> > > > write 
> > > > means that you can not have a read lease, caller included.
> > > 
> > > Why not?  Certainly, others will not be able to take out a read lease,
> > > so there's very little point to only having a read lease, but I don't
> > > see why we should deny it.
> > > 
> > 
> > by definition: a read lease means there are no writers. so, the question
> > is 
> > not 'why not', the question is why? why hand out a read lease to an open
> > for write?
> 
> Andy,
> 
> Look more closely at my earlier table. 
> 
> Regardless of what the answer to your question is, the 
> current semantics are bizarre.  As things stand, a process
> can open a file O_RDWR, and and can place a WRITE lease 
> but not a READ lease.  That can't be right.

yes - i was being too strict. looking at NFSv4 delegations; a read lease does 
not mean there are no writers, it means there are no other clients (fl_owners) 
writing.

the other side of the coin would be break_lease. it should not break a read 
lease on an open for write in the case where the fl_owner of the read lease is 
also the owner of the open for write.

-->Andy 

> 
> FWIW it's worth, I think the read lease should be allowed.
> Oplocks are concerned with what other processes are doing, 
> not what the caller is doing.  Also, the current semantcis
> break backward compatibility.
> 
> Cheers,
> 
> Michael
> 
> -- 
> +++ Neu: Echte DSL-Flatrates von GMX - Surfen ohne Limits +++
> Always online ab 4,99 Euro/Monat: http://www.gmx.net/de/go/dsl

Re: fcntl: F_SETLEASE/F_RDLCK question

[J. Bruce Fields]

On Tue, May 03, 2005 at 12:21:24PM -0400, William A.(Andy) Adamson wrote:
> the other side of the coin would be break_lease.

Yeah, I'm a little confused as to why anyone would have the expectation
that read leases would not conflict with write opens by the same
process, given that break_lease() has never functioned that way, so
later write opens by the same process have always broken any read lease.

Are there applications that actually depend on the old behaviour?  Is
there any documentation that blesses it?  All I can find is the fcntl
man page, and as far as I can tell an implementation that makes read
leases conflict with all write opens (by the same process or not) is
consistent with that man page.

--b.

Re: fcntl: F_SETLEASE/F_RDLCK question

[Michael Kerrisk]

Hello Stephen,

Sorry for the long delay in replying; other things intervened.

> On Tue, 3 May 2005 12:00:18 +0200 (MEST) "Michael Kerrisk"
> <mtk-lkml@gmx.net> wrote:
> >
> > Indeed the problem referred to is fixed, but it looks like another 
> > one may have been introduced.
> > 
> > It now appears (I tested on 2.6.11.6) that if a process opens 
> > a file O_RDWR and then tries to place a read lease via that
> > file descriptor, then the F_SETLEASE fails with EAGAIN, 
> > even though no other process has the file open for writing.  
> > (On the other hand, if the process opens the file 
> > O_WRONLY, then it can place either a read or a write lease.  
> > This is how I think things always worked, but it seems 
> > inconsistent with the aforementioned behaviour.)  
> > 
> > Some further testing showed the following (both open() 
> > and fcntl(F_SETLEASE) from same process):
> > 
> >  open()  |  lease requested
> >   flag   | F_RDLCK  | F_WRLCK
> > ---------+----------+----------
> > O_RDONLY | okay     |  okay
> > O_WRONLY | EAGAIN   |  okay
> > O_RDWR   | EAGAIN   |  okay
> > 
> > This seems strange (I imagine the caller should be excluded 
> > from the list of processes being checked to see if the file 
> > is opened for writing), and differs from earlier kernel
> > versions.  What is the intended behaviour here?
> 
> Thanks for the testing.  My expectation is that it shouldn't matter how
> the current process opened the file for either type of lease.  However,
> you are right (IMHO) that the current process should *not* be counted as 
> a writer in the case of trying to obtain a F_RDLCK lease.
> 
> How does this (completely untested, not even compiled) patch look?
> 
> diff -ruNP linus/fs/locks.c linus-leases.1/fs/locks.c
> --- linus/fs/locks.c	2005-04-26 15:38:00.000000000 +1000
> +++ linus-leases.1/fs/locks.c	2005-05-03 23:00:14.000000000 +1000
> @@ -1288,7 +1288,8 @@
>  		goto out;
>  
>  	error = -EAGAIN;
> -	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))
> +	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount)
> +				 > ((filp->f_mode & FMODE_WRITE) ? 1 : 0)))
>  		goto out;
>  	if ((arg == F_WRLCK)
>  	    && ((atomic_read(&dentry->d_count) > 1)

I applied this against 2.6.12-rc4, and it fixes the problem 
(and I've also teasted various other facets of file leases 
and this change causes no obvious breakage elsewhere).

Are you going to push this fix into 2.6.12?

Cheers,

Michael

-- 
Geschenkt: 3 Monate GMX ProMail gratis + 3 Ausgaben stern gratis
++ Jetzt anmelden & testen ++ http://www.gmx.net/de/go/promail ++

Re: fcntl: F_SETLEASE/F_RDLCK question

[J. Bruce Fields]

On Tue, May 31, 2005 at 04:53:50PM +0200, Michael Kerrisk wrote:
> I applied this against 2.6.12-rc4, and it fixes the problem 
> (and I've also teasted various other facets of file leases 
> and this change causes no obvious breakage elsewhere).
> 
> Are you going to push this fix into 2.6.12?

Are you sure this is actually a problem?

I still have the following questions I had before:

> I'm a little confused as to why anyone would have the expectation
> that read leases would not conflict with write opens by the same
> process, given that break_lease() has never functioned that way, so
> later write opens by the same process have always broken any read lease.
>
> Are there applications that actually depend on the old behaviour?  Is
> there any documentation that blesses it?  All I can find is the fcntl
> man page, and as far as I can tell an implementation that makes read
> leases conflict with all write opens (by the same process or not) is
> consistent with that man page.

--b.

File leases and fork()

[Michael Kerrisk]

Hi Stephen,

The other recent thread on F_SETLEASE prompted me to go
and recheck another blemish in the file lease code,
which still exists: viz leases and fork().

Via its inherited file descriptors, a child of fork() inherits 
a reference to the file leases held by the parent (file leases 
are maintained in the open file table).  The following details 
are relevant:

-- The file lease is registered only to the parent (as seen
   in /proc/locks).  If a lease breaker arrives, only the 
   parent is signalled.

-- If no explicit F_SETLEASE is done to remove a lease, then
   the lease is only removed when all file descriptors referring 
   to the open file table entry are closed.

These two factors lead to various weirdnesses.  Some examples:

1. If the parent closes its file descriptor, or simply terminates,
   then the lease continues exist (because of the child's open 
   descriptor).  However, if a lease breaker arrives, the
   kernel does not know what process to signal: the child is not 
   signalled and the lease breaker's open() (or truncate()) is
   blocked until /proc/sys/fs/lease-break-time expires (default 
   of 45 seconds), at which point the kernel forcibly breaks or 
   downgrades the lease, allowing the lease breaker to proceed.

2. Suppose a parent does the following:

   -- opens a file, yielding a file descriptor fd
   -- calls fork()
   -- and the child takes out a lease on fd, and then exits

   At this point, the lease continues to exist (because of 
   the *parent's* open file descriptor).  If a lease breaker
   arrives, then as above, no process is signalled (the lease 
   is marked as belonging to the now dead child, as can be
   seen in /proc/locks), and the lease breaker blocks until 
   /proc/sys/fs/lease-break-time expires.

What are the intended semantics of file leases with respect 
to fork()?  I realize that file leases were designed with
SAMBA in mind, and fork() probably wasn't part of the 
original vision, but the current semantics seem broken and 
confusing.  Simplest would perhaps be that a child did not
inherit a reference to the same lease, but I'm not sure
how achievable that would be in the existing design.

Cheers,

Michael

-- 
Weitersagen: GMX DSL-Flatrates mit Tempo-Garantie!
Ab 4,99 Euro/Monat: http://www.gmx.net/de/go/dsl

Re: fcntl: F_SETLEASE/F_RDLCK question

[Michael Kerrisk]

Bruce,

> On Tue, May 31, 2005 at 04:53:50PM +0200, Michael Kerrisk wrote:
> > I applied this against 2.6.12-rc4, and it fixes the problem 
> > (and I've also teasted various other facets of file leases 
> > and this change causes no obvious breakage elsewhere).
> > 
> > Are you going to push this fix into 2.6.12?
> 
> Are you sure this is actually a problem?
> 
> I still have the following questions I had before:
> 
> > I'm a little confused as to why anyone would have the expectation
> > that read leases would not conflict with write opens by the same
> > process, given that break_lease() has never functioned that way, so
> > later write opens by the same process have always broken any read 
> > lease.
> >
> > Are there applications that actually depend on the old behaviour?  Is
> > there any documentation that blesses it?  All I can find is the fcntl
> > man page, and as far as I can tell an implementation that makes read
> > leases conflict with all write opens (by the same process or not) is
> > consistent with that man page.

I believe it is still a problem: primarily because it broke
old behavior for no apparent reason (Stephen Rothwell, who was 
one of the original implementers seems to agree, since he 
suggested that one line patch).  I suspect the change was 
unintentional.

By the way, I wrote the text in the fcntl() man page
by looking at the code and experimenting.  There was no 
existing documentation of F_SETLEASE.  I'm questioning 
the change based on my understanding of how things should 
work (I didn't happen to write up this point because it 
seemed self-evident *to me*); however, I know rather 
little of the workings of SAMBA.

Cheers,

Michael

-- 
Weitersagen: GMX DSL-Flatrates mit Tempo-Garantie!
Ab 4,99 Euro/Monat: http://www.gmx.net/de/go/dsl