On Thu, Jul 05, 2012 at 06:42:51PM -0400, J. Bruce Fields wrote: > On Tue, Jun 26, 2012 at 09:50:48PM -0400, Filipe Brandenburger wrote: > > When calling fcntl(F_SETLEASE) for a second time on the same file descriptor, > > do_fcntl_add_lease will allocate and initialize a new file_lock to pass to > > __vfs_setlease. If that function decides to reuse the existing file_lock, it > > will free the newly allocated one to prevent leaking memory. > > > > However, the newly allocate file_lock was initialized with a valid file > > descriptor pointer and fl_lmops that contains a lm_release_private function, > > so calling locks_free_lock will trigger a call to lease_release_private_callback > > which will clear the fcntl(F_SETOWN) and fcntl(F_SETSIG) settings for the file > > descriptor even though the lease is not really being cleared at that point (as > > only the temporary file_lock is being freed.) > > > > This patch will fix this bug by calling kmem_cache_free directly instead of > > locks_free_lock if the file_lock will not be used. This will end up avoiding > > the call to lease_release_private_callback. > > > > This bug was tracked in kernel.org Bugzilla database: > > https://bugzilla.kernel.org/show_bug.cgi?id=43336 Apologies for not taking a closer look till now. Having done so.... I think the real problem is that we have this release callback at all. The fact is, this doesn't really have anything to do with releasing resources. And of the places we free a lease, I believe only one of them actually wants this. That callback was added without any visible justification by the patch below, pulled out of one of the historical git repos. It looks to me like we could just revert it. Could you try that? If it works, could you send in the result with proper changelog, etc.? --b. commit fd08d90925ac99d076382bcf4089085f92992954 Author: William A. Adamson <andros@xxxxxxxxxxxxxxxxxxx> Date: Tue Oct 19 18:44:22 2004 -0700 [PATCH] nfs4 lease: move the f_delown processing Move the f_delown processing from lease_modify() into a new default lock manager fl_release_private callback. Signed-off-by: Andy Adamson <andros@xxxxxxxxxxxxxx> Signed-off-by: Andrew Morton <akpm@xxxxxxxx> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxx> diff --git a/fs/locks.c b/fs/locks.c index e05dffc..1257539 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -397,8 +397,18 @@ static void lease_break_callback(struct file_lock *fl) kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG); } +static void lease_release_private_callback(struct file_lock *fl) +{ + if (!fl->fl_file) + return; + + f_delown(fl->fl_file); + fl->fl_file->f_owner.signum = 0; +} + struct lock_manager_operations lease_manager_ops = { .fl_break = lease_break_callback, + .fl_release_private = lease_release_private_callback, }; /* @@ -1056,13 +1066,8 @@ static int lease_modify(struct file_lock **before, int arg) if (error) return error; locks_wake_up_blocks(fl); - if (arg == F_UNLCK) { - struct file *filp = fl->fl_file; - - f_delown(filp); - filp->f_owner.signum = 0; + if (arg == F_UNLCK) locks_delete_lock(before); - } return 0; } -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
