Return-Path: Received: from fieldses.org ([174.143.236.118]:55165 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752013Ab0J0Ozq (ORCPT ); Wed, 27 Oct 2010 10:55:46 -0400 Date: Wed, 27 Oct 2010 10:55:39 -0400 From: "J. Bruce Fields" To: Arnd Bergmann Cc: Christoph Hellwig , Linus Torvalds , Bryan Schumaker , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: nfsd changes for 2.6.37 Message-ID: <20101027145538.GC6328@fieldses.org> References: <20101026164549.GD19445@fieldses.org> <20101027083924.GA28129@infradead.org> <20101027133924.GB6328@fieldses.org> <201010271546.09036.arnd@arndb.de> Content-Type: text/plain; charset=us-ascii In-Reply-To: <201010271546.09036.arnd@arndb.de> Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 On Wed, Oct 27, 2010 at 03:46:08PM +0200, Arnd Bergmann wrote: > On Wednesday 27 October 2010, J. Bruce Fields wrote: > > On Wed, Oct 27, 2010 at 04:39:24AM -0400, Christoph Hellwig wrote: > > > Do locks_alloc_lock and initialization of the heap struct file_lock > > > in the caller. This also avoids an entirely useless copy of the > > > lock structure. free the passed in structure if we are modifying > > > an existing lock structure. > > > > That sounds good; I'll give it a try. > > I've already started, this is what I've come up with. I don't have > a good way to test it though, and don't know as much as you about this > code. Feel free to use it as a starting point if you like. > It does look correct to me, but I'm not very confident in this. Hm, two problems: - We introduce the possibility of fcntl(fd, F_SETLEASE, F_UNLCK) failing with ENOMEM. - fasync_helper(.,.,1,.) sleeps. Argh. --b. > > Signed-off-by: Arnd Bergmann > --- > > diff --git a/fs/locks.c b/fs/locks.c > index 8b2b6ad..c5ec771 100644 > --- a/fs/locks.c > +++ b/fs/locks.c > @@ -162,10 +162,11 @@ EXPORT_SYMBOL_GPL(unlock_flocks); > static struct kmem_cache *filelock_cache __read_mostly; > > /* Allocate an empty lock structure. */ > -static struct file_lock *locks_alloc_lock(void) > +struct file_lock *locks_alloc_lock(void) > { > return kmem_cache_alloc(filelock_cache, GFP_KERNEL); > } > +EXPORT_SYMBOL_GPL(locks_alloc_lock); > > void locks_release_private(struct file_lock *fl) > { > @@ -1365,7 +1366,6 @@ int fcntl_getlease(struct file *filp) > int generic_setlease(struct file *filp, long arg, struct file_lock **flp) > { > struct file_lock *fl, **before, **my_before = NULL, *lease; > - struct file_lock *new_fl = NULL; > struct dentry *dentry = filp->f_path.dentry; > struct inode *inode = dentry->d_inode; > int error, rdlease_count = 0, wrlease_count = 0; > @@ -1385,11 +1385,6 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp) > lease = *flp; > > if (arg != F_UNLCK) { > - error = -ENOMEM; > - new_fl = locks_alloc_lock(); > - if (new_fl == NULL) > - goto out; > - > error = -EAGAIN; > if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0)) > goto out; > @@ -1434,23 +1429,19 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp) > goto out; > } > > - error = 0; > if (arg == F_UNLCK) > - goto out; > + goto out_unlck; > > error = -EINVAL; > if (!leases_enable) > goto out; > > - locks_copy_lock(new_fl, lease); > - locks_insert_lock(before, new_fl); > - > - *flp = new_fl; > + locks_insert_lock(before, lease); > +out_unlck: > return 0; > > out: > - if (new_fl != NULL) > - locks_free_lock(new_fl); > + locks_free_lock(lease); > return error; > } > EXPORT_SYMBOL(generic_setlease); > @@ -1514,26 +1505,31 @@ EXPORT_SYMBOL_GPL(vfs_setlease); > */ > int fcntl_setlease(unsigned int fd, struct file *filp, long arg) > { > - struct file_lock fl, *flp = &fl; > + struct file_lock *fl; > struct inode *inode = filp->f_path.dentry->d_inode; > int error; > > - locks_init_lock(&fl); > - error = lease_init(filp, arg, &fl); > - if (error) > + fl = locks_alloc_lock(); > + if (!fl) > + return -ENOMEM; > + > + locks_init_lock(fl); > + error = lease_init(filp, arg, fl); > + if (error) { > + locks_free_lock(fl); > return error; > + } > > lock_flocks(); > - > - error = __vfs_setlease(filp, arg, &flp); > + error = __vfs_setlease(filp, arg, &fl); > if (error || arg == F_UNLCK) > goto out_unlock; > > - error = fasync_helper(fd, filp, 1, &flp->fl_fasync); > + error = fasync_helper(fd, filp, 1, &fl->fl_fasync); > if (error < 0) { > /* remove lease just inserted by setlease */ > - flp->fl_type = F_UNLCK | F_INPROGRESS; > - flp->fl_break_time = jiffies - 10; > + fl->fl_type = F_UNLCK | F_INPROGRESS; > + fl->fl_break_time = jiffies - 10; > time_out_leases(inode); > goto out_unlock; > } > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 9019e8e..56347e0 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2614,7 +2614,7 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta > struct nfs4_delegation *dp; > struct nfs4_stateowner *sop = stp->st_stateowner; > int cb_up = atomic_read(&sop->so_client->cl_cb_set); > - struct file_lock fl, *flp = &fl; > + struct file_lock *fl; > int status, flag = 0; > > flag = NFS4_OPEN_DELEGATE_NONE; > @@ -2648,20 +2648,24 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open, struct nfs4_sta > flag = NFS4_OPEN_DELEGATE_NONE; > goto out; > } > - locks_init_lock(&fl); > - fl.fl_lmops = &nfsd_lease_mng_ops; > - fl.fl_flags = FL_LEASE; > - fl.fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK; > - fl.fl_end = OFFSET_MAX; > - fl.fl_owner = (fl_owner_t)dp; > - fl.fl_file = find_readable_file(stp->st_file); > - BUG_ON(!fl.fl_file); > - fl.fl_pid = current->tgid; > + status = -ENOMEM; > + fl = locks_alloc_lock(); > + if (!fl) > + goto out; > + locks_init_lock(fl); > + fl->fl_lmops = &nfsd_lease_mng_ops; > + fl->fl_flags = FL_LEASE; > + fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK; > + fl->fl_end = OFFSET_MAX; > + fl->fl_owner = (fl_owner_t)dp; > + fl->fl_file = find_readable_file(stp->st_file); > + BUG_ON(!fl->fl_file); > + fl->fl_pid = current->tgid; > > /* vfs_setlease checks to see if delegation should be handed out. > * the lock_manager callbacks fl_mylease and fl_change are used > */ > - if ((status = vfs_setlease(fl.fl_file, fl.fl_type, &flp))) { > + if ((status = vfs_setlease(fl->fl_file, fl->fl_type, &fl))) { > dprintk("NFSD: setlease failed [%d], no delegation\n", status); > unhash_delegation(dp); > flag = NFS4_OPEN_DELEGATE_NONE;