[PATCH] fs/locks.c Partially fixes leases issue

[PATCH] fs/locks.c Partially fixes leases issue

[Joseph D. Wagner]

I apologize for being rude earlier.  It's just so frustrating to a newbie 
programmer -- or perhaps phrased better -- a programmer new to Linux kernel 
programming.

I hope my temper does not keep you from including a valid, useful patch in 
the Linux kernel.  This patch can be applied to both the 2.4.x and 2.6.x 
series.

Everything you need to know about the patch is documented in the patch 
itself.  I hope you find it useful, and I hope that my poor first 
impression will not prevent us from working together on filesystem 
development in the future.

Sincerely and apologetically,

Joseph D. Wagner

--- /old/src/linux-2.4.22/fs/locks.c    2003-08-25 17:44:43.000000000 +0600
+++ /new/src/linux-2.4.22/fs/locks.c    2003-11-28 00:05:11.000000000 +0600
@@ -111,10 +111,17 @@
  *  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.
+ *
+ *  PARTIALLY 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
+ *  has been partially resolved.  See FIXME below.
+ *  Joseph D. Wagner <wagnerjd@users.sourceforge.net> November 2003
  */

 #include <linux/slab.h>
 #include <linux/file.h>
 #include <linux/smp_lock.h>
@@ -1287,14 +1294,33 @@

        lock_kernel();

        time_out_leases(inode);
 
-       /*
-        * FIXME: What about F_RDLCK and files open for writing?
-        */
        error = -EAGAIN;
+       if ((arg == F_RDLCK)
+           && ((atomic_read(&dentry->d_count) > 1)
+               || (atomic_read(&inode->i_count) > 1))) {
+
+               /*
+                * FIXME: Theoretically, what would happen next
+                * is a loop which checks each open file to see
+                * if the file is open for writing (i.e. O_WRONLY,
+                * O_RDWR, O_CREAT, or O_TRUNC).  However, since
+                * that would require major overhauls in other
+                * files, it is simply assumed that if a file is
+                * open that the file is open for writing.  In
+                * effect, this creates an exclusive read lease
+                * such that only one process can obtain a read
+                * lease at any given time.  Theoretically, a file
+                * should be able to have a virtually limitless
+                * number of read leases provided that no process
+                * has the file open for writing.
+                */
+
+               goto out_unlock;
+       }
        if ((arg == F_WRLCK)
            && ((atomic_read(&dentry->d_count) > 1)
                || (atomic_read(&inode->i_count) > 1)))
                goto out_unlock;

Re: [PATCH] fs/locks.c Partially fixes leases issue

[Jamie Lokier]

Joseph D. Wagner wrote:
> Everything you need to know about the patch is documented in the patch 
> itself.

Thanks.  A couple of things about the patch:

> +       if ((arg == F_RDLCK)
> +           && ((atomic_read(&dentry->d_count) > 1)
> +               || (atomic_read(&inode->i_count) > 1))) {
> [...]
> +               goto out_unlock;

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

These two are so similar they can be merged; just don't test "arg".

The long comment which I wrote as "[...]" above is excessive: the code
is clear without it.  If every small patch had a comment that large,
the kernel would be all comments and it paradoxically makes it harder
to understand the code.

(I used to write lots of comments in code myself, until I discovered
colleages didn't read them because there were too many, so they
skipped the really important technical notes.  Now I follow the
balance set by other programmers).

A short FIXME comment indicating that the F_RDLCK case is not
implemented properly is useful, though.

> I hope you find it useful, and I hope that my poor first 
> impression will not prevent us from working together on filesystem 
> development in the future.

No problem; welcome!

I don't agree with this patch as it is because, as Sean Neakums
pointed out, there is actually a field `inode->i_writecount' which is
remarkeably appropriate.

F_RDLCK should use `deny_write_access', and `get_write_access' should
be changed to break any F_RDLCK leases.  (Checking that locking for
`break_lease' in the places where `get_write_access' is called is left
as an exercise).

F_WRLCK should use `get_write_accesss' because taking a write lease is
equivalent to demanding write access - it should break any outstanding
F_RDLCK leases.  Alternatively, F_WRLCK should insist that
`filp->f_mode & FMODE_WRITE' is set.

-- Jamie

Re: [PATCH] fs/locks.c Partially fixes leases issue

[Joseph D. Wagner]

On Thursday, December 4, 2003 8:55 am, Jamie Lokier wrote:
> These two are so similar they can be merged; just don't test "arg".

That's what I through at first, but since this is only a partial patch, and 
a future patch would need to run additional checks independent of any write 
leases, I figured that it would be easier for the next guy if it had its 
own section so the next guy would know where the patch goes and have an 
easier time figuring it out.

> The long comment which I wrote as "[...]" above is excessive: the code
> is clear without it.  If every small patch had a comment that large,
> the kernel would be all comments and it paradoxically makes it harder
> to understand the code.

OK.  Since I'm new at this, I don't know how clear my code (or everyone 
else's) is, so I wanted to error on the side of caution.

> I don't agree with this patch as it is because, as Sean Neakums
> pointed out, there is actually a field `inode->i_writecount' which is
> remarkeably appropriate.
>
> F_RDLCK should use `deny_write_access', and `get_write_access' should
> be changed to break any F_RDLCK leases.  (Checking that locking for
> `break_lease' in the places where `get_write_access' is called is left
> as an exercise).
>
> F_WRLCK should use `get_write_accesss' because taking a write lease is
> equivalent to demanding write access - it should break any outstanding
> F_RDLCK leases.  Alternatively, F_WRLCK should insist that
> `filp->f_mode & FMODE_WRITE' is set.

As I said, it's just a partial fix until someone does all that stuff you 
describe.  I really don't know enough to do all that, at least not yet.

Are you saying my patch won't get in?

Joseph D. Wagner

Re: [PATCH] fs/locks.c Partially fixes leases issue

[Jamie Lokier]

Joseph D. Wagner wrote:
> On Thursday, December 4, 2003 8:55 am, Jamie Lokier wrote:
> > These two are so similar they can be merged; just don't test "arg".
> 
> That's what I through at first, but since this is only a partial patch, and 
> a future patch would need to run additional checks independent of any write 
> leases, I figured that it would be easier for the next guy if it had its 
> own section so the next guy would know where the patch goes and have an 
> easier time figuring it out.

That's what a comment saying "FIXME: R_RDLCK is not implemented
properly; it's currently treated the same as F_WRLCK" prior to the
merged test would be good for.

Anyone able to implement F_RDLCK properly is not going to have a
hard time knowing where to put the code :)  Creating an empty place
just in case someone may fill it in future is the same as "this
page is under construction" on a web site :) :)

> > I don't agree with this patch as it is because, as Sean Neakums
> > pointed out, there is actually a field `inode->i_writecount' which is
> > remarkeably appropriate.
[...]
> 
> As I said, it's just a partial fix until someone does all that stuff you 
> describe.  I really don't know enough to do all that, at least not yet.

If you have the time, try my suggestion.  It is not complicated and
will fix the feature properly - and you'll learn a lot!

> Are you saying my patch won't get in?

That's not my decision to make.

It probably should go in (with the simpler test and comment), as it's
a genuine bug fix.

I'm saying that F_RDLCK is presently useless because it's broken, so
no applications dare use it as intended, and your fix doesn't really
change that - it still isn't useful.  I'm egging you on to do it properly :)

-- Jamie

Re: [PATCH] fs/locks.c Partially fixes leases issue

[Matthew Wilcox]

On Wed, Dec 03, 2003 at 09:49:21PM +0600, Joseph D. Wagner wrote:
> Are you saying my patch won't get in?

As the file locking maintainer, I'm saying your patch won't get in --
at least not soon.  Why?  Not because of your attitude or anything else,
but because we're in deep deep freeze.  This bug's been there for over
3 years, we can live with it for a little longer.

-- 
"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