Re: [PATCH] problem with locks_remove_posix calling filesystem lock operation

Re: [PATCH] problem with locks_remove_posix calling filesystem lock operation

[Matthew Wilcox]

On Fri, May 24, 2002 at 02:16:25PM -0500, Brian Dixon wrote:
> Steps to Reproduce:
> 1. fcntl locking on a filesystem that defines the lock operation
> 2. a second thread tests a lock while the first is in the process of unlock
>    due to the file being closed.

Nice analysis!  I have an alternate patch (based on the work I've
been doing for 2.5), which I'd suggest applying instead.  It makes
locks_remove_posix act much more like a normal fcntl setlk operation.
Comments?

--- linux-2418/fs/locks.c	Thu Oct 11 08:52:18 2001
+++ linux-2418-flock/fs/locks.c	Fri May 24 21:05:02 2002
@@ -473,25 +473,16 @@
 }
 
 /*
- * Remove lock from the lock lists
+ * Delete a lock and then free it.
  */
-static inline void _unhash_lock(struct file_lock **thisfl_p)
+static void locks_delete_lock(struct file_lock **thisfl_p, unsigned int wait)
 {
 	struct file_lock *fl = *thisfl_p;
 
 	*thisfl_p = fl->fl_next;
 	fl->fl_next = NULL;
-
 	list_del_init(&fl->fl_link);
-}
 
-/*
- * Wake up processes that are blocked waiting for this lock,
- * notify the FS that the lock has been cleared and
- * finally free the lock.
- */
-static inline void _delete_lock(struct file_lock *fl, unsigned int wait)
-{
 	fasync_helper(0, fl->fl_file, 0, &fl->fl_fasync);
 	if (fl->fl_fasync != NULL){
 		printk(KERN_ERR "locks_delete_lock: fasync == %p\n", fl->fl_fasync);
@@ -505,36 +496,6 @@
 	locks_free_lock(fl);
 }
 
-/*
- * Delete a lock and then free it.
- */
-static void locks_delete_lock(struct file_lock **thisfl_p, unsigned int wait)
-{
-	struct file_lock *fl = *thisfl_p;
-
-	_unhash_lock(thisfl_p);
-	_delete_lock(fl, wait);
-}
-
-/*
- * Call back client filesystem in order to get it to unregister a lock,
- * then delete lock. Essentially useful only in locks_remove_*().
- * Note: this must be called with the semaphore already held!
- */
-static inline void locks_unlock_delete(struct file_lock **thisfl_p)
-{
-	struct file_lock *fl = *thisfl_p;
-	int (*lock)(struct file *, int, struct file_lock *);
-
-	_unhash_lock(thisfl_p);
-	if (fl->fl_file->f_op &&
-	    (lock = fl->fl_file->f_op->lock) != NULL) {
-		fl->fl_type = F_UNLCK;
-		lock(fl->fl_file, F_SETLK, fl);
-	}
-	_delete_lock(fl, 0);
-}
-
 /* Determine if lock sys_fl blocks lock caller_fl. Common functionality
  * checks for shared/exclusive status of overlapping locks.
  */
@@ -1665,33 +1626,24 @@
  */
 void locks_remove_posix(struct file *filp, fl_owner_t owner)
 {
-	struct inode * inode = filp->f_dentry->d_inode;
-	struct file_lock *fl;
-	struct file_lock **before;
-
-	/*
-	 * For POSIX locks we free all locks on this file for the given task.
-	 */
-	if (!inode->i_flock) {
-		/*
-		 * Notice that something might be grabbing a lock right now.
-		 * Consider it as a race won by us - event is async, so even if
-		 * we miss the lock added we can trivially consider it as added
-		 * after we went through this call.
-		 */
-		return;
-	}
-	lock_kernel();
-	before = &inode->i_flock;
-	while ((fl = *before) != NULL) {
-		if ((fl->fl_flags & FL_POSIX) && fl->fl_owner == owner) {
-			locks_unlock_delete(before);
-			before = &inode->i_flock;
-			continue;
-		}
-		before = &fl->fl_next;
+	struct file_lock lock;
+	
+	lock.fl_type = F_UNLCK;
+	lock.fl_flags = FL_POSIX;
+	lock.fl_start = 0;
+	lock.fl_end = OFFSET_MAX;
+	lock.fl_owner = owner;
+	lock.fl_pid = current->pid;
+	lock.fl_file = filp;
+	lock.fl_insert = NULL;
+	lock.fl_remove = NULL;
+	
+	if (filp->f_op && filp->f_op->lock != NULL) {
+		filp->f_op->lock(filp, F_SETLK, &lock);
+		/* Ignore any error -- we must remove the locks anyway */
 	}
-	unlock_kernel();
+
+	posix_lock_file(filp, &lock, 0);
 }
 
 /*

-- 
Revolutions do not require corporate support.