Return-Path: Received: from fieldses.org ([174.143.236.118]:50548 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754012Ab0KCUmF (ORCPT ); Wed, 3 Nov 2010 16:42:05 -0400 Date: Wed, 3 Nov 2010 16:41:48 -0400 From: "J. Bruce Fields" To: Christoph Hellwig Cc: Arnd Bergmann , Linus Torvalds , Bryan Schumaker , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] locks: let the caller free file_lock on ->setlease failure Message-ID: <20101103204147.GA6777@fieldses.org> References: <20101026164549.GD19445@fieldses.org> <20101027083924.GA28129@infradead.org> <20101027133924.GB6328@fieldses.org> <201010271546.09036.arnd@arndb.de> <20101027145538.GC6328@fieldses.org> <20101027145929.GA5788@infradead.org> <20101030212500.GE480@fieldses.org> <20101031123419.GA4491@infradead.org> <20101031123510.GA4520@infradead.org> Content-Type: text/plain; charset=us-ascii In-Reply-To: <20101031123510.GA4520@infradead.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Sun, Oct 31, 2010 at 08:35:10AM -0400, Christoph Hellwig wrote: > The caller allocated it, the caller should free it. The only issue so > far is that we could change the flp pointer even on an error return if > the fl_change callback failed. But we can simply move the flp assignment > after the fl_change invocation, as the callers don't care about the > flp return value if the setlease call failed. > > Signed-off-by: Christoph Hellwig > > Index: linux-2.6/fs/cifs/cifsfs.c > =================================================================== > --- linux-2.6.orig/fs/cifs/cifsfs.c 2010-10-31 07:10:07.636004223 -0400 > +++ linux-2.6/fs/cifs/cifsfs.c 2010-10-31 07:10:10.922004154 -0400 > @@ -625,11 +625,8 @@ static int cifs_setlease(struct file *fi > knows that the file won't be changed on the server > by anyone else */ > return generic_setlease(file, arg, lease); > - else { > - if (arg != F_UNLCK) > - locks_free_lock(*lease); > + else > return -EAGAIN; > - } > } > > struct file_system_type cifs_fs_type = { > Index: linux-2.6/fs/gfs2/file.c > =================================================================== > --- linux-2.6.orig/fs/gfs2/file.c 2010-10-31 07:10:07.643004363 -0400 > +++ linux-2.6/fs/gfs2/file.c 2010-10-31 07:10:10.923003665 -0400 > @@ -629,8 +629,6 @@ static ssize_t gfs2_file_aio_write(struc > > static int gfs2_setlease(struct file *file, long arg, struct file_lock **fl) > { > - if (arg != F_UNLCK) > - locks_free_lock(*fl); > return -EINVAL; > } > > Index: linux-2.6/fs/locks.c > =================================================================== > --- linux-2.6.orig/fs/locks.c 2010-10-31 07:10:07.649004084 -0400 > +++ linux-2.6/fs/locks.c 2010-10-31 07:34:10.102255587 -0400 > @@ -1428,8 +1425,9 @@ int generic_setlease(struct file *filp, > goto out; > > if (my_before != NULL) { > - *flp = *my_before; > error = lease->fl_lmops->fl_change(my_before, arg); > + if (!error) > + *flp = *my_before; Argh, missed this: we're leaking the passed-in lease in this case. --b. > goto out; > } > > @@ -1444,8 +1442,6 @@ int generic_setlease(struct file *filp, > return 0; > > out: > - if (arg != F_UNLCK) > - locks_free_lock(lease); > return error; > } > EXPORT_SYMBOL(generic_setlease); > @@ -1524,8 +1520,11 @@ static int do_fcntl_add_lease(unsigned i > } > lock_flocks(); > error = __vfs_setlease(filp, arg, &fl); > - if (error) > - goto out_unlock; > + if (error) { > + unlock_flocks(); > + locks_free_lock(fl); > + goto out_free_fasync; > + } > > /* > * fasync_insert_entry() returns the old entry if any. > @@ -1541,12 +1540,12 @@ static int do_fcntl_add_lease(unsigned i > fl->fl_type = F_UNLCK | F_INPROGRESS; > fl->fl_break_time = jiffies - 10; > time_out_leases(inode); > - goto out_unlock; > + } else { > + error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); > } > - > - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); > -out_unlock: > unlock_flocks(); > + > +out_free_fasync: > if (new) > fasync_free(new); > return error; > Index: linux-2.6/fs/nfs/file.c > =================================================================== > --- linux-2.6.orig/fs/nfs/file.c 2010-10-31 07:10:07.658003804 -0400 > +++ linux-2.6/fs/nfs/file.c 2010-10-31 07:10:10.936003734 -0400 > @@ -884,7 +884,5 @@ static int nfs_setlease(struct file *fil > dprintk("NFS: setlease(%s/%s, arg=%ld)\n", > file->f_path.dentry->d_parent->d_name.name, > file->f_path.dentry->d_name.name, arg); > - if (arg != F_UNLCK) > - locks_free_lock(*fl); > return -EINVAL; > } > Index: linux-2.6/fs/nfsd/nfs4state.c > =================================================================== > --- linux-2.6.orig/fs/nfsd/nfs4state.c 2010-10-31 07:10:07.666004084 -0400 > +++ linux-2.6/fs/nfsd/nfs4state.c 2010-10-31 07:32:56.906254608 -0400 > @@ -2652,6 +2652,7 @@ nfs4_open_delegation(struct svc_fh *fh, > if ((status = vfs_setlease(fl->fl_file, fl->fl_type, &fl))) { > dprintk("NFSD: setlease failed [%d], no delegation\n", status); > dp->dl_flock = NULL; > + locks_free_lock(fl); > unhash_delegation(dp); > flag = NFS4_OPEN_DELEGATE_NONE; > goto out;