Re: PATCH [2/7] Fix posix locking code
From: Matthew Wilcox <hidden>
Date: 2004-08-14 20:00:49
Also in:
lkml
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;
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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