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

From: Trond Myklebust <[email protected]>

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.

Add a field to struct nfs4_ol_stateid, so that the lock stateid may continue to check for existing lock state before being released.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 31 +++++++++++++++----------------
 fs/nfsd/state.h     |  2 +-
 2 files changed, 16 insertions(+), 17 deletions(-)

fs/nfsd/nfs4state.c +9 -12

index 4c9404500a8e..9b5775f2af57 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 int check_for_locks(struct inode *inode, 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;

@@ -3065,14 +3061,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);

@@ -3716,7 +3712,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;

@@ -3726,7 +3721,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)

@@ -4206,7 +4201,7 @@ nfsd4_free_lock_stateid(struct nfs4_ol_stateid *stp)
 {
 	struct nfs4_lockowner *lo = lockowner(stp->st_stateowner);
 
-	if (check_for_locks(stp->st_file, lo))
+	if (check_for_locks(stp->st_inode, lo))
 		return nfserr_locks_held;
 	release_lockowner_if_empty(lo);
 	return nfs_ok;

@@ -4665,7 +4660,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;

fs/nfsd/nfs4state.c +5 -4

+	stp->st_inode = inode;
 	stp->st_file = fp;
 	stp->st_access_bmap = 0;
 	stp->st_deny_bmap = open_stp->st_deny_bmap;

@@ -4725,6 +4724,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);

@@ -4745,7 +4745,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;

@@ -5098,10 +5098,9 @@ out_nfserr:
  * 	0: no locks held by lockowner
  */
 static int
-check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner)
+check_for_locks(struct inode *inode, struct nfs4_lockowner *lowner)
 {
 	struct file_lock **flpp;
-	struct inode *inode = filp->fi_inode;
 	int status = 0;
 
 	spin_lock(&inode->i_lock);

@@ -5160,7 +5159,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 	lo = lockowner(sop);
 	/* see if there are still any locks associated with it */
 	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
-		if (check_for_locks(stp->st_file, lo))
+		if (check_for_locks(stp->st_inode, lo))
 			goto out;
 	}
 

fs/nfsd/state.h +1 -1

index 33cf950b3873..ba711c66561e 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;
 };
 

@@ -412,6 +411,7 @@ struct nfs4_ol_stateid {
 	unsigned char                 st_access_bmap;
 	unsigned char                 st_deny_bmap;
 	struct nfs4_ol_stateid         * st_openstp;
+	struct inode		    * st_inode;
 };
 
 static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)

Signature · 1 line

1.9.3

Quoted 111 lines · Jeff Layton

From: Trond Myklebust <[email protected]>

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.

Add a field to struct nfs4_ol_stateid, so that the lock stateid may continue to check for existing lock state before being released.

Signed-off-by Trond Myklebust <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/nfsd/nfs4state.c | 31 +++++++++++++++----------------
 fs/nfsd/state.h     |  2 +-
 2 files changed, 16 insertions(+), 17 deletions(-)

fs/nfsd/nfs4state.c +9 -12

index 4c9404500a8e..9b5775f2af57 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 int check_for_locks(struct inode *inode, 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;

@@ -3065,14 +3061,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);

@@ -3716,7 +3712,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;

@@ -3726,7 +3721,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)

@@ -4206,7 +4201,7 @@ nfsd4_free_lock_stateid(struct nfs4_ol_stateid *stp)
 {
 	struct nfs4_lockowner *lo = lockowner(stp->st_stateowner);
 
-	if (check_for_locks(stp->st_file, lo))
+	if (check_for_locks(stp->st_inode, lo))
 		return nfserr_locks_held;
 	release_lockowner_if_empty(lo);
 	return nfs_ok;

@@ -4665,7 +4660,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;

fs/nfsd/nfs4state.c alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct · line 4677

Quoted 4 lines

fs/nfsd/nfs4state.c +1 -0

@@ -4677,6 +4674,8 @@ alloc_init_lock_stateid(struct nfs4_lockowner *lo, struct nfs4_file *fp, struct
 	list_add(&stp->st_perstateowner, &lo->lo_owner.so_stateids);
 	stp->st_stateowner = &lo->lo_owner;
 	get_nfs4_file(fp);
+	ihold(inode);

Don't we need a corresponding put somewhere? Am I overlooking it?

--b.

Quoted 65 lines

fs/nfsd/nfs4state.c +5 -4

+	stp->st_inode = inode;
 	stp->st_file = fp;
 	stp->st_access_bmap = 0;
 	stp->st_deny_bmap = open_stp->st_deny_bmap;

@@ -4725,6 +4724,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);

@@ -4745,7 +4745,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;

@@ -5098,10 +5098,9 @@ out_nfserr:
  * 	0: no locks held by lockowner
  */
 static int
-check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner)
+check_for_locks(struct inode *inode, struct nfs4_lockowner *lowner)
 {
 	struct file_lock **flpp;
-	struct inode *inode = filp->fi_inode;
 	int status = 0;
 
 	spin_lock(&inode->i_lock);

@@ -5160,7 +5159,7 @@ nfsd4_release_lockowner(struct svc_rqst *rqstp,
 	lo = lockowner(sop);
 	/* see if there are still any locks associated with it */
 	list_for_each_entry(stp, &sop->so_stateids, st_perstateowner) {
-		if (check_for_locks(stp->st_file, lo))
+		if (check_for_locks(stp->st_inode, lo))
 			goto out;
 	}
 

fs/nfsd/state.h +1 -1

index 33cf950b3873..ba711c66561e 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;
 };
 

@@ -412,6 +411,7 @@ struct nfs4_ol_stateid {
 	unsigned char                 st_access_bmap;
 	unsigned char                 st_deny_bmap;
 	struct nfs4_ol_stateid         * st_openstp;
+	struct inode		    * st_inode;
 };
 
 static inline struct nfs4_ol_stateid *openlockstateid(struct nfs4_stid *s)

Signature · 1 line

1.9.3

This is a port of the patches that Trond sent last night 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. It now only takes them indirectly by virtue of struct file objects in the fi_fds array.

If this looks good, I'll resend the unmerged delegation overhaul patches, rebased on top of this series.

Trond Myklebust (4):
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
nfsd: Do not let nfs4_file pin the struct inode

 fs/nfsd/nfs4state.c | 68 +++++++++++++++++++++++++++++++----------------------
 fs/nfsd/state.h     |  3 ++-
 2 files changed, 42 insertions(+), 29 deletions(-)

Signature · 1 line

1.9.3

Quoted 7 lines · Jeff Layton

This is a port of the patches that Trond sent last night 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. It now only takes them indirectly by virtue of struct file objects in the fi_fds array.

Looks reasonable on a quick skim.

It might make sense to get rid of dl_fh at some point?

--b.

Quoted 18 lines

If this looks good, I'll resend the unmerged delegation overhaul patches, rebased on top of this series.

Trond Myklebust (4):
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
nfsd: Do not let nfs4_file pin the struct inode

 fs/nfsd/nfs4state.c | 68 +++++++++++++++++++++++++++++++----------------------
 fs/nfsd/state.h     |  3 ++-
 2 files changed, 42 insertions(+), 29 deletions(-)

Signature · 1 line

1.9.3