Re: [PATCH 1/3] NFSv4.1: Fix session initialisation races

["Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx>]

On Wed, 2012-05-23 at 19:14 +0000, Myklebust, Trond wrote:
> On Wed, 2012-05-23 at 18:37 +0000, Adamson, Andy wrote:
> > 
> > On May 23, 2012, at 1:32 PM, Trond Myklebust wrote:
> > 
> > > Session initialisation is not complete until the lease manager
> > > has run. We need to ensure that both nfs4_init_session and
> > > nfs4_init_ds_session do so, and that they check for any resulting
> > > errors in clp->cl_cons_state.
> > > 
> > > Only after this is done, can nfs4_ds_connect check the contents
> > > of clp->cl_exchange_flags.
> > > 
> > > Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
> > > Cc: Andy Adamson <andros@xxxxxxxxxx>
> > > ---
> > > fs/nfs/client.c            |   16 ---------
> > > fs/nfs/internal.h          |    3 +-
> > > fs/nfs/nfs4filelayoutdev.c |   23 +------------
> > > fs/nfs/nfs4proc.c          |   77 ++++++++++++++++++++++++++++---------------
> > > 4 files changed, 52 insertions(+), 67 deletions(-)
> > > 
> > > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > > index 60f7e4e..78970a1 100644
> > > --- a/fs/nfs/client.c
> > > +++ b/fs/nfs/client.c
> > > @@ -593,22 +593,6 @@ void nfs_mark_client_ready(struct nfs_client *clp, int state)
> > > }
> > > 
> > > /*
> > > - * With sessions, the client is not marked ready until after a
> > > - * successful EXCHANGE_ID and CREATE_SESSION.
> > > - *
> > > - * Map errors cl_cons_state errors to EPROTONOSUPPORT to indicate
> > > - * other versions of NFS can be tried.
> > > - */
> > > -int nfs4_check_client_ready(struct nfs_client *clp)
> > > -{
> > > -	if (!nfs4_has_session(clp))
> > > -		return 0;
> > > -	if (clp->cl_cons_state < NFS_CS_READY)
> > > -		return -EPROTONOSUPPORT;
> > > -	return 0;
> > > -}
> > > -
> > > -/*
> > >  * Initialise the timeout values for a connection
> > >  */
> > > static void nfs_init_timeout_values(struct rpc_timeout *to, int proto,
> > > diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> > > index b777bda..00b66de 100644
> > > --- a/fs/nfs/internal.h
> > > +++ b/fs/nfs/internal.h
> > > @@ -168,7 +168,6 @@ extern struct nfs_server *nfs_clone_server(struct nfs_server *,
> > > 					   struct nfs_fattr *,
> > > 					   rpc_authflavor_t);
> > > extern void nfs_mark_client_ready(struct nfs_client *clp, int state);
> > > -extern int nfs4_check_client_ready(struct nfs_client *clp);
> > > extern struct nfs_client *nfs4_set_ds_client(struct nfs_client* mds_clp,
> > > 					     const struct sockaddr *ds_addr,
> > > 					     int ds_addrlen, int ds_proto);
> > > @@ -237,7 +236,7 @@ extern const u32 nfs41_maxwrite_overhead;
> > > extern struct rpc_procinfo nfs4_procedures[];
> > > #endif
> > > 
> > > -extern int nfs4_init_ds_session(struct nfs_client *clp);
> > > +extern int nfs4_init_ds_session(struct nfs_client *, unsigned long);
> > > 
> > > /* proc.c */
> > > void nfs_close_context(struct nfs_open_context *ctx, int is_sync);
> > > diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
> > > index c9cff9a..0764c98 100644
> > > --- a/fs/nfs/nfs4filelayoutdev.c
> > > +++ b/fs/nfs/nfs4filelayoutdev.c
> > > @@ -176,28 +176,7 @@ nfs4_ds_connect(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
> > > 		goto out;
> > > 	}
> > > 
> > > -	if ((clp->cl_exchange_flags & EXCHGID4_FLAG_MASK_PNFS) != 0) {
> > > -		if (!is_ds_client(clp)) {
> > > -			status = -ENODEV;
> > > -			goto out_put;
> > > -		}
> > > -		ds->ds_clp = clp;
> > > -		dprintk("%s [existing] server=%s\n", __func__,
> > > -			ds->ds_remotestr);
> > > -		goto out;
> > > -	}
> > > -
> > > -	/*
> > > -	 * Do not set NFS_CS_CHECK_LEASE_TIME instead set the DS lease to
> > > -	 * be equal to the MDS lease. Renewal is scheduled in create_session.
> > > -	 */
> > > -	spin_lock(&mds_srv->nfs_client->cl_lock);
> > > -	clp->cl_lease_time = mds_srv->nfs_client->cl_lease_time;
> > > -	spin_unlock(&mds_srv->nfs_client->cl_lock);
> > > -	clp->cl_last_renewal = jiffies;
> > > -
> > > -	/* New nfs_client */
> > > -	status = nfs4_init_ds_session(clp);
> > > +	status = nfs4_init_ds_session(clp, mds_srv->nfs_client->cl_lease_time);
> > > 	if (status)
> > > 		goto out_put;
> > > 
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index 99650aa..272c4ad 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -5605,53 +5605,76 @@ int nfs4_proc_destroy_session(struct nfs4_session *session)
> > > 	return status;
> > > }
> > > 
> > > +/*
> > > + * With sessions, the client is not marked ready until after a
> > > + * successful EXCHANGE_ID and CREATE_SESSION.
> > > + *
> > > + * Map errors cl_cons_state errors to EPROTONOSUPPORT to indicate
> > > + * other versions of NFS can be tried.
> > > + */
> > > +static int nfs41_check_session_ready(struct nfs_client *clp)
> > > +{
> > > +	int ret;
> > > +	
> > > +	ret = nfs4_client_recover_expired_lease(clp);
> > > +	if (ret)
> > > +		return ret;
> > > +	if (clp->cl_cons_state < NFS_CS_READY)
> > > +		return -EPROTONOSUPPORT;
> > > +	return 0;
> > > +}
> > > +
> > > int nfs4_init_session(struct nfs_server *server)
> > > {
> > > 	struct nfs_client *clp = server->nfs_client;
> > > 	struct nfs4_session *session;
> > > 	unsigned int rsize, wsize;
> > > -	int ret;
> > > 
> > > 	if (!nfs4_has_session(clp))
> > > 		return 0;
> > > 
> > > 	session = clp->cl_session;
> > > -	if (!test_and_clear_bit(NFS4_SESSION_INITING, &session->session_state))
> > > -		return 0;
> > > +	spin_lock(&clp->cl_lock);
> > > +	if (test_and_clear_bit(NFS4_SESSION_INITING, &session->session_state)) {
> > > 
> > > -	rsize = server->rsize;
> > > -	if (rsize == 0)
> > > -		rsize = NFS_MAX_FILE_IO_SIZE;
> > > -	wsize = server->wsize;
> > > -	if (wsize == 0)
> > > -		wsize = NFS_MAX_FILE_IO_SIZE;
> > > +		rsize = server->rsize;
> > > +		if (rsize == 0)
> > > +			rsize = NFS_MAX_FILE_IO_SIZE;
> > > +		wsize = server->wsize;
> > > +		if (wsize == 0)
> > > +			wsize = NFS_MAX_FILE_IO_SIZE;
> > > 
> > > -	session->fc_attrs.max_rqst_sz = wsize + nfs41_maxwrite_overhead;
> > > -	session->fc_attrs.max_resp_sz = rsize + nfs41_maxread_overhead;
> > > +		session->fc_attrs.max_rqst_sz = wsize + nfs41_maxwrite_overhead;
> > > +		session->fc_attrs.max_resp_sz = rsize + nfs41_maxread_overhead;
> > > +	}
> > > +	spin_unlock(&clp->cl_lock);
> > > 
> > > -	ret = nfs4_recover_expired_lease(server);
> > > -	if (!ret)
> > > -		ret = nfs4_check_client_ready(clp);
> > > -	return ret;
> > > +	return nfs41_check_session_ready(clp);
> > > }
> > > 
> > > -int nfs4_init_ds_session(struct nfs_client *clp)
> > > +int nfs4_init_ds_session(struct nfs_client *clp, unsigned long lease_time)
> > > {
> > > 	struct nfs4_session *session = clp->cl_session;
> > > 	int ret;
> > > 
> > > -	if (!test_and_clear_bit(NFS4_SESSION_INITING, &session->session_state))
> > > -		return 0;
> > > -
> > > -	ret = nfs4_client_recover_expired_lease(clp);
> > > -	if (!ret)
> > > -		/* Test for the DS role */
> > > -		if (!is_ds_client(clp))
> > > -			ret = -ENODEV;
> > > -	if (!ret)
> > > -		ret = nfs4_check_client_ready(clp);
> > > -	return ret;
> > > +	spin_lock(&clp->cl_lock);
> > > +	if (test_and_clear_bit(NFS4_SESSION_INITING, &session->session_state)) {
> > > +		/*
> > > +		 * Do not set NFS_CS_CHECK_LEASE_TIME instead set the
> > > +		 * DS lease to be equal to the MDS lease.
> > > +		 */
> > > +		clp->cl_lease_time = lease_time;
> > > +		clp->cl_last_renewal = jiffies;
> > > +	}
> > > +	spin_unlock(&clp->cl_lock);
> > > 
> > > +	ret = nfs41_check_session_ready(clp);
> > 
> > If I read the code correctly, on an existing client,  this call needlessly waits for the state manager to finish whatever it is doing. As the state manager could be operating on a different file system than the DS belongs to,  and recovering a bunch of state,  it might be worth while to avoid.  That was the intention of checking the cl_exchange_flags in nfs4_ds_connect.
> > In fact, on an existing client, nfs4_init_ds_session does not need to be called.
> 
> Yes it does. Right now you have no guarantee that the state manager is
> finished setting up the session, and so no guarantee that it will
> succeed.
> 
> We can perhaps skip the call to nfs4_recover_expired_lease() if (and
> only if) clp->cl_cons_state <= NFS_CS_READY, but otherwise it does need
> to complete.

IOW: Something like the following change to nfs41_check_session_ready():

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 565105b..c856298 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5614,9 +5614,11 @@ static int nfs41_check_session_ready(struct nfs_client *clp)
 {
 	int ret;
 	
-	ret = nfs4_client_recover_expired_lease(clp);
-	if (ret)
-		return ret;
+	if (clp->cl_cons_state == NFS_CS_SESSION_INITING) {
+		ret = nfs4_client_recover_expired_lease(clp);
+		if (ret)
+			return ret;
+	}
 	if (clp->cl_cons_state < NFS_CS_READY)
 		return -EPROTONOSUPPORT;
 	return 0;


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com

��.n��������+%������w��{.n�����{��w���jg��������ݢj����G�������j:+v���w�m������w�������h�����٥