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