Skip to contentSkip to conversation

nfsd: don't let nfs4_file pin down the inode when it has no open state

PATCH In review Jeff Layton 3 participants 8 messages Jul 23, 2014 – Jul 25, 2014

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

Jeff Layton

author
#0 lore raw

v2 cover nfsd: don't let nfs4_file pin down the inode when it has no open state

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(-)
Signature · 1 line
1.9.3
author
#1 lore raw

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

From: Trond Myklebust <[email protected]>

For use when we may not have a struct inode.

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 10 ++++++----
 fs/nfsd/state.h     |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)
fs/nfsd/nfs4state.c +6 -4
index 66a3b843a82b..859891f30958 100644
@@ -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)
fs/nfsd/state.h +1 -0
index e68a9ae30fd7..33cf950b3873 100644
@@ -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;
 };
Signature · 1 line
1.9.3
author
#2 lore raw

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

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

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

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)
fs/nfsd/nfs4state.c +23 -12
index 859891f30958..ab96718df3cc 100644
@@ -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 */
Signature · 1 line
1.9.3
author
#3 lore raw

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

In reply to Jeff Layton

From: Trond Myklebust <[email protected]>

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

Signed-off-by Trond Myklebust <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/nfsd/nfs4state.c +1 -1
index ab96718df3cc..6ced8d566c0b 100644
@@ -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;
 }
Signature · 1 line
1.9.3
author
#4 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Jeff Layton <[email protected]>
---
 fs/nfsd/nfs4state.c | 49 ++++++++++++++++++++++++++++---------------------
 fs/nfsd/state.h     |  1 -
 2 files changed, 28 insertions(+), 22 deletions(-)
fs/nfsd/nfs4state.c +28 -21
index 6ced8d566c0b..1dfc8ee85c93 100644
@@ -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;
 }
fs/nfsd/state.h +0 -1
index 33cf950b3873..0097d4771521 100644
@@ -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;
 };
Signature · 1 line
1.9.3
#5 lore raw
In reply to Jeff Layton
Quoted 7 lines · Jeff Layton

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.

Quoted 17 lines

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(-)
Signature · 1 line
1.9.3
#6 lore raw
Quoted 2 lines · J. Bruce Fields

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.

#7 lore raw
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · J. Bruce Fields

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.