[PATCH v2 0/4] nfsd: don't let nfs4_file pin down the inode when it has no open state

[PATCH v2 0/4] nfsd: don't let nfs4_file pin down the inode when it has no open state

[Jeff Layton]

v2:
- eliminate the st_inode field from struct nfs4_ol_stateid

This is a port of the patches that Trond sent the other day onto the
current tip of Bruce's for-3.17 branch. It basically changes how
nfs4_file objects are hashed. Instead of using the inode pointer (and
pinning down an inode in the process), it uses the filehandle. This
allows us to avoid taking an inode reference directly for the nfs4_file.
With this, they're only taken by virtue of the files in the fi_fds
array.

Jeff Layton (1):
  nfsd: Do not let nfs4_file pin the struct inode

Trond Myklebust (3):
  nfsd: Store the filehandle with the struct nfs4_file
  nfsd: Use the filehandle to look up the struct nfs4_file instead of
    inode
  nfsd: nfs4_check_fh - make it actually check the filehandle

 fs/nfsd/nfs4state.c | 86 +++++++++++++++++++++++++++++++++--------------------
 fs/nfsd/state.h     |  2 +-
 2 files changed, 54 insertions(+), 34 deletions(-)

-- 
1.9.3

[PATCH v2 1/4] nfsd: Store the filehandle with the struct nfs4_file

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

For use when we may not have a struct inode.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 10 ++++++----
 fs/nfsd/state.h     |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 66a3b843a82b..859891f30958 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2833,7 +2833,8 @@ static struct nfs4_file *nfsd4_alloc_file(void)
 }
 
 /* OPEN Share state helper functions */
-static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
+static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino,
+		struct knfsd_fh *fh)
 {
 	unsigned int hashval = file_hashval(ino);
 
@@ -2845,6 +2846,7 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino)
 	INIT_LIST_HEAD(&fp->fi_delegations);
 	ihold(ino);
 	fp->fi_inode = ino;
+	fh_copy_shallow(&fp->fi_fhandle, fh);
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
 	fp->fi_share_deny = 0;
@@ -3049,14 +3051,14 @@ find_file(struct inode *ino)
 }
 
 static struct nfs4_file *
-find_or_add_file(struct inode *ino, struct nfs4_file *new)
+find_or_add_file(struct inode *ino, struct nfs4_file *new, struct knfsd_fh *fh)
 {
 	struct nfs4_file *fp;
 
 	spin_lock(&state_lock);
 	fp = find_file_locked(ino);
 	if (fp == NULL) {
-		nfsd4_init_file(new, ino);
+		nfsd4_init_file(new, ino, fh);
 		fp = new;
 	}
 	spin_unlock(&state_lock);
@@ -3711,7 +3713,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * and check for delegations in the process of being recalled.
 	 * If not found, create the nfs4_file struct
 	 */
-	fp = find_or_add_file(ino, open->op_file);
+	fp = find_or_add_file(ino, open->op_file, &current_fh->fh_handle);
 	if (fp != open->op_file) {
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e68a9ae30fd7..33cf950b3873 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -396,6 +396,7 @@ struct nfs4_file {
 	struct file		*fi_deleg_file;
 	struct file_lock	*fi_lease;
 	atomic_t		fi_delegees;
+	struct knfsd_fh		fi_fhandle;
 	struct inode		*fi_inode;
 	bool			fi_had_conflict;
 };
-- 
1.9.3

[PATCH v2 2/4] nfsd: Use the filehandle to look up the struct nfs4_file instead of inode

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

This makes more sense anyway since an inode pointer value can change
even when the filehandle doesn't.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 859891f30958..ab96718df3cc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -368,10 +368,22 @@ static unsigned int ownerstr_hashval(u32 clientid, struct xdr_netobj *ownername)
 #define FILE_HASH_BITS                   8
 #define FILE_HASH_SIZE                  (1 << FILE_HASH_BITS)
 
-static unsigned int file_hashval(struct inode *ino)
+static unsigned int nfsd_fh_hashval(struct knfsd_fh *fh)
 {
-	/* XXX: why are we hashing on inode pointer, anyway? */
-	return hash_ptr(ino, FILE_HASH_BITS);
+	return jhash2(fh->fh_base.fh_pad, XDR_QUADLEN(fh->fh_size), 0);
+}
+
+static unsigned int file_hashval(struct knfsd_fh *fh)
+{
+	return nfsd_fh_hashval(fh) & (FILE_HASH_SIZE - 1);
+}
+
+static bool nfsd_fh_match(struct knfsd_fh *fh1, struct knfsd_fh *fh2)
+{
+	return fh1->fh_size == fh2->fh_size &&
+		!memcmp(fh1->fh_base.fh_pad,
+				fh2->fh_base.fh_pad,
+				fh1->fh_size);
 }
 
 static struct hlist_head file_hashtbl[FILE_HASH_SIZE];
@@ -2836,7 +2848,7 @@ static struct nfs4_file *nfsd4_alloc_file(void)
 static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino,
 		struct knfsd_fh *fh)
 {
-	unsigned int hashval = file_hashval(ino);
+	unsigned int hashval = file_hashval(fh);
 
 	lockdep_assert_held(&state_lock);
 
@@ -3023,15 +3035,15 @@ find_openstateowner_str(unsigned int hashval, struct nfsd4_open *open,
 
 /* search file_hashtbl[] for file */
 static struct nfs4_file *
-find_file_locked(struct inode *ino)
+find_file_locked(struct knfsd_fh *fh)
 {
-	unsigned int hashval = file_hashval(ino);
+	unsigned int hashval = file_hashval(fh);
 	struct nfs4_file *fp;
 
 	lockdep_assert_held(&state_lock);
 
 	hlist_for_each_entry(fp, &file_hashtbl[hashval], fi_hash) {
-		if (fp->fi_inode == ino) {
+		if (nfsd_fh_match(&fp->fi_fhandle, fh)) {
 			get_nfs4_file(fp);
 			return fp;
 		}
@@ -3040,12 +3052,12 @@ find_file_locked(struct inode *ino)
 }
 
 static struct nfs4_file *
-find_file(struct inode *ino)
+find_file(struct knfsd_fh *fh)
 {
 	struct nfs4_file *fp;
 
 	spin_lock(&state_lock);
-	fp = find_file_locked(ino);
+	fp = find_file_locked(fh);
 	spin_unlock(&state_lock);
 	return fp;
 }
@@ -3056,7 +3068,7 @@ find_or_add_file(struct inode *ino, struct nfs4_file *new, struct knfsd_fh *fh)
 	struct nfs4_file *fp;
 
 	spin_lock(&state_lock);
-	fp = find_file_locked(ino);
+	fp = find_file_locked(fh);
 	if (fp == NULL) {
 		nfsd4_init_file(new, ino, fh);
 		fp = new;
@@ -3073,11 +3085,10 @@ find_or_add_file(struct inode *ino, struct nfs4_file *new, struct knfsd_fh *fh)
 static __be32
 nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type)
 {
-	struct inode *ino = current_fh->fh_dentry->d_inode;
 	struct nfs4_file *fp;
 	__be32 ret = nfs_ok;
 
-	fp = find_file(ino);
+	fp = find_file(&current_fh->fh_handle);
 	if (!fp)
 		return ret;
 	/* Check for conflicting share reservations */
-- 
1.9.3

[PATCH v2 3/4] nfsd: nfs4_check_fh - make it actually check the filehandle

[Jeff Layton]

From: Trond Myklebust <trond.myklebust@primarydata.com>

...instead of just checking the inode that corresponds to it.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ab96718df3cc..6ced8d566c0b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3951,7 +3951,7 @@ laundromat_main(struct work_struct *laundry)
 
 static inline __be32 nfs4_check_fh(struct svc_fh *fhp, struct nfs4_ol_stateid *stp)
 {
-	if (fhp->fh_dentry->d_inode != stp->st_file->fi_inode)
+	if (!nfsd_fh_match(&fhp->fh_handle, &stp->st_file->fi_fhandle))
 		return nfserr_bad_stateid;
 	return nfs_ok;
 }
-- 
1.9.3

[PATCH v2 4/4] nfsd: Do not let nfs4_file pin the struct inode

[Jeff Layton]

Remove the fi_inode field in struct nfs4_file in order to remove the
possibility of struct nfs4_file pinning the inode when it does not have
any open state.

The only place we still need to get to an inode is in check_for_locks,
so change it to use find_any_file and use the inode from any that it
finds. If it doesn't find one, then just assume there aren't any.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 49 ++++++++++++++++++++++++++++---------------------
 fs/nfsd/state.h     |  1 -
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6ced8d566c0b..1dfc8ee85c93 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -70,7 +70,7 @@ static u64 current_sessionid = 1;
 #define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
 
 /* forward declarations */
-static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
+static bool check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner);
 
 /* Locking: */
 
@@ -259,7 +259,6 @@ put_nfs4_file(struct nfs4_file *fi)
 	if (atomic_dec_and_lock(&fi->fi_ref, &state_lock)) {
 		hlist_del(&fi->fi_hash);
 		spin_unlock(&state_lock);
-		iput(fi->fi_inode);
 		nfsd4_free_file(fi);
 	}
 }
@@ -2845,8 +2844,7 @@ static struct nfs4_file *nfsd4_alloc_file(void)
 }
 
 /* OPEN Share state helper functions */
-static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino,
-		struct knfsd_fh *fh)
+static void nfsd4_init_file(struct nfs4_file *fp, struct knfsd_fh *fh)
 {
 	unsigned int hashval = file_hashval(fh);
 
@@ -2856,8 +2854,6 @@ static void nfsd4_init_file(struct nfs4_file *fp, struct inode *ino,
 	spin_lock_init(&fp->fi_lock);
 	INIT_LIST_HEAD(&fp->fi_stateids);
 	INIT_LIST_HEAD(&fp->fi_delegations);
-	ihold(ino);
-	fp->fi_inode = ino;
 	fh_copy_shallow(&fp->fi_fhandle, fh);
 	fp->fi_had_conflict = false;
 	fp->fi_lease = NULL;
@@ -3063,14 +3059,14 @@ find_file(struct knfsd_fh *fh)
 }
 
 static struct nfs4_file *
-find_or_add_file(struct inode *ino, struct nfs4_file *new, struct knfsd_fh *fh)
+find_or_add_file(struct nfs4_file *new, struct knfsd_fh *fh)
 {
 	struct nfs4_file *fp;
 
 	spin_lock(&state_lock);
 	fp = find_file_locked(fh);
 	if (fp == NULL) {
-		nfsd4_init_file(new, ino, fh);
+		nfsd4_init_file(new, fh);
 		fp = new;
 	}
 	spin_unlock(&state_lock);
@@ -3714,7 +3710,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	struct nfsd4_compoundres *resp = rqstp->rq_resp;
 	struct nfs4_client *cl = open->op_openowner->oo_owner.so_client;
 	struct nfs4_file *fp = NULL;
-	struct inode *ino = current_fh->fh_dentry->d_inode;
 	struct nfs4_ol_stateid *stp = NULL;
 	struct nfs4_delegation *dp = NULL;
 	__be32 status;
@@ -3724,7 +3719,7 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
 	 * and check for delegations in the process of being recalled.
 	 * If not found, create the nfs4_file struct
 	 */
-	fp = find_or_add_file(ino, open->op_file, &current_fh->fh_handle);
+	fp = find_or_add_file(open->op_file, &current_fh->fh_handle);
 	if (fp != open->op_file) {
 		status = nfs4_check_deleg(cl, open, &dp);
 		if (status)
@@ -4663,7 +4658,9 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp, str
 }
 
 static struct nfs4_ol_stateid *
-alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct nfs4_ol_stateid *open_stp)
+alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp,
+		struct inode *inode,
+		struct nfs4_ol_stateid *open_stp)
 {
 	struct nfs4_ol_stateid *stp;
 	struct nfs4_client *clp = lo->lo_owner.so_client;
@@ -4723,6 +4720,7 @@ static __be32 lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, s
 	struct nfs4_file *fi = ost->st_file;
 	struct nfs4_openowner *oo = openowner(ost->st_stateowner);
 	struct nfs4_client *cl = oo->oo_owner.so_client;
+	struct inode *inode = cstate->current_fh.fh_dentry->d_inode;
 	struct nfs4_lockowner *lo;
 	unsigned int strhashval;
 	struct nfsd_net *nn = net_generic(cl->net, nfsd_net_id);
@@ -4743,7 +4741,7 @@ static __be32 lookup_or_create_lock_state(struct nfsd4_compound_state *cstate, s
 
 	*lst = find_lock_stateid(lo, fi);
 	if (*lst == NULL) {
-		*lst = alloc_init_lock_stateid(lo, fi, ost);
+		*lst = alloc_init_lock_stateid(lo, fi, inode, ost);
 		if (*lst == NULL) {
 			release_lockowner_if_empty(lo);
 			return nfserr_jukebox;
@@ -5092,25 +5090,34 @@ out_nfserr:
 
 /*
  * returns
- * 	1: locks held by lockowner
- * 	0: no locks held by lockowner
+ * 	true:  locks held by lockowner
+ * 	false: no locks held by lockowner
  */
-static int
-check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner)
+static bool
+check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner)
 {
 	struct file_lock **flpp;
-	struct inode *inode = filp->fi_inode;
-	int status = 0;
+	int status = false;
+	struct file *filp = find_any_file(fp);
+	struct inode *inode;
+
+	if (!filp) {
+		/* Any valid lock stateid should have some sort of access */
+		WARN_ON_ONCE(1);
+		return status;
+	}
+
+	inode = file_inode(filp);
 
 	spin_lock(&inode->i_lock);
 	for (flpp = &inode->i_flock; *flpp != NULL; flpp = &(*flpp)->fl_next) {
 		if ((*flpp)->fl_owner == (fl_owner_t)lowner) {
-			status = 1;
-			goto out;
+			status = true;
+			break;
 		}
 	}
-out:
 	spin_unlock(&inode->i_lock);
+	fput(filp);
 	return status;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 33cf950b3873..0097d4771521 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -397,7 +397,6 @@ struct nfs4_file {
 	struct file_lock	*fi_lease;
 	atomic_t		fi_delegees;
 	struct knfsd_fh		fi_fhandle;
-	struct inode		*fi_inode;
 	bool			fi_had_conflict;
 };
 
-- 
1.9.3

Re: [PATCH v2 0/4] nfsd: don't let nfs4_file pin down the inode when it has no open state

[J. Bruce Fields]

On Wed, Jul 23, 2014 at 04:17:37PM -0400, Jeff Layton wrote:
> This is a port of the patches that Trond sent the other day onto the
> current tip of Bruce's for-3.17 branch. It basically changes how
> nfs4_file objects are hashed. Instead of using the inode pointer (and
> pinning down an inode in the process), it uses the filehandle. This
> allows us to avoid taking an inode reference directly for the nfs4_file.
> With this, they're only taken by virtue of the files in the fi_fds
> array.

Looks OK to me.  I'll give it a day in case Christoph or someone spots a
problem.

--b.

> 
> Jeff Layton (1):
>   nfsd: Do not let nfs4_file pin the struct inode
> 
> Trond Myklebust (3):
>   nfsd: Store the filehandle with the struct nfs4_file
>   nfsd: Use the filehandle to look up the struct nfs4_file instead of
>     inode
>   nfsd: nfs4_check_fh - make it actually check the filehandle
> 
>  fs/nfsd/nfs4state.c | 86 +++++++++++++++++++++++++++++++++--------------------
>  fs/nfsd/state.h     |  2 +-
>  2 files changed, 54 insertions(+), 34 deletions(-)
> 
> -- 
> 1.9.3
> 

Re: [PATCH v2 0/4] nfsd: don't let nfs4_file pin down the inode when it has no open state

[Christoph Hellwig]

On Wed, Jul 23, 2014 at 04:47:12PM -0400, J. Bruce Fields wrote:
> Looks OK to me.  I'll give it a day in case Christoph or someone spots a
> problem.

The series looks fine, but I've not done a detailed review.

Re: [PATCH v2 0/4] nfsd: don't let nfs4_file pin down the inode when it has no open state

[J. Bruce Fields]

On Thu, Jul 24, 2014 at 08:22:03AM -0700, Christoph Hellwig wrote:
> On Wed, Jul 23, 2014 at 04:47:12PM -0400, J. Bruce Fields wrote:
> > Looks OK to me.  I'll give it a day in case Christoph or someone spots a
> > problem.
> 
> The series looks fine, but I've not done a detailed review.

OK, applied and pushed out.

--b.