From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthew Wilcox Subject: Re: [PATCH] problem with locks_remove_posix calling filesystem lock operation Date: Sat, 25 May 2002 04:14:15 +0100 Sender: linux-fsdevel-owner@vger.kernel.org Message-ID: <20020525041415.B10366@parcelfarce.linux.theplanet.co.uk> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: matthew@wil.cx, linux-fsdevel@vger.kernel.org Return-path: To: Brian Dixon Content-Disposition: inline In-Reply-To: ; from dixonbp@us.ibm.com on Fri, May 24, 2002 at 02:16:25PM -0500 List-Id: linux-fsdevel.vger.kernel.org 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.