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

[Jamie Lokier <jamie@shareable.org>]

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