diff mbox

[v2,04/10] nfsd: Ensure stateids remain unique until they are freed

Message ID 1405521125-2303-5-git-send-email-jlayton@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton July 16, 2014, 2:31 p.m. UTC 
From: Trond Myklebust <trond.myklebust@primarydata.com>

Add an extra delegation state to allow the stateid to remain in the idr
tree until the last reference has been released. This will be necessary
to ensure uniqueness once the client_mutex is removed.

[jlayton: reset the sc_type under the state_lock in unhash_delegation]

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 fs/nfsd/nfs4state.c | 8 ++++----
 fs/nfsd/state.h     | 1 +
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

J. Bruce Fields July 17, 2014, 6:44 p.m. UTC | #1 
On Wed, Jul 16, 2014 at 10:31:59AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> Add an extra delegation state to allow the stateid to remain in the idr
> tree until the last reference has been released. This will be necessary
> to ensure uniqueness once the client_mutex is removed.

Cyclic idr allocation should make reuse extremely unlikely, shouldn't
it?

Probably needs to be fixed anyway, applying.

--b.

> 
> [jlayton: reset the sc_type under the state_lock in unhash_delegation]
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfsd/nfs4state.c | 8 ++++----
>  fs/nfsd/state.h     | 1 +
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1b01a20827ab..fd4deb049ddf 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -616,6 +616,7 @@ void
>  nfs4_put_delegation(struct nfs4_delegation *dp)
>  {
>  	if (atomic_dec_and_test(&dp->dl_count)) {
> +		remove_stid(&dp->dl_stid);
>  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
>  		num_delegations--;
>  	}
> @@ -657,6 +658,7 @@ unhash_delegation(struct nfs4_delegation *dp)
>  	struct nfs4_file *fp = dp->dl_file;
>  
>  	spin_lock(&state_lock);
> +	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
>  	list_del_init(&dp->dl_perclnt);
>  	list_del_init(&dp->dl_recall_lru);
>  	spin_lock(&fp->fi_lock);
> @@ -670,19 +672,15 @@ unhash_delegation(struct nfs4_delegation *dp)
>  	}
>  }
>  
> -
> -
>  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
>  {
>  	list_del_init(&dp->dl_recall_lru);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
>  static void destroy_delegation(struct nfs4_delegation *dp)
>  {
>  	unhash_delegation(dp);
> -	remove_stid(&dp->dl_stid);
>  	nfs4_put_delegation(dp);
>  }
>  
> @@ -4036,7 +4034,9 @@ static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
>  		return nfs_ok;
>  	default:
>  		printk("unknown stateid type %x\n", s->sc_type);
> +		/* Fallthrough */
>  	case NFS4_CLOSED_STID:
> +	case NFS4_CLOSED_DELEG_STID:
>  		return nfserr_bad_stateid;
>  	}
>  }
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 81b7522e3f67..996d61eeb357 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -80,6 +80,7 @@ struct nfs4_stid {
>  #define NFS4_CLOSED_STID 8
>  /* For a deleg stateid kept around only to process free_stateid's: */
>  #define NFS4_REVOKED_DELEG_STID 16
> +#define NFS4_CLOSED_DELEG_STID 32
>  	unsigned char sc_type;
>  	stateid_t sc_stateid;
>  	struct nfs4_client *sc_client;
> -- 
> 1.9.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Layton July 17, 2014, 6:46 p.m. UTC | #2 
On Thu, 17 Jul 2014 14:44:17 -0400
"J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Wed, Jul 16, 2014 at 10:31:59AM -0400, Jeff Layton wrote:
> > From: Trond Myklebust <trond.myklebust@primarydata.com>
> > 
> > Add an extra delegation state to allow the stateid to remain in the idr
> > tree until the last reference has been released. This will be necessary
> > to ensure uniqueness once the client_mutex is removed.
> 
> Cyclic idr allocation should make reuse extremely unlikely, shouldn't
> it?
> 
> Probably needs to be fixed anyway, applying.
> 
> --b.
> 

Yes, for the most part, but reuse is still possible once the counter
wraps if you had a long-lived stateid (for instance). Not a likely
problem, but we might as well fix it while we're in here.
 
> > 
> > [jlayton: reset the sc_type under the state_lock in
> > unhash_delegation]
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> > Signed-off-by: Jeff Layton <jlayton@primarydata.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/nfsd/nfs4state.c | 8 ++++----
> >  fs/nfsd/state.h     | 1 +
> >  2 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 1b01a20827ab..fd4deb049ddf 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -616,6 +616,7 @@ void
> >  nfs4_put_delegation(struct nfs4_delegation *dp)
> >  {
> >  	if (atomic_dec_and_test(&dp->dl_count)) {
> > +		remove_stid(&dp->dl_stid);
> >  		nfs4_free_stid(deleg_slab, &dp->dl_stid);
> >  		num_delegations--;
> >  	}
> > @@ -657,6 +658,7 @@ unhash_delegation(struct nfs4_delegation *dp)
> >  	struct nfs4_file *fp = dp->dl_file;
> >  
> >  	spin_lock(&state_lock);
> > +	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
> >  	list_del_init(&dp->dl_perclnt);
> >  	list_del_init(&dp->dl_recall_lru);
> >  	spin_lock(&fp->fi_lock);
> > @@ -670,19 +672,15 @@ unhash_delegation(struct nfs4_delegation *dp)
> >  	}
> >  }
> >  
> > -
> > -
> >  static void destroy_revoked_delegation(struct nfs4_delegation *dp)
> >  {
> >  	list_del_init(&dp->dl_recall_lru);
> > -	remove_stid(&dp->dl_stid);
> >  	nfs4_put_delegation(dp);
> >  }
> >  
> >  static void destroy_delegation(struct nfs4_delegation *dp)
> >  {
> >  	unhash_delegation(dp);
> > -	remove_stid(&dp->dl_stid);
> >  	nfs4_put_delegation(dp);
> >  }
> >  
> > @@ -4036,7 +4034,9 @@ static __be32 nfsd4_validate_stateid(struct
> > nfs4_client *cl, stateid_t *stateid) return nfs_ok;
> >  	default:
> >  		printk("unknown stateid type %x\n", s->sc_type);
> > +		/* Fallthrough */
> >  	case NFS4_CLOSED_STID:
> > +	case NFS4_CLOSED_DELEG_STID:
> >  		return nfserr_bad_stateid;
> >  	}
> >  }
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 81b7522e3f67..996d61eeb357 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -80,6 +80,7 @@ struct nfs4_stid {
> >  #define NFS4_CLOSED_STID 8
> >  /* For a deleg stateid kept around only to process free_stateid's:
> > */ #define NFS4_REVOKED_DELEG_STID 16
> > +#define NFS4_CLOSED_DELEG_STID 32
> >  	unsigned char sc_type;
> >  	stateid_t sc_stateid;
> >  	struct nfs4_client *sc_client;
> > -- 
> > 1.9.3
> >
diff mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1b01a20827ab..fd4deb049ddf 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -616,6 +616,7 @@  void
 nfs4_put_delegation(struct nfs4_delegation *dp)
 {
 	if (atomic_dec_and_test(&dp->dl_count)) {
+		remove_stid(&dp->dl_stid);
 		nfs4_free_stid(deleg_slab, &dp->dl_stid);
 		num_delegations--;
 	}
@@ -657,6 +658,7 @@  unhash_delegation(struct nfs4_delegation *dp)
 	struct nfs4_file *fp = dp->dl_file;
 
 	spin_lock(&state_lock);
+	dp->dl_stid.sc_type = NFS4_CLOSED_DELEG_STID;
 	list_del_init(&dp->dl_perclnt);
 	list_del_init(&dp->dl_recall_lru);
 	spin_lock(&fp->fi_lock);
@@ -670,19 +672,15 @@  unhash_delegation(struct nfs4_delegation *dp)
 	}
 }
 
-
-
 static void destroy_revoked_delegation(struct nfs4_delegation *dp)
 {
 	list_del_init(&dp->dl_recall_lru);
-	remove_stid(&dp->dl_stid);
 	nfs4_put_delegation(dp);
 }
 
 static void destroy_delegation(struct nfs4_delegation *dp)
 {
 	unhash_delegation(dp);
-	remove_stid(&dp->dl_stid);
 	nfs4_put_delegation(dp);
 }
 
@@ -4036,7 +4034,9 @@  static __be32 nfsd4_validate_stateid(struct nfs4_client *cl, stateid_t *stateid)
 		return nfs_ok;
 	default:
 		printk("unknown stateid type %x\n", s->sc_type);
+		/* Fallthrough */
 	case NFS4_CLOSED_STID:
+	case NFS4_CLOSED_DELEG_STID:
 		return nfserr_bad_stateid;
 	}
 }
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 81b7522e3f67..996d61eeb357 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -80,6 +80,7 @@  struct nfs4_stid {
 #define NFS4_CLOSED_STID 8
 /* For a deleg stateid kept around only to process free_stateid's: */
 #define NFS4_REVOKED_DELEG_STID 16
+#define NFS4_CLOSED_DELEG_STID 32
 	unsigned char sc_type;
 	stateid_t sc_stateid;
 	struct nfs4_client *sc_client;