by Adamson, Andy

[permalink] [raw]

Subject: Re: [PATCH Version 7 8/8] NFS pnfs data server multipath session trunking

> On Aug 5, 2016, at 6:05 PM, Trond Myklebust <[email protected]> wrote:
>
>
>> On Jul 27, 2016, at 14:43, [email protected] wrote:
>>
>> From: Andy Adamson <[email protected]>
>>
>> Try all multipath addresses for a data server. The first address that
>> successfully connects and creates a session is the DS mount address.
>> All subsequent addresses are tested for session trunking and
>> added as aliases.
>>
>> Signed-off-by: Andy Adamson <[email protected]>
>> ---
>> fs/nfs/nfs4_fs.h | 2 ++
>> fs/nfs/nfs4proc.c | 2 ++
>> fs/nfs/pnfs_nfs.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
>> 3 files changed, 42 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>> index b2a94ca..8e4343f 100644
>> --- a/fs/nfs/nfs4_fs.h
>> +++ b/fs/nfs/nfs4_fs.h
>> @@ -59,6 +59,8 @@ struct nfs4_minor_version_ops {
>> struct nfs4_lock_state *);
>> struct nfs_seqid *
>> (*alloc_seqid)(struct nfs_seqid_counter *, gfp_t);
>> + int (*session_trunk)(struct rpc_clnt *, struct rpc_xprt_switch *,
>> + struct rpc_xprt *, void *);
>> const struct rpc_call_ops *call_sync_ops;
>> const struct nfs4_state_recovery_ops *reboot_recovery_ops;
>> const struct nfs4_state_recovery_ops *nograce_recovery_ops;
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index fa9ae75..8767720 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -8934,6 +8934,7 @@ static const struct nfs4_minor_version_ops nfs_v4_1_minor_ops = {
>> .find_root_sec = nfs41_find_root_sec,
>> .free_lock_state = nfs41_free_lock_state,
>> .alloc_seqid = nfs_alloc_no_seqid,
>> + .session_trunk = nfs4_test_session_trunk,
>> .call_sync_ops = &nfs41_call_sync_ops,
>> .reboot_recovery_ops = &nfs41_reboot_recovery_ops,
>> .nograce_recovery_ops = &nfs41_nograce_recovery_ops,
>> @@ -8963,6 +8964,7 @@ static const struct nfs4_minor_version_ops nfs_v4_2_minor_ops = {
>> .free_lock_state = nfs41_free_lock_state,
>> .call_sync_ops = &nfs41_call_sync_ops,
>> .alloc_seqid = nfs_alloc_no_seqid,
>> + .session_trunk = nfs4_test_session_trunk,
>> .reboot_recovery_ops = &nfs41_reboot_recovery_ops,
>> .nograce_recovery_ops = &nfs41_nograce_recovery_ops,
>> .state_renewal_ops = &nfs41_state_renewal_ops,
>> diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
>> index f3468b5..7cc1c4d 100644
>> --- a/fs/nfs/pnfs_nfs.c
>> +++ b/fs/nfs/pnfs_nfs.c
>> @@ -690,13 +690,44 @@ static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv,
>> dprintk("%s: DS %s: trying address %s\n",
>> __func__, ds->ds_remotestr, da->da_remotestr);
>>
>> - clp = nfs4_set_ds_client(mds_srv,
>> - (struct sockaddr *)&da->da_addr,
>> - da->da_addrlen, IPPROTO_TCP,
>> - timeo, retrans, minor_version,
>> - au_flavor);
>> - if (!IS_ERR(clp))
>> - break;
>> + if (!IS_ERR(clp) && clp->cl_mvops->session_trunk) {
>> + struct xprt_create xprt_args = {
>> + .ident = XPRT_TRANSPORT_TCP,
>> + .net = clp->cl_net,
>> + .dstaddr = (struct sockaddr *)&da->da_addr,
>> + .addrlen = da->da_addrlen,
>> + .servername = clp->cl_hostname,
>> + };
>> + struct nfs4_add_xprt_data xprtdata = {
>> + .clp = clp,
>> + };
>> + xprtdata.cred = nfs4_get_clid_cred(clp);
>> +
>> + /**
>> + * Test this address for session trunking and
>> + * add as an alias
>> + */
>> + rpc_clnt_add_xprt(clp->cl_rpcclient, &xprt_args,
>> + clp->cl_mvops->session_trunk,
>> + &xprtdata);
>> + } else {
>> + clp = nfs4_set_ds_client(mds_srv,
>> + (struct sockaddr *)&da->da_addr,
>> + da->da_addrlen, IPPROTO_TCP,
>> + timeo, retrans, minor_version,
>> + au_flavor);
>> + if (IS_ERR(clp))
>> + continue;
>> +
>> + status = nfs4_init_ds_session(clp,
>> + mds_srv->nfs_client->cl_lease_time);
>> + if (status) {
>> + nfs_put_client(clp);
>> + clp = ERR_PTR(-EIO);
>> + continue;
>> + }
>> +
>> + }
>> }
>>
>> if (IS_ERR(clp)) {
>> @@ -704,18 +735,11 @@ static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv,
>> goto out;
>> }
>>
>> - status = nfs4_init_ds_session(clp, mds_srv->nfs_client->cl_lease_time);
>> - if (status)
>> - goto out_put;
>> -
>> smp_wmb();
>> ds->ds_clp = clp;
>> dprintk("%s [new] addr: %s\n", __func__, ds->ds_remotestr);
>> out:
>> return status;
>> -out_put:
>> - nfs_put_client(clp);
>> - goto out;
>> }
>
> Is there something which checks whether or not a particular transport has already been added?

Is there a reason that rpc_switch_xprt_add_xprt() doesn?t check to see if the xprt has already been added? IOW should I add the check to rpc_switch_xprt_add_xprt() or have a new interface?

> Also, doesn?t something need to change in nfs_match_client() so that we can find the struct nfs_client using any of the IP addresses that are registered to it?

OK

?>Andy

>
>

2016-08-08 20:18:21

by Trond Myklebust

[permalink] [raw]

Subject: Re: [PATCH Version 7 8/8] NFS pnfs data server multipath session trunking

> On Aug 8, 2016, at 15:57, Adamson, Andy <[email protected]> wrote:
>
>>
>> On Aug 5, 2016, at 6:05 PM, Trond Myklebust <[email protected]> wrote:
>>
>>
>>> On Jul 27, 2016, at 14:43, [email protected] wrote:
>>>
>>> From: Andy Adamson <[email protected]>
>>>
>>> Try all multipath addresses for a data server. The first address that
>>> successfully connects and creates a session is the DS mount address.
>>> All subsequent addresses are tested for session trunking and
>>> added as aliases.
>>>
>>> Signed-off-by: Andy Adamson <[email protected]>
>>> ---
>>> fs/nfs/nfs4_fs.h | 2 ++
>>> fs/nfs/nfs4proc.c | 2 ++
>>> fs/nfs/pnfs_nfs.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
>>> 3 files changed, 42 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4_fs.h b/fs/nfs/nfs4_fs.h
>>> index b2a94ca..8e4343f 100644
>>> --- a/fs/nfs/nfs4_fs.h
>>> +++ b/fs/nfs/nfs4_fs.h
>>> @@ -59,6 +59,8 @@ struct nfs4_minor_version_ops {
>>> struct nfs4_lock_state *);
>>> struct nfs_seqid *
>>> (*alloc_seqid)(struct nfs_seqid_counter *, gfp_t);
>>> + int (*session_trunk)(struct rpc_clnt *, struct rpc_xprt_switch *,
>>> + struct rpc_xprt *, void *);
>>> const struct rpc_call_ops *call_sync_ops;
>>> const struct nfs4_state_recovery_ops *reboot_recovery_ops;
>>> const struct nfs4_state_recovery_ops *nograce_recovery_ops;
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index fa9ae75..8767720 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -8934,6 +8934,7 @@ static const struct nfs4_minor_version_ops nfs_v4_1_minor_ops = {
>>> .find_root_sec = nfs41_find_root_sec,
>>> .free_lock_state = nfs41_free_lock_state,
>>> .alloc_seqid = nfs_alloc_no_seqid,
>>> + .session_trunk = nfs4_test_session_trunk,
>>> .call_sync_ops = &nfs41_call_sync_ops,
>>> .reboot_recovery_ops = &nfs41_reboot_recovery_ops,
>>> .nograce_recovery_ops = &nfs41_nograce_recovery_ops,
>>> @@ -8963,6 +8964,7 @@ static const struct nfs4_minor_version_ops nfs_v4_2_minor_ops = {
>>> .free_lock_state = nfs41_free_lock_state,
>>> .call_sync_ops = &nfs41_call_sync_ops,
>>> .alloc_seqid = nfs_alloc_no_seqid,
>>> + .session_trunk = nfs4_test_session_trunk,
>>> .reboot_recovery_ops = &nfs41_reboot_recovery_ops,
>>> .nograce_recovery_ops = &nfs41_nograce_recovery_ops,
>>> .state_renewal_ops = &nfs41_state_renewal_ops,
>>> diff --git a/fs/nfs/pnfs_nfs.c b/fs/nfs/pnfs_nfs.c
>>> index f3468b5..7cc1c4d 100644
>>> --- a/fs/nfs/pnfs_nfs.c
>>> +++ b/fs/nfs/pnfs_nfs.c
>>> @@ -690,13 +690,44 @@ static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv,
>>> dprintk("%s: DS %s: trying address %s\n",
>>> __func__, ds->ds_remotestr, da->da_remotestr);
>>>
>>> - clp = nfs4_set_ds_client(mds_srv,
>>> - (struct sockaddr *)&da->da_addr,
>>> - da->da_addrlen, IPPROTO_TCP,
>>> - timeo, retrans, minor_version,
>>> - au_flavor);
>>> - if (!IS_ERR(clp))
>>> - break;
>>> + if (!IS_ERR(clp) && clp->cl_mvops->session_trunk) {
>>> + struct xprt_create xprt_args = {
>>> + .ident = XPRT_TRANSPORT_TCP,
>>> + .net = clp->cl_net,
>>> + .dstaddr = (struct sockaddr *)&da->da_addr,
>>> + .addrlen = da->da_addrlen,
>>> + .servername = clp->cl_hostname,
>>> + };
>>> + struct nfs4_add_xprt_data xprtdata = {
>>> + .clp = clp,
>>> + };
>>> + xprtdata.cred = nfs4_get_clid_cred(clp);
>>> +
>>> + /**
>>> + * Test this address for session trunking and
>>> + * add as an alias
>>> + */
>>> + rpc_clnt_add_xprt(clp->cl_rpcclient, &xprt_args,
>>> + clp->cl_mvops->session_trunk,
>>> + &xprtdata);
>>> + } else {
>>> + clp = nfs4_set_ds_client(mds_srv,
>>> + (struct sockaddr *)&da->da_addr,
>>> + da->da_addrlen, IPPROTO_TCP,
>>> + timeo, retrans, minor_version,
>>> + au_flavor);
>>> + if (IS_ERR(clp))
>>> + continue;
>>> +
>>> + status = nfs4_init_ds_session(clp,
>>> + mds_srv->nfs_client->cl_lease_time);
>>> + if (status) {
>>> + nfs_put_client(clp);
>>> + clp = ERR_PTR(-EIO);
>>> + continue;
>>> + }
>>> +
>>> + }
>>> }
>>>
>>> if (IS_ERR(clp)) {
>>> @@ -704,18 +735,11 @@ static int _nfs4_pnfs_v4_ds_connect(struct nfs_server *mds_srv,
>>> goto out;
>>> }
>>>
>>> - status = nfs4_init_ds_session(clp, mds_srv->nfs_client->cl_lease_time);
>>> - if (status)
>>> - goto out_put;
>>> -
>>> smp_wmb();
>>> ds->ds_clp = clp;
>>> dprintk("%s [new] addr: %s\n", __func__, ds->ds_remotestr);
>>> out:
>>> return status;
>>> -out_put:
>>> - nfs_put_client(clp);
>>> - goto out;
>>> }
>>
>> Is there something which checks whether or not a particular transport has already been added?
>
> Is there a reason that rpc_switch_xprt_add_xprt() doesn?t check to see if the xprt has already been added? IOW should I add the check to rpc_switch_xprt_add_xprt() or have a new interface?
>

Adding it to rpc_switch_xprt_add_xprt() should be OK.

>
>> Also, doesn?t something need to change in nfs_match_client() so that we can find the struct nfs_client using any of the IP addresses that are registered to it?
>
> OK
>
> ?>Andy