From patchwork Sat Aug 23 14:41:15 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 4769551 Return-Path: X-Original-To: patchwork-cifs-client@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 4B26CC0338 for ; Sat, 23 Aug 2014 14:41:52 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 3701C2018A for ; Sat, 23 Aug 2014 14:41:51 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id F031B201C0 for ; Sat, 23 Aug 2014 14:41:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751795AbaHWOlk (ORCPT ); Sat, 23 Aug 2014 10:41:40 -0400 Received: from mail-qa0-f49.google.com ([209.85.216.49]:64540 "EHLO mail-qa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751353AbaHWOli (ORCPT ); Sat, 23 Aug 2014 10:41:38 -0400 Received: by mail-qa0-f49.google.com with SMTP id dc16so10782298qab.36 for ; Sat, 23 Aug 2014 07:41:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id :in-reply-to:references; bh=q3sd2igKPJVh6fMGNGgSfGUJq18LHxC4gEmMB0HOF+U=; b=YhgwI2d3Mi5cCk6u2B68NS7KYU4IMkOH99az8Xt3cF96zZkx/JIm/8wn+igHrOTffy BKApSKFVZrf4knA2U/Q50Rh97MSUfbucgvdiP1xoGFGNUJiR7hXRxls3t70iDN/hr4UA BETviC1Qsjp2rZtwS+2YnDxiV6wtYkZ9osVxD90LBjZv6pPs945vyxBeR13SwbwObIfA kSe47Ow/q446Rlv5ZF3sP5+C++CPogEFAlvK1dIxppneYT6FIrGcc0N6QlRC4oSxfkAI Zu6nduOJzmBvKvtTRj9yjlzi16pZzPpeKnL47zwWgzoV+3N2ie4iPYr72phMIq6KPxcg 9U1g== X-Gm-Message-State: ALoCoQmC+6ggiWJ37Kj6PnAt+MIh7P7KLNITwj37Pd7OAeynoe38a8MzVg/Td1+kyVH5SO9Pgp8p X-Received: by 10.224.19.138 with SMTP id a10mr17772552qab.98.1408804898201; Sat, 23 Aug 2014 07:41:38 -0700 (PDT) Received: from tlielax.poochiereds.net ([2001:470:8:d63:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id o3sm64529771qab.21.2014.08.23.07.41.36 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 23 Aug 2014 07:41:37 -0700 (PDT) From: Jeff Layton To: linux-fsdevel@vger.kernel.org Cc: bfields@fieldses.org, hch@infradead.org, cluster-devel@redhat.com, linux-cifs@vger.kernel.org, linux-nfs@vger.kernel.org Subject: [PATCH 07/10] locks: define a lm_setup handler for leases Date: Sat, 23 Aug 2014 10:41:15 -0400 Message-Id: <1408804878-1331-8-git-send-email-jlayton@primarydata.com> X-Mailer: git-send-email 1.9.3 In-Reply-To: <1408804878-1331-1-git-send-email-jlayton@primarydata.com> References: <1408804878-1331-1-git-send-email-jlayton@primarydata.com> Sender: linux-cifs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-cifs@vger.kernel.org X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP ...and call it when setting up a new lease or modifying an existing one. Add a lm_setup handler for fcntl leases and move the fasync setup into it. At the same time, change the semantics of how the file_lock double pointer is handled. Up until now on a successful lease return, you get back a pointer to the lock on the inode's i_flock list. This is bad, since that pointer can't be relied on as valid once the inode->i_lock has been released. Leases also don't carry a lot of information, so tracking a pointer to it isn't terribly helpful anyway. Change the code to instead just zero out the pointer if the lease we passed in ended up being used. Then the callers can just check to see if it's NULL after the call and free it if it isn't. The new aux argument for lm_setup has the same semantics. The lm_setup function can zero the pointer out to signal to the caller that it should not be freed after the function returns. Signed-off-by: Jeff Layton --- fs/locks.c | 96 +++++++++++++++++++++++++++++------------------------ fs/nfsd/nfs4state.c | 9 ++--- include/linux/fs.h | 1 + 3 files changed, 56 insertions(+), 50 deletions(-) diff --git a/fs/locks.c b/fs/locks.c index 906e09da115a..b35b706c05fe 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -432,9 +432,32 @@ static void lease_break_callback(struct file_lock *fl) kill_fasync(&fl->fl_fasync, SIGIO, POLL_MSG); } +static void +lease_setup(struct file_lock *fl, void **aux) +{ + struct file *filp = fl->fl_file; + struct fasync_struct *fa = *aux; + + /* + * fasync_insert_entry() returns the old entry if any. If there was no + * old entry, then it used "aux" and inserted it into the fasync list. + * Clear the pointer to indicate that it shouldn't be freed. + */ + if (!fasync_insert_entry(fa->fa_fd, filp, &fl->fl_fasync, fa)) + *aux = NULL; + + /* + * Despite the fact that it's an int return function, __f_setown never + * returns an error. Just ignore any error return here, but spew a + * WARN_ON_ONCE in case this ever changes. + */ + WARN_ON_ONCE(__f_setown(filp, task_pid(current), PIDTYPE_PID, 0)); +} + static const struct lock_manager_operations lease_manager_ops = { .lm_break = lease_break_callback, .lm_change = lease_modify, + .lm_setup = lease_setup, }; /* @@ -1609,9 +1632,9 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au if (my_before != NULL) { lease = *my_before; error = lease->fl_lmops->lm_change(my_before, arg); - if (!error) - *flp = *my_before; - goto out; + if (error) + goto out; + goto out_setup; } error = -EINVAL; @@ -1632,9 +1655,15 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **au error = check_conflicting_open(dentry, arg); if (error) goto out_unlink; + +out_setup: + if (lease->fl_lmops->lm_setup) + lease->fl_lmops->lm_setup(lease, aux); out: if (is_deleg) mutex_unlock(&inode->i_mutex); + if (!error && !my_before) + *flp = NULL; return error; out_unlink: locks_unlink_lock(before); @@ -1659,10 +1688,11 @@ static int generic_delete_lease(struct file *filp) /** * generic_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @flp: input - file_lock to use, output - file_lock inserted - * @aux: auxillary data for lm_setup + * @filp: file pointer + * @arg: type of lease to obtain + * @flp: input - file_lock to use, output - file_lock inserted + * @aux: auxillary data for lm_setup (may be NULL if lm_setup + * doesn't require it) * * The (input) flp->fl_lmops->lm_break function is required * by break_lease(). @@ -1702,21 +1732,13 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp, } EXPORT_SYMBOL(generic_setlease); -static int -__vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux) -{ - if (filp->f_op->setlease) - return filp->f_op->setlease(filp, arg, lease, aux); - else - return generic_setlease(filp, arg, lease, aux); -} - /** * vfs_setlease - sets a lease on an open file - * @filp: file pointer - * @arg: type of lease to obtain - * @lease: file_lock to use when adding a lease - * @aux: auxillary info for lm_setup when adding a lease + * @filp: file pointer + * @arg: type of lease to obtain + * @lease: file_lock to use when adding a lease + * @aux: auxillary info for lm_setup when adding a lease (may be + * NULL if lm_setup doesn't require it) * * Call this to establish a lease on the file. The "lease" argument is not * used for F_UNLCK requests and may be NULL. For commands that set or alter @@ -1730,8 +1752,10 @@ __vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux * where fcntl_getlease() can find it. Since fcntl_getlease() only reports * whether the current task holds a lease, a cluster filesystem need only do * this for leases held by processes on this node. + * + * The "aux" pointer is passed directly to the lm_setup function as-is. It + * may be NULL if the lm_setup operation doesn't require it. */ - int vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux) { @@ -1739,17 +1763,18 @@ vfs_setlease(struct file *filp, long arg, struct file_lock **lease, void **aux) int error; spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, lease, aux); + if (filp->f_op->setlease) + error = filp->f_op->setlease(filp, arg, lease, aux); + else + error = generic_setlease(filp, arg, lease, aux); spin_unlock(&inode->i_lock); - return error; } EXPORT_SYMBOL_GPL(vfs_setlease); static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) { - struct file_lock *fl, *ret; - struct inode *inode = file_inode(filp); + struct file_lock *fl; struct fasync_struct *new; int error; @@ -1762,26 +1787,9 @@ static int do_fcntl_add_lease(unsigned int fd, struct file *filp, long arg) locks_free_lock(fl); return -ENOMEM; } - ret = fl; - spin_lock(&inode->i_lock); - error = __vfs_setlease(filp, arg, &ret, NULL); - if (error) - goto out_unlock; - if (ret == fl) - fl = NULL; + new->fa_fd = fd; - /* - * fasync_insert_entry() returns the old entry if any. - * If there was no old entry, then it used 'new' and - * inserted it into the fasync list. Clear new so that - * we don't release it here. - */ - if (!fasync_insert_entry(fd, filp, &ret->fl_fasync, new)) - new = NULL; - - error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0); -out_unlock: - spin_unlock(&inode->i_lock); + error = vfs_setlease(filp, arg, &fl, (void **)&new); if (fl) locks_free_lock(fl); if (new) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index d964a41c9151..6af5d0890373 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -3773,7 +3773,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_file *fp, int flag) static int nfs4_setlease(struct nfs4_delegation *dp) { struct nfs4_file *fp = dp->dl_stid.sc_file; - struct file_lock *fl, *ret; + struct file_lock *fl; struct file *filp; int status = 0; @@ -3787,14 +3787,11 @@ static int nfs4_setlease(struct nfs4_delegation *dp) return -EBADF; } fl->fl_file = filp; - ret = fl; status = vfs_setlease(filp, fl->fl_type, &fl, NULL); - if (status) { + if (fl) locks_free_lock(fl); + if (status) goto out_fput; - } - if (ret != fl) - locks_free_lock(fl); spin_lock(&state_lock); spin_lock(&fp->fi_lock); /* Did the lease get broken before we took the lock? */ diff --git a/include/linux/fs.h b/include/linux/fs.h index 2668d054147f..70d22c12924f 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -874,6 +874,7 @@ struct lock_manager_operations { int (*lm_grant)(struct file_lock *, int); void (*lm_break)(struct file_lock *); int (*lm_change)(struct file_lock **, int); + void (*lm_setup)(struct file_lock *, void **); }; struct lock_manager {