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�����٥
