| Message ID | 1408804878-1331-9-git-send-email-jlayton@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> Some comments on further work I'd like to see in this area, though: > + spin_lock(&inode->i_lock); > + time_out_leases(inode); > for (before = &inode->i_flock; > ((fl = *before) != NULL) && IS_LEASE(fl); > before = &fl->fl_next) { > if (fl->fl_file != filp) > continue; > - return fl->fl_lmops->lm_change(before, F_UNLCK); > + error = fl->fl_lmops->lm_change(before, F_UNLCK); > } We really should split a lm_release from lm_change, the way it is used is highly confusing. In addition I think a lot of code currently in lease_modify should move here instead, e.g. something like: if (fl->fl_file != filp) continue; fl = *before; fl->fl_type = F_UNLCK; lease_clear_pending(fl, F_UNLCK); locks_wake_up_blocks(fl); if (fl->fl_lmops->lm_delete) fl->fl_lmops->lm_delete(fl); locks_delete_lock(before, NULL); with lm_delete for user space leases as: static void lease_delete(struct file_lock *fl) { struct file *filp = fl->fl_file; f_delown(filp); filp->f_owner.signum = 0; 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); fl->fl_fasync = NULL; } } and a NULL implementation for delegations. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Aug 24, 2014 at 09:06:34AM -0700, Christoph Hellwig wrote: > We really should split a lm_release from lm_change, the way it is > used is highly confusing. In addition I think a lot of code > currently in lease_modify should move here instead, e.g. something like: At this point the old lm_change actually becomes superflous if we simply disallow changes for delegation. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 24 Aug 2014 09:06:34 -0700 Christoph Hellwig <hch@infradead.org> wrote: > Looks good, > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > Some comments on further work I'd like to see in this area, though: > > > + spin_lock(&inode->i_lock); > > + time_out_leases(inode); > > for (before = &inode->i_flock; > > ((fl = *before) != NULL) && IS_LEASE(fl); > > before = &fl->fl_next) { > > if (fl->fl_file != filp) > > continue; > > - return fl->fl_lmops->lm_change(before, F_UNLCK); > > + error = fl->fl_lmops->lm_change(before, F_UNLCK); > > } > > We really should split a lm_release from lm_change, the way it is > used is highly confusing. In addition I think a lot of code > currently in lease_modify should move here instead, e.g. something like: > > > if (fl->fl_file != filp) > continue; > > fl = *before; > fl->fl_type = F_UNLCK; > lease_clear_pending(fl, F_UNLCK); > locks_wake_up_blocks(fl); > if (fl->fl_lmops->lm_delete) > fl->fl_lmops->lm_delete(fl); > locks_delete_lock(before, NULL); > > with lm_delete for user space leases as: > > static void lease_delete(struct file_lock *fl) > { > struct file *filp = fl->fl_file; > > f_delown(filp); > filp->f_owner.signum = 0; > 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); > fl->fl_fasync = NULL; > } > } > > and a NULL implementation for delegations. Good idea. I'll spin that up on the next iteration. Thanks,
On Sun, 24 Aug 2014 09:11:34 -0700 Christoph Hellwig <hch@infradead.org> wrote: > On Sun, Aug 24, 2014 at 09:06:34AM -0700, Christoph Hellwig wrote: > > We really should split a lm_release from lm_change, the way it is > > used is highly confusing. In addition I think a lot of code > > currently in lease_modify should move here instead, e.g. something like: > > At this point the old lm_change actually becomes superflous if we > simply disallow changes for delegation. > This sounds like a reasonable change, but I think it'll be easier to implement a little later once we've done some cleanup and change how file locks are handled. I'm going to leave this out of this current patchset, but I'll keep it in mind for a later cleanup.
diff --git a/fs/locks.c b/fs/locks.c index b35b706c05fe..49210d5cbf41 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1335,6 +1335,8 @@ static void time_out_leases(struct inode *inode) struct file_lock **before; struct file_lock *fl; + lockdep_assert_held(&inode->i_lock); + before = &inode->i_flock; while ((fl = *before) && IS_LEASE(fl) && lease_breaking(fl)) { trace_time_out_leases(inode, fl); @@ -1595,6 +1597,9 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au return -EINVAL; } + spin_lock(&inode->i_lock); + time_out_leases(inode); + error = check_conflicting_open(dentry, arg); if (error) goto out; @@ -1660,6 +1665,7 @@ out_setup: if (lease->fl_lmops->lm_setup) lease->fl_lmops->lm_setup(lease, aux); out: + spin_unlock(&inode->i_lock); if (is_deleg) mutex_unlock(&inode->i_mutex); if (!error && !my_before) @@ -1672,18 +1678,22 @@ out_unlink: static int generic_delete_lease(struct file *filp) { + int error = -EAGAIN; struct file_lock *fl, **before; struct dentry *dentry = filp->f_path.dentry; struct inode *inode = dentry->d_inode; + spin_lock(&inode->i_lock); + time_out_leases(inode); for (before = &inode->i_flock; ((fl = *before) != NULL) && IS_LEASE(fl); before = &fl->fl_next) { if (fl->fl_file != filp) continue; - return fl->fl_lmops->lm_change(before, F_UNLCK); + error = fl->fl_lmops->lm_change(before, F_UNLCK); } - return -EAGAIN; + spin_unlock(&inode->i_lock); + return error; } /** @@ -1702,8 +1712,7 @@ static int generic_delete_lease(struct file *filp) int generic_setlease(struct file *filp, long arg, struct file_lock **flp, void **aux) { - struct dentry *dentry = filp->f_path.dentry; - struct inode *inode = dentry->d_inode; + struct inode *inode = file_inode(filp); int error; if ((!uid_eq(current_fsuid(), inode->i_uid)) && !capable(CAP_LEASE)) @@ -1714,8 +1723,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp, if (error) return error; - time_out_leases(inode); - switch (arg) { case F_UNLCK: return generic_delete_lease(filp); @@ -1759,16 +1766,10 @@ EXPORT_SYMBOL(generic_setlease); int vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux) { - struct inode *inode = file_inode(filp); - int error; - - spin_lock(&inode->i_lock); if (filp->f_op->setlease) - error = filp->f_op->setlease(filp, arg, lease, aux); + return filp->f_op->setlease(filp, arg, lease, aux); else - error = generic_setlease(filp, arg, lease, aux); - spin_unlock(&inode->i_lock); - return error; + return generic_setlease(filp, arg, lease, aux); } EXPORT_SYMBOL_GPL(vfs_setlease);
Now that we have a saner internal API for managing leases, we no longer need to mandate that the inode->i_lock be held over most of the lease code. Push it down into generic_add_lease and generic_delete_lease. With this change ->setlease calls can now block, which is a necessary (but not sufficient!) step toward allowing leases to work with distributed filesystems. Signed-off-by: Jeff Layton <jlayton@primarydata.com> --- fs/locks.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)