* PATCH [2/7] Fix posix locking code
@ 2004-08-14 19:29 Trond Myklebust
2004-08-14 19:53 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Trond Myklebust @ 2004-08-14 19:29 UTC (permalink / raw)
To: Linux Filesystem Development, linux-kernel, Linus Torvalds,
Andrew Morton
VFS: Enable filesystems and to hook certain functions for copying
locks, and freeing locks using the new struct file_lock_operations.
VFS: Enable lock managers (i.e. lockd) to hook functions for comparing
lock ownership using the new struct lock_manager_operations.
Signed-off-by: Trond Myklebust <trond.myklebust@fys.uio.no>
fs/locks.c | 33 +++++++++++++++++++++++++++------
include/linux/fs.h | 11 +++++++++++
2 files changed, 38 insertions(+), 6 deletions(-)
diff -u --recursive --new-file --show-c-function linux-2.6.8.1-01-fix_locks/fs/locks.c linux-2.6.8.1-02-fix_locks2/fs/locks.c
--- linux-2.6.8.1-01-fix_locks/fs/locks.c 2004-08-14 14:28:58.000000000 -0400
+++ linux-2.6.8.1-02-fix_locks2/fs/locks.c 2004-08-14 14:29:12.000000000 -0400
@@ -167,6 +167,12 @@ static inline void locks_free_lock(struc
if (!list_empty(&fl->fl_link))
panic("Attempting to free lock on active lock list");
+ if (fl->fl_ops && fl->fl_ops->fl_release_private) {
+ fl->fl_ops->fl_release_private(fl);
+ fl->fl_ops = NULL;
+ }
+ fl->fl_lmops = NULL;
+
kmem_cache_free(filelock_cache, fl);
}
@@ -186,6 +192,8 @@ void locks_init_lock(struct file_lock *f
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
}
EXPORT_SYMBOL(locks_init_lock);
@@ -220,7 +228,10 @@ void locks_copy_lock(struct file_lock *n
new->fl_notify = fl->fl_notify;
new->fl_insert = fl->fl_insert;
new->fl_remove = fl->fl_remove;
- new->fl_u = fl->fl_u;
+ new->fl_ops = fl->fl_ops;
+ new->fl_lmops = fl->fl_lmops;
+ if (fl->fl_ops && fl->fl_ops->fl_copy_lock)
+ fl->fl_ops->fl_copy_lock(new, fl);
}
EXPORT_SYMBOL(locks_copy_lock);
@@ -324,6 +335,8 @@ static int flock_to_posix_lock(struct fi
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
return assign_type(fl, l->l_type);
}
@@ -364,6 +377,8 @@ static int flock64_to_posix_lock(struct
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
switch (l->l_type) {
case F_RDLCK:
@@ -400,6 +415,8 @@ static int lease_alloc(struct file *filp
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
*flp = fl;
return 0;
@@ -419,10 +436,9 @@ static inline int locks_overlap(struct f
static inline int
posix_same_owner(struct file_lock *fl1, struct file_lock *fl2)
{
- /* FIXME: Replace this sort of thing with struct file_lock_operations */
- if ((fl1->fl_type | fl2->fl_type) & FL_LOCKD)
- return fl1->fl_owner == fl2->fl_owner &&
- fl1->fl_pid == fl2->fl_pid;
+ if (fl1->fl_lmops && fl1->fl_lmops->fl_compare_owner)
+ return fl2->fl_lmops == fl1->fl_lmops &&
+ fl1->fl_lmops->fl_compare_owner(fl1, fl2);
return fl1->fl_owner == fl2->fl_owner;
}
@@ -981,6 +997,8 @@ int locks_mandatory_area(int read_write,
break;
}
+ if (fl.fl_ops && fl.fl_ops->fl_release_private)
+ fl.fl_ops->fl_release_private(&fl);
return error;
}
@@ -1415,7 +1433,6 @@ int fcntl_getlk(struct file *filp, struc
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
-
out:
return error;
}
@@ -1665,6 +1682,8 @@ void locks_remove_posix(struct file *fil
lock.fl_owner = owner;
lock.fl_pid = current->tgid;
lock.fl_file = filp;
+ lock.fl_ops = NULL;
+ lock.fl_lmops = NULL;
if (filp->f_op && filp->f_op->lock != NULL) {
filp->f_op->lock(filp, F_SETLK, &lock);
@@ -1684,6 +1703,8 @@ void locks_remove_posix(struct file *fil
before = &fl->fl_next;
}
unlock_kernel();
+ if (lock.fl_ops && lock.fl_ops->fl_release_private)
+ lock.fl_ops->fl_release_private(&lock);
}
EXPORT_SYMBOL(locks_remove_posix);
diff -u --recursive --new-file --show-c-function linux-2.6.8.1-01-fix_locks/include/linux/fs.h linux-2.6.8.1-02-fix_locks2/include/linux/fs.h
--- linux-2.6.8.1-01-fix_locks/include/linux/fs.h 2004-08-14 14:25:55.000000000 -0400
+++ linux-2.6.8.1-02-fix_locks2/include/linux/fs.h 2004-08-14 14:40:11.000000000 -0400
@@ -626,6 +626,15 @@ extern void close_private_file(struct fi
*/
typedef struct files_struct *fl_owner_t;
+struct file_lock_operations {
+ void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
+ void (*fl_release_private)(struct file_lock *);
+};
+
+struct lock_manager_operations {
+ int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
+};
+
/* that will die - we need it for nfs_lock_info */
#include <linux/nfs_fs_i.h>
@@ -649,6 +658,8 @@ struct file_lock {
struct fasync_struct * fl_fasync; /* for lease break notifications */
unsigned long fl_break_time; /* for nonblocking lease breaks */
+ struct file_lock_operations *fl_ops; /* Callbacks for filesystems */
+ struct lock_manager_operations *fl_lmops; /* Callbacks for lockmanagers */
union {
struct nfs_lock_info nfs_fl;
} fl_u;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PATCH [2/7] Fix posix locking code
2004-08-14 19:29 PATCH [2/7] Fix posix locking code Trond Myklebust
@ 2004-08-14 19:53 ` Christoph Hellwig
2004-08-14 20:19 ` Trond Myklebust
2004-08-14 20:00 ` Matthew Wilcox
2004-08-14 20:30 ` PATCH [2/7] Fix posix locking code (resend with fixes) Trond Myklebust
2 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2004-08-14 19:53 UTC (permalink / raw)
To: Trond Myklebust
Cc: Linux Filesystem Development, linux-kernel, Linus Torvalds,
Andrew Morton
On Sat, Aug 14, 2004 at 03:29:53PM -0400, Trond Myklebust wrote:
> VFS: Enable filesystems and to hook certain functions for copying
> locks, and freeing locks using the new struct file_lock_operations.
>
> VFS: Enable lock managers (i.e. lockd) to hook functions for comparing
> lock ownership using the new struct lock_manager_operations.
Please document these operations and their locking rules in
Documentation/filesystems/Locking
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH [2/7] Fix posix locking code
2004-08-14 19:53 ` Christoph Hellwig
@ 2004-08-14 20:19 ` Trond Myklebust
0 siblings, 0 replies; 6+ messages in thread
From: Trond Myklebust @ 2004-08-14 20:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Linux Filesystem Development, linux-kernel, Linus Torvalds,
Andrew Morton
På lau , 14/08/2004 klokka 15:53, skreiv Christoph Hellwig:
> On Sat, Aug 14, 2004 at 03:29:53PM -0400, Trond Myklebust wrote:
> > VFS: Enable filesystems and to hook certain functions for copying
> > locks, and freeing locks using the new struct file_lock_operations.
> >
> > VFS: Enable lock managers (i.e. lockd) to hook functions for comparing
> > lock ownership using the new struct lock_manager_operations.
>
> Please document these operations and their locking rules in
> Documentation/filesystems/Locking
Right here...
diff -u --recursive --new-file linux-2.6.8.1-07-cleanup_posix/Documentation/filesystems/Locking linux-2.6.8.1-08-Documentation/Documentation/filesystems/Locking
--- linux-2.6.8.1-07-cleanup_posix/Documentation/filesystems/Locking 2004-08-14 14:27:32.000000000 -0400
+++ linux-2.6.8.1-08-Documentation/Documentation/filesystems/Locking 2004-08-14 16:07:11.000000000 -0400
@@ -276,21 +276,34 @@
internal fs locking and real critical areas are much smaller than the areas
filesystems protect now.
---------------------------- file_lock ------------------------------------
+----------------------- file_lock_operations ------------------------------
prototypes:
- void (*fl_notify)(struct file_lock *); /* unblock callback */
void (*fl_insert)(struct file_lock *); /* lock insertion callback */
void (*fl_remove)(struct file_lock *); /* lock removal callback */
+ void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
+ void (*fl_release_private)(struct file_lock *);
+
locking rules:
- BKL may block
-fl_notify: yes no
-fl_insert: yes no
-fl_remove: yes no
+ BKL may block
+fl_insert: yes no
+fl_remove: yes no
+fl_copy_lock: yes no
+fl_release_private: yes yes
+
+----------------------- lock_manager_operations ---------------------------
+prototypes:
+ int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
+ void (*fl_notify)(struct file_lock *); /* unblock callback */
+
+locking rules:
+ BKL may block
+fl_compare_owner: yes no
+fl_notify: yes no
+
Currently only NLM provides instances of this class. None of the
them block. If you have out-of-tree instances - please, show up. Locking
in that area will change.
-
--------------------------- buffer_head -----------------------------------
prototypes:
void (*b_end_io)(struct buffer_head *bh, int uptodate);
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH [2/7] Fix posix locking code
2004-08-14 19:29 PATCH [2/7] Fix posix locking code Trond Myklebust
2004-08-14 19:53 ` Christoph Hellwig
@ 2004-08-14 20:00 ` Matthew Wilcox
2004-08-14 20:18 ` Trond Myklebust
2004-08-14 20:30 ` PATCH [2/7] Fix posix locking code (resend with fixes) Trond Myklebust
2 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2004-08-14 20:00 UTC (permalink / raw)
To: Trond Myklebust
Cc: Linux Filesystem Development, linux-kernel, Linus Torvalds,
Andrew Morton
On Sat, Aug 14, 2004 at 03:29:53PM -0400, Trond Myklebust wrote:
> if (!list_empty(&fl->fl_link))
> panic("Attempting to free lock on active lock list");
>
> + if (fl->fl_ops && fl->fl_ops->fl_release_private) {
> + fl->fl_ops->fl_release_private(fl);
> + fl->fl_ops = NULL;
> + }
> + fl->fl_lmops = NULL;
So if fl_ops is set, but fl_ops->fl_release_private isn't, we won't set
fl_ops to NULL -- we should probably just do:
if (fl->fl_ops && fl->fl_ops->fl_release_private)
fl->fl_ops->fl_release_private(fl);
fl->fl_ops = NULL;
fl->fl_lmops = NULL;
> @@ -981,6 +997,8 @@ int locks_mandatory_area(int read_write,
> break;
> }
>
> + if (fl.fl_ops && fl.fl_ops->fl_release_private)
> + fl.fl_ops->fl_release_private(&fl);
> return error;
> }
>
I don't see how fl.fl_ops can be non-null here. We initialise it to
NULL in locks_init_lock() and then don't give the underlying filesystem
an opportunity to set it.
> @@ -626,6 +626,15 @@ extern void close_private_file(struct fi
> */
> typedef struct files_struct *fl_owner_t;
>
> +struct file_lock_operations {
> + void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
> + void (*fl_release_private)(struct file_lock *);
> +};
> +
> +struct lock_manager_operations {
> + int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
> +};
> +
> /* that will die - we need it for nfs_lock_info */
> #include <linux/nfs_fs_i.h>
>
> @@ -649,6 +658,8 @@ struct file_lock {
> struct fasync_struct * fl_fasync; /* for lease break notifications */
> unsigned long fl_break_time; /* for nonblocking lease breaks */
>
> + struct file_lock_operations *fl_ops; /* Callbacks for filesystems */
> + struct lock_manager_operations *fl_lmops; /* Callbacks for lockmanagers */
> union {
> struct nfs_lock_info nfs_fl;
> } fl_u;
I know I said I thought file_lock_operations was the right thing to
do ... but now I think that this isn't a property of the file_lock so
much as it is a property of the underlying filesystem. I think putting a
lock_operations into struct file is maybe a bit much. How about adding
a lock_operations pointer to file_operations? It'd be a little clunky
to get to -- fl->fl_file->f_op->lock_ops, so I'd be interested in other
suggestions.
--
"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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PATCH [2/7] Fix posix locking code
2004-08-14 20:00 ` Matthew Wilcox
@ 2004-08-14 20:18 ` Trond Myklebust
0 siblings, 0 replies; 6+ messages in thread
From: Trond Myklebust @ 2004-08-14 20:18 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Linux Filesystem Development, linux-kernel, Linus Torvalds,
Andrew Morton
På lau , 14/08/2004 klokka 16:00, skreiv Matthew Wilcox:
> I know I said I thought file_lock_operations was the right thing to
> do ... but now I think that this isn't a property of the file_lock so
> much as it is a property of the underlying filesystem. I think putting a
> lock_operations into struct file is maybe a bit much. How about adding
> a lock_operations pointer to file_operations?
Lock operations are as much a property of the lock *type*. A BSD lock
may/will have an entirely different set of callbacks.
Trond
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* PATCH [2/7] Fix posix locking code (resend with fixes)
2004-08-14 19:29 PATCH [2/7] Fix posix locking code Trond Myklebust
2004-08-14 19:53 ` Christoph Hellwig
2004-08-14 20:00 ` Matthew Wilcox
@ 2004-08-14 20:30 ` Trond Myklebust
2 siblings, 0 replies; 6+ messages in thread
From: Trond Myklebust @ 2004-08-14 20:30 UTC (permalink / raw)
To: Linux Filesystem Development, Matthew Wilcox
Cc: linux-kernel, Linus Torvalds, Andrew Morton
VFS: Enable filesystems and to hook certain functions for copying
locks, and freeing locks using the new struct file_lock_operations.
VFS: Enable lock managers (i.e. lockd) to hook functions for comparing
lock ownership using the new struct lock_manager_operations.
Signed-off-by: Trond Myklebust <trond.myklebust@fys.uio.no>
fs/locks.c | 32 ++++++++++++++++++++++++++------
include/linux/fs.h | 11 +++++++++++
2 files changed, 37 insertions(+), 6 deletions(-)
diff -u --recursive --new-file --show-c-function linux-2.6.8.1-01-fix_locks/fs/locks.c linux-2.6.8.1-02-fix_locks2/fs/locks.c
--- linux-2.6.8.1-01-fix_locks/fs/locks.c 2004-08-14 14:28:58.000000000 -0400
+++ linux-2.6.8.1-02-fix_locks2/fs/locks.c 2004-08-14 16:24:29.000000000 -0400
@@ -167,6 +167,13 @@ static inline void locks_free_lock(struc
if (!list_empty(&fl->fl_link))
panic("Attempting to free lock on active lock list");
+ if (fl->fl_ops) {
+ if (fl->fl_ops->fl_release_private)
+ fl->fl_ops->fl_release_private(fl);
+ fl->fl_ops = NULL;
+ }
+ fl->fl_lmops = NULL;
+
kmem_cache_free(filelock_cache, fl);
}
@@ -186,6 +193,8 @@ void locks_init_lock(struct file_lock *f
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
}
EXPORT_SYMBOL(locks_init_lock);
@@ -220,7 +229,10 @@ void locks_copy_lock(struct file_lock *n
new->fl_notify = fl->fl_notify;
new->fl_insert = fl->fl_insert;
new->fl_remove = fl->fl_remove;
- new->fl_u = fl->fl_u;
+ new->fl_ops = fl->fl_ops;
+ new->fl_lmops = fl->fl_lmops;
+ if (fl->fl_ops && fl->fl_ops->fl_copy_lock)
+ fl->fl_ops->fl_copy_lock(new, fl);
}
EXPORT_SYMBOL(locks_copy_lock);
@@ -324,6 +336,8 @@ static int flock_to_posix_lock(struct fi
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
return assign_type(fl, l->l_type);
}
@@ -364,6 +378,8 @@ static int flock64_to_posix_lock(struct
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
switch (l->l_type) {
case F_RDLCK:
@@ -400,6 +416,8 @@ static int lease_alloc(struct file *filp
fl->fl_notify = NULL;
fl->fl_insert = NULL;
fl->fl_remove = NULL;
+ fl->fl_ops = NULL;
+ fl->fl_lmops = NULL;
*flp = fl;
return 0;
@@ -419,10 +437,9 @@ static inline int locks_overlap(struct f
static inline int
posix_same_owner(struct file_lock *fl1, struct file_lock *fl2)
{
- /* FIXME: Replace this sort of thing with struct file_lock_operations */
- if ((fl1->fl_type | fl2->fl_type) & FL_LOCKD)
- return fl1->fl_owner == fl2->fl_owner &&
- fl1->fl_pid == fl2->fl_pid;
+ if (fl1->fl_lmops && fl1->fl_lmops->fl_compare_owner)
+ return fl2->fl_lmops == fl1->fl_lmops &&
+ fl1->fl_lmops->fl_compare_owner(fl1, fl2);
return fl1->fl_owner == fl2->fl_owner;
}
@@ -1415,7 +1432,6 @@ int fcntl_getlk(struct file *filp, struc
error = -EFAULT;
if (!copy_to_user(l, &flock, sizeof(flock)))
error = 0;
-
out:
return error;
}
@@ -1665,6 +1681,8 @@ void locks_remove_posix(struct file *fil
lock.fl_owner = owner;
lock.fl_pid = current->tgid;
lock.fl_file = filp;
+ lock.fl_ops = NULL;
+ lock.fl_lmops = NULL;
if (filp->f_op && filp->f_op->lock != NULL) {
filp->f_op->lock(filp, F_SETLK, &lock);
@@ -1684,6 +1702,8 @@ void locks_remove_posix(struct file *fil
before = &fl->fl_next;
}
unlock_kernel();
+ if (lock.fl_ops && lock.fl_ops->fl_release_private)
+ lock.fl_ops->fl_release_private(&lock);
}
EXPORT_SYMBOL(locks_remove_posix);
diff -u --recursive --new-file --show-c-function linux-2.6.8.1-01-fix_locks/include/linux/fs.h linux-2.6.8.1-02-fix_locks2/include/linux/fs.h
--- linux-2.6.8.1-01-fix_locks/include/linux/fs.h 2004-08-14 14:25:55.000000000 -0400
+++ linux-2.6.8.1-02-fix_locks2/include/linux/fs.h 2004-08-14 14:40:11.000000000 -0400
@@ -626,6 +626,15 @@ extern void close_private_file(struct fi
*/
typedef struct files_struct *fl_owner_t;
+struct file_lock_operations {
+ void (*fl_copy_lock)(struct file_lock *, struct file_lock *);
+ void (*fl_release_private)(struct file_lock *);
+};
+
+struct lock_manager_operations {
+ int (*fl_compare_owner)(struct file_lock *, struct file_lock *);
+};
+
/* that will die - we need it for nfs_lock_info */
#include <linux/nfs_fs_i.h>
@@ -649,6 +658,8 @@ struct file_lock {
struct fasync_struct * fl_fasync; /* for lease break notifications */
unsigned long fl_break_time; /* for nonblocking lease breaks */
+ struct file_lock_operations *fl_ops; /* Callbacks for filesystems */
+ struct lock_manager_operations *fl_lmops; /* Callbacks for lockmanagers */
union {
struct nfs_lock_info nfs_fl;
} fl_u;
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-08-14 20:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-08-14 19:29 PATCH [2/7] Fix posix locking code Trond Myklebust
2004-08-14 19:53 ` Christoph Hellwig
2004-08-14 20:19 ` Trond Myklebust
2004-08-14 20:00 ` Matthew Wilcox
2004-08-14 20:18 ` Trond Myklebust
2004-08-14 20:30 ` PATCH [2/7] Fix posix locking code (resend with fixes) Trond Myklebust
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).