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

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

[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.

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(-)

-- 
1.9.3

[PATCH 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 a3a828d17563..393cd83a4f42 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;
@@ -3051,14 +3053,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);
@@ -3713,7 +3715,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 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 393cd83a4f42..c540a46f6305 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);
 
@@ -3025,15 +3037,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;
 		}
@@ -3042,12 +3054,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;
 }
@@ -3058,7 +3070,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;
@@ -3075,11 +3087,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 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 c540a46f6305..4c9404500a8e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3953,7 +3953,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 4/4] nfsd: Do not let nfs4_file pin the struct inode

[Jeff Layton]

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

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 <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 31 +++++++++++++++----------------
 fs/nfsd/state.h     |  2 +-
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 4c9404500a8e..9b5775f2af57 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 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;
@@ -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);
+	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;
 	}
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 33cf950b3873..ba711c66561e 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;
 };
 
@@ -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)
-- 
1.9.3

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

[J. Bruce Fields]

On Tue, Jul 22, 2014 at 12:49:40PM -0400, Jeff Layton wrote:
> 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.

> 
> 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(-)
> 
> -- 
> 1.9.3
> 

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

[Jeff Layton]

On Tue, 22 Jul 2014 13:51:55 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Jul 22, 2014 at 12:49:40PM -0400, Jeff Layton wrote:
> > 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.
> 

Yeah, I think that should be doable. I'll look at rolling that into the
delegation patches.


> > 
> > 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(-)
> > 
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>

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

[J. Bruce Fields]

On Tue, Jul 22, 2014 at 12:49:44PM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> 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 <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> ---
>  fs/nfsd/nfs4state.c | 31 +++++++++++++++----------------
>  fs/nfsd/state.h     |  2 +-
>  2 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4c9404500a8e..9b5775f2af57 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 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;
> @@ -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.

> +	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;
>  	}
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 33cf950b3873..ba711c66561e 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;
>  };
>  
> @@ -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)
> -- 
> 1.9.3
> 

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

[Jeff Layton]

On Tue, 22 Jul 2014 16:16:16 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Tue, Jul 22, 2014 at 12:49:44PM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > 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 <trond.myklebust@primarydata.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > ---
> >  fs/nfsd/nfs4state.c | 31 +++++++++++++++----------------
> >  fs/nfsd/state.h     |  2 +-
> >  2 files changed, 16 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 4c9404500a8e..9b5775f2af57 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 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;
> > @@ -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.
> 

Quite right. I'm not sure how that hunk got dropped, but it does need
to be there. Now that I look too, I'm not certain that the st_inode
field is really needed.

We should be able to just do a find_any_file vs. the nfs4_file to get
the inode, and if that comes back NULL, we just return 0.

I'll send an updated set tomorrow once I've had time to do a bit more
testing.

> > +	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;
> >  	}
> >  
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 33cf950b3873..ba711c66561e 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;
> >  };
> >  
> > @@ -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)
> > -- 
> > 1.9.3
> > 


-- 
Jeff Layton <jlayton@primarydata.com>