Re: [PATCH v2 10/49] nfs41: create NFSv3 DS connection if specified

[Tom Haynes <thomas.haynes@primarydata.com>]

On Mon, Jan 05, 2015 at 11:57:17AM -0500, Anna Schumaker wrote:
> Hey Tom and Peng,
> 
> On 12/24/2014 02:12 AM, Tom Haynes wrote:
> > From: Peng Tao <tao.peng@primarydata.com>
> > 
> > Signed-off-by: Peng Tao <tao.peng@primarydata.com>
> > Signed-off-by: Tom Haynes <Thomas.Haynes@primarydata.com>
> > ---
> >  fs/nfs/pnfs_dev.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 51 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfs/pnfs_dev.c b/fs/nfs/pnfs_dev.c
> > index 56f5c16..655333d 100644
> > --- a/fs/nfs/pnfs_dev.c
> > +++ b/fs/nfs/pnfs_dev.c
> > @@ -615,7 +615,44 @@ static void nfs4_clear_ds_conn_bit(struct nfs4_pnfs_ds *ds)
> >  	wake_up_bit(&ds->ds_state, NFS4DS_CONNECTING);
> >  }
> >  
> > -static int _nfs4_pnfs_ds_connect(struct nfs_server *mds_srv,
> > +static int _nfs4_pnfs_v3_ds_connect(struct nfs_server *mds_srv,
> > +				 struct nfs4_pnfs_ds *ds,
> > +				 unsigned int timeo,
> > +				 unsigned int retrans,
> > +				 rpc_authflavor_t au_flavor)
> > +{
> > +	struct nfs_client *clp = ERR_PTR(-EIO);
> > +	struct nfs4_pnfs_ds_addr *da;
> > +	int status = 0;
> > +
> > +	dprintk("--> %s DS %s au_flavor %d\n", __func__,
> > +		ds->ds_remotestr, au_flavor);
> > +
> > +	list_for_each_entry(da, &ds->ds_addrs, da_node) {
> > +		dprintk("%s: DS %s: trying address %s\n",
> > +			__func__, ds->ds_remotestr, da->da_remotestr);
> > +
> > +		clp = nfs3_set_ds_client(mds_srv->nfs_client,
> > +					(struct sockaddr *)&da->da_addr,
> > +					da->da_addrlen, IPPROTO_TCP,
> > +					timeo, retrans, au_flavor);
> > +		if (!IS_ERR(clp))
> > +			break;
> > +	}
> > +
> > +	if (IS_ERR(clp)) {
> > +		status = PTR_ERR(clp);
> > +		goto out;
> > +	}
> > +
> > +	smp_wmb();
> > +	ds->ds_clp = clp;
> > +	dprintk("%s [new] addr: %s\n", __func__, ds->ds_remotestr);
> > +out:
> > +	return status;
> > +}
> > +
> > +static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv,
> >  				 struct nfs4_pnfs_ds *ds,
> >  				 unsigned int timeo,
> >  				 unsigned int retrans,
> > @@ -674,8 +711,19 @@ void nfs4_pnfs_ds_connect(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds,
> >  	if (test_and_set_bit(NFS4DS_CONNECTING, &ds->ds_state) == 0) {
> >  		int err = 0;
> >  
> > -		err = _nfs4_pnfs_ds_connect(mds_srv, ds, timeo, retrans,
> > -					    minor_version, au_flavor);
> > +		if (version == 3) {
> > +			err = _nfs4_pnfs_v3_ds_connect(mds_srv, ds, timeo,
> > +						       retrans, au_flavor);
> > +		} else if (version == 4) {
> > +			err = _nfs4_pnfs_v4_ds_connect(mds_srv, ds, timeo,
> > +						       retrans, minor_version,
> > +						       au_flavor);
> 
> Is it possible to do this with NFS-version specific function pointers, similar to how we have the rpc_ops array?
> 
> Thanks,
> Anna

Hi Anna,

I think that nfs4_pnfs_ds_connect is the function that abstracts
away calling the version specific code. To set up the function
pointers, we would have to have the callers do so. And while
nfs4_fl_prepare_ds() will always call _nfs4_pnfs_v4_ds_connect(),
nfs4_ff_layout_prepare_ds() could call either. I.e., I think this
approach hides that complexity from the caller.

Thanks,
Tom

> 
> > +		} else {
> > +			dprintk("%s: unsupported DS version %d\n", __func__,
> > +				version);
> > +			err = -EPROTONOSUPPORT;
> > +		}
> > +
> >  		if (err)
> >  			nfs4_mark_deviceid_unavailable(devid);
> >  		nfs4_clear_ds_conn_bit(ds);
> > 
>