LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27 18:03     ` Jamie Lokier
@ 2003-11-27  8:49       ` Joseph D. Wagner
  2003-11-28  1:15         ` Jamie Lokier
  0 siblings, 1 reply; 8+ messages in thread
From: Joseph D. Wagner @ 2003-11-27  8:49 UTC (permalink / raw)
  To: Jamie Lokier, Nikita Danilov
  Cc: linux-fsdevel, linux-kernel, 'Matthew Wilcox'

>> But I THINK this is how a patch would fix the problem, in theory.

> Sorry, it won't.
...
> To detect if anyone has the file open for writing, you'll a new count
> field which keeps track of writer references.  Something like this:
>
>        if ((arg == F_RDLCK)
>            && ((atomic_read(&inode->i_writer_count) != 0)))
>
> You'll also need to modify all the places where that needs to be
> maintained.

Well, dang it all.  Why didn't they guy who implemented leasing in the first 
place bother to do it right the first time?

I don't have the time or technical expertise in kernel development to go 
through all that.  Somebody else is going to have to pick up his slack.

Joseph D. Wagner


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
@ 2003-11-27 16:35 Joseph D. Wagner
  2003-11-27 16:50 ` Jamie Lokier
  0 siblings, 1 reply; 8+ messages in thread
From: Joseph D. Wagner @ 2003-11-27 16:35 UTC (permalink / raw)
  To: linux-fsdevel, linux-kernel
  Cc: 'Matthew Wilcox', 'Jamie Lokier'

Gee, that seemed simple enough.

Keep in mind that I'm new to this whole 'kernel development' thing, and I offer no assurance that my patch actually works.  I only know that it compiles without errors or warnings.

But I THINK this is how a patch would fix the problem, in theory.

This is where the 'theory of a thousand eyes' is going to have to help me out.

TIA guys.

BTW, this patch should be applied to both the 2.4 and 2.6 series of kernels (if it works).

Joseph D. Wagner

--- /old/linux-2.4.22/fs/locks.c	2003-08-25 17:44:43.000000000 +0600
+++ /new/linux-2.4.22/fs/locks.c	2003-11-27 10:08:41.000000000 +0600
@@ -111,10 +111,16 @@
  *  Matthew Wilcox <willy@thepuffingroup.com>, March, 2000.
  *
  *  Leases and LOCK_MAND
  *  Matthew Wilcox <willy@linuxcare.com>, June, 2000.
  *  Stephen Rothwell <sfr@canb.auug.org.au>, June, 2000.
+ *
+ *  FIXED Leases Issue -- Function fcntl_setlease would only check if a
+ *  file had been opened before granting a F_WRLCK, a.k.a. a write lease.
+ *  It would not check if the file had be opened for writing before
+ *  granting a F_RDLCK, a.k.a. a read lease.  This issue is now resolved.
+ *  Joseph D. Wagner <wagnerjd@users.sourceforge.net> November 2003
  */
 
 #include <linux/slab.h>
 #include <linux/file.h>
 #include <linux/smp_lock.h>
@@ -1287,18 +1293,25 @@
 
 	lock_kernel();
 
 	time_out_leases(inode);
 
-	/*
-	 * FIXME: What about F_RDLCK and files open for writing?
-	 */
 	error = -EAGAIN;
 	if ((arg == F_WRLCK)
 	    && ((atomic_read(&dentry->d_count) > 1)
 		|| (atomic_read(&inode->i_count) > 1)))
 		goto out_unlock;
+	if ((arg == F_RDLCK)
+	    && ((dentry->d_flags & O_WRONLY)
+		|| (dentry->d_flags & O_RDWR)
+		|| (dentry->d_flags & O_CREAT)
+		|| (dentry->d_flags & O_TRUNC)
+		|| (inode->i_flags & O_WRONLY)
+		|| (inode->i_flags & O_RDWR)
+		|| (inode->i_flags & O_CREAT)
+		|| (inode->i_flags & O_TRUNC)))
+		goto out_unlock;
 
 	/*
 	 * At this point, we know that if there is an exclusive
 	 * lease on this file, then we hold it on this filp
 	 * (otherwise our open of this file would have blocked).


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27 16:35 [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease Joseph D. Wagner
@ 2003-11-27 16:50 ` Jamie Lokier
  2003-11-27 17:45   ` Nikita Danilov
  2003-12-02 10:53   ` Sean Neakums
  0 siblings, 2 replies; 8+ messages in thread
From: Jamie Lokier @ 2003-11-27 16:50 UTC (permalink / raw)
  To: Joseph D. Wagner; +Cc: linux-fsdevel, linux-kernel, 'Matthew Wilcox'

Joseph D. Wagner wrote:
> But I THINK this is how a patch would fix the problem, in theory.

Sorry, it won't.

> +	if ((arg == F_RDLCK)
> +	    && ((dentry->d_flags & O_WRONLY)
> +		|| (dentry->d_flags & O_RDWR)
> +		|| (dentry->d_flags & O_CREAT)
> +		|| (dentry->d_flags & O_TRUNC)
> +		|| (inode->i_flags & O_WRONLY)
> +		|| (inode->i_flags & O_RDWR)
> +		|| (inode->i_flags & O_CREAT)
> +		|| (inode->i_flags & O_TRUNC)))
> +		goto out_unlock;

dentry->d_flags is a combination of the S_ flags, not O_ flags.
E.g. S_SYNC, S_NOATIME etc.

inode->i_flags is a combination of the DCACHE_ flags.
E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.

To detect if anyone has the file open for writing, you'll a new count
field which keeps track of writer references.  Something like this:

	if ((arg == F_RDLCK)
	    && ((atomic_read(&inode->i_writer_count) != 0)))

You'll also need to modify all the places where that needs to be
maintained.

Btw, I'm not sure why the F_WRLCK case needs to check d_count and
i_count.  Isn't it enough to check d_count?  Won't all opens reference
the inode through a dentry?:

>  	if ((arg == F_WRLCK)
>  	    && ((atomic_read(&dentry->d_count) > 1)
>  		|| (atomic_read(&inode->i_count) > 1)))

-- Jamie

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27 16:50 ` Jamie Lokier
@ 2003-11-27 17:45   ` Nikita Danilov
  2003-11-27 18:03     ` Jamie Lokier
  2003-12-02 10:53   ` Sean Neakums
  1 sibling, 1 reply; 8+ messages in thread
From: Nikita Danilov @ 2003-11-27 17:45 UTC (permalink / raw)
  To: Jamie Lokier
  Cc: Joseph D. Wagner, linux-fsdevel, linux-kernel, 'Matthew Wilcox'

Jamie Lokier writes:
 > Joseph D. Wagner wrote:
 > > But I THINK this is how a patch would fix the problem, in theory.
 > 
 > Sorry, it won't.
 > 
 > > +	if ((arg == F_RDLCK)
 > > +	    && ((dentry->d_flags & O_WRONLY)
 > > +		|| (dentry->d_flags & O_RDWR)
 > > +		|| (dentry->d_flags & O_CREAT)
 > > +		|| (dentry->d_flags & O_TRUNC)
 > > +		|| (inode->i_flags & O_WRONLY)
 > > +		|| (inode->i_flags & O_RDWR)
 > > +		|| (inode->i_flags & O_CREAT)
 > > +		|| (inode->i_flags & O_TRUNC)))
 > > +		goto out_unlock;
 > 
 > dentry->d_flags is a combination of the S_ flags, not O_ flags.
 > E.g. S_SYNC, S_NOATIME etc.
 > 
 > inode->i_flags is a combination of the DCACHE_ flags.
 > E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.

I think it is other way around. ->i_flags are combined from S_SYNC (see,
include/linux/fs.h:IS_IMMUTABLE(), for example), and ->d_flags are
combined from DCACHE_*, latter is explicitly stated in
include/linux/dcache.h

 > 
 > To detect if anyone has the file open for writing, you'll a new count
 > field which keeps track of writer references.  Something like this:
 > 
 > 	if ((arg == F_RDLCK)
 > 	    && ((atomic_read(&inode->i_writer_count) != 0)))
 > 
 > You'll also need to modify all the places where that needs to be
 > maintained.
 > 
 > Btw, I'm not sure why the F_WRLCK case needs to check d_count and
 > i_count.  Isn't it enough to check d_count?  Won't all opens reference
 > the inode through a dentry?:
 > 
 > >  	if ((arg == F_WRLCK)
 > >  	    && ((atomic_read(&dentry->d_count) > 1)
 > >  		|| (atomic_read(&inode->i_count) > 1)))
 > 
 > -- Jamie

Nikita.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27 17:45   ` Nikita Danilov
@ 2003-11-27 18:03     ` Jamie Lokier
  2003-11-27  8:49       ` Joseph D. Wagner
  0 siblings, 1 reply; 8+ messages in thread
From: Jamie Lokier @ 2003-11-27 18:03 UTC (permalink / raw)
  To: Nikita Danilov
  Cc: Joseph D. Wagner, linux-fsdevel, linux-kernel, 'Matthew Wilcox'

Nikita Danilov wrote:
>  > dentry->d_flags is a combination of the S_ flags, not O_ flags.
>  > E.g. S_SYNC, S_NOATIME etc.
>  > 
>  > inode->i_flags is a combination of the DCACHE_ flags.
>  > E.g. DCACHE_AUTOFS_PENDING, DCACHE_REFERENCED tc.
> 
> I think it is other way around. ->i_flags are combined from S_SYNC (see,
> include/linux/fs.h:IS_IMMUTABLE(), for example), and ->d_flags are
> combined from DCACHE_*, latter is explicitly stated in
> include/linux/dcache.h

Oh, yes of course :)

-- Jamie

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27  8:49       ` Joseph D. Wagner
@ 2003-11-28  1:15         ` Jamie Lokier
  0 siblings, 0 replies; 8+ messages in thread
From: Jamie Lokier @ 2003-11-28  1:15 UTC (permalink / raw)
  To: Joseph D. Wagner; +Cc: linux-fsdevel, linux-kernel

Joseph D. Wagner wrote:
> Well, dang it all.  Why didn't they guy who implemented leasing in the first 
> place bother to do it right the first time?
>
> I don't have the time or technical expertise in kernel development to go 
> through all that.  Somebody else is going to have to pick up his slack.

This isn't a commercial project with teams picking up "slack".  It's
mostly volunteers, using their private time as they see fit.  Progress
is made in pieces, nobody is obliged to "finish" anything, and people
contribute incomplete features so that someone else with time, energy
and inclination might complete them.

Being rude is inappropriate.  Words like "didn't bother" and "slack"
are offensive in this context, much like calling someone who works 100
hours a week "a lazy programmer" because they'd need 200 hours to do
what you think they should have done is offensive.

-- Jamie

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-11-27 16:50 ` Jamie Lokier
  2003-11-27 17:45   ` Nikita Danilov
@ 2003-12-02 10:53   ` Sean Neakums
  2003-12-03  2:02     ` Jamie Lokier
  1 sibling, 1 reply; 8+ messages in thread
From: Sean Neakums @ 2003-12-02 10:53 UTC (permalink / raw)
  To: linux-kernel

Jamie Lokier <jamie@shareable.org> writes:

> To detect if anyone has the file open for writing, you'll a new count
> field which keeps track of writer references.  Something like this:
>
> 	if ((arg == F_RDLCK)
> 	    && ((atomic_read(&inode->i_writer_count) != 0)))
>
> You'll also need to modify all the places where that needs to be
> maintained.

Looking at 2.6.0-test11, it seems that its struct inode has such a
field, albeit named i_writecount.  Commentary from fs/namei.c:

/*
 * get_write_access() gets write permission for a file.
 * put_write_access() releases this write permission.
 * This is used for regular files.
 * We cannot support write (and maybe mmap read-write shared) accesses and
 * MAP_DENYWRITE mmappings simultaneously. The i_writecount field of an inode
 * can have the following values:
 * 0: no writers, no VM_DENYWRITE mappings
 * < 0: (-i_writecount) vm_area_structs with VM_DENYWRITE set exist
 * > 0: (i_writecount) users are writing to the file.
 *
 * Normally we operate on that counter with atomic_{inc,dec} and it's safe
 * except for the cases where we don't hold i_writecount yet. Then we need to
 * use {get,deny}_write_access() - these functions check the sign and refuse
 * to do the change if sign is wrong. Exclusion between them is provided by
 * spinlock (arbitration_lock) and I'll rip the second arsehole to the first
 * who will try to move it in struct inode - just leave it here.
 */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease
  2003-12-02 10:53   ` Sean Neakums
@ 2003-12-03  2:02     ` Jamie Lokier
  0 siblings, 0 replies; 8+ messages in thread
From: Jamie Lokier @ 2003-12-03  2:02 UTC (permalink / raw)
  To: linux-kernel

Sean Neakums wrote:
> > 	if ((arg == F_RDLCK)
> > 	    && ((atomic_read(&inode->i_writer_count) != 0)))
> >
> > You'll also need to modify all the places where that needs to be
> > maintained.
> 
> Looking at 2.6.0-test11, it seems that its struct inode has such a
> field, albeit named i_writecount.  Commentary from fs/namei.c:

Cool.  At a glance, it looks like deny_write_access() can be used to
handle the F_RDLCK case.

(It would also be nice to have some way of blocking until the lease can
be taken, but even the F_WRLCK case doesn't offer that at the moment).

-- Jamie

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2003-12-03  2:02 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-11-27 16:35 [PATCH] fs/locks.c fcntl_setlease did not check if a file was opened for writing before granting a read lease Joseph D. Wagner
2003-11-27 16:50 ` Jamie Lokier
2003-11-27 17:45   ` Nikita Danilov
2003-11-27 18:03     ` Jamie Lokier
2003-11-27  8:49       ` Joseph D. Wagner
2003-11-28  1:15         ` Jamie Lokier
2003-12-02 10:53   ` Sean Neakums
2003-12-03  2:02     ` Jamie Lokier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).