Currently, our trunking code will check for session trunking, but will
fail to detect client id trunking. This is a problem, because it means
that the client will fail to recognise that the two connections represent
shared state, even if they do not permit a shared session.
By removing the check for the server minor id, and only checking the
major id, we will end up doing the right thing in both cases: we close
down the new nfs_client and fall back to using the existing one.
Fixes: 05f4c350ee02e ("NFS: Discover NFSv4 server trunking when mounting")
Cc: Chuck Lever <[email protected]>
Cc: [email protected] # 3.7.x
Signed-off-by: Trond Myklebust <[email protected]>
---
fs/nfs/nfs4client.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 03311259b0c4..d949d0f378ec 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -566,20 +566,14 @@ static bool nfs4_match_clientids(struct nfs_client *a, struct nfs_client *b)
}
/*
- * Returns true if the server owners match
+ * Returns true if the server major ids match
*/
static bool
-nfs4_match_serverowners(struct nfs_client *a, struct nfs_client *b)
+nfs4_check_clientid_trunking(struct nfs_client *a, struct nfs_client *b)
{
struct nfs41_server_owner *o1 = a->cl_serverowner;
struct nfs41_server_owner *o2 = b->cl_serverowner;
- if (o1->minor_id != o2->minor_id) {
- dprintk("NFS: --> %s server owner minor IDs do not match\n",
- __func__);
- return false;
- }
-
if (o1->major_id_sz != o2->major_id_sz)
goto out_major_mismatch;
if (memcmp(o1->major_id, o2->major_id, o1->major_id_sz) != 0)
@@ -654,7 +648,12 @@ int nfs41_walk_client_list(struct nfs_client *new,
if (!nfs4_match_clientids(pos, new))
continue;
- if (!nfs4_match_serverowners(pos, new))
+ /*
+ * Note that session trunking is just a special subcase of
+ * client id trunking. In either case, we want to fall back
+ * to using the existing nfs_client.
+ */
+ if (!nfs4_check_clientid_trunking(pos, new))
continue;
atomic_inc(&pos->cl_count);
--
2.1.0
On Jan 2, 2015, at 4:39 PM, Trond Myklebust <[email protected]> wrote:
> Currently, our trunking code will check for session trunking, but will
> fail to detect client id trunking. This is a problem, because it means
> that the client will fail to recognise that the two connections represent
> shared state, even if they do not permit a shared session.
> By removing the check for the server minor id, and only checking the
> major id, we will end up doing the right thing in both cases: we close
> down the new nfs_client and fall back to using the existing one.
Fair enough.
Does this detect the case where two concurrent NFSv4.1 mounts
of the same server use different transport protocols?
> Fixes: 05f4c350ee02e ("NFS: Discover NFSv4 server trunking when mounting")
> Cc: Chuck Lever <[email protected]>
> Cc: [email protected] # 3.7.x
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
> fs/nfs/nfs4client.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 03311259b0c4..d949d0f378ec 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -566,20 +566,14 @@ static bool nfs4_match_clientids(struct nfs_client *a, struct nfs_client *b)
> }
>
> /*
> - * Returns true if the server owners match
> + * Returns true if the server major ids match
> */
> static bool
> -nfs4_match_serverowners(struct nfs_client *a, struct nfs_client *b)
> +nfs4_check_clientid_trunking(struct nfs_client *a, struct nfs_client *b)
> {
> struct nfs41_server_owner *o1 = a->cl_serverowner;
> struct nfs41_server_owner *o2 = b->cl_serverowner;
>
> - if (o1->minor_id != o2->minor_id) {
> - dprintk("NFS: --> %s server owner minor IDs do not match\n",
> - __func__);
> - return false;
> - }
> -
> if (o1->major_id_sz != o2->major_id_sz)
> goto out_major_mismatch;
> if (memcmp(o1->major_id, o2->major_id, o1->major_id_sz) != 0)
> @@ -654,7 +648,12 @@ int nfs41_walk_client_list(struct nfs_client *new,
> if (!nfs4_match_clientids(pos, new))
> continue;
>
> - if (!nfs4_match_serverowners(pos, new))
> + /*
> + * Note that session trunking is just a special subcase of
> + * client id trunking. In either case, we want to fall back
> + * to using the existing nfs_client.
> + */
> + if (!nfs4_check_clientid_trunking(pos, new))
> continue;
>
> atomic_inc(&pos->cl_count);
> --
> 2.1.0
>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, Jan 2, 2015 at 4:44 PM, Chuck Lever <[email protected]> wrote:
>
> On Jan 2, 2015, at 4:39 PM, Trond Myklebust <[email protected]> wrote:
>
>> Currently, our trunking code will check for session trunking, but will
>> fail to detect client id trunking. This is a problem, because it means
>> that the client will fail to recognise that the two connections represent
>> shared state, even if they do not permit a shared session.
>> By removing the check for the server minor id, and only checking the
>> major id, we will end up doing the right thing in both cases: we close
>> down the new nfs_client and fall back to using the existing one.
>
> Fair enough.
>
> Does this detect the case where two concurrent NFSv4.1 mounts
> of the same server use different transport protocols?
I'm not sure I understand what you mean. The client should completely
ignore the transport protocol when doing trunking detection. The only
thing that it cares about is whether or not the server believes we
should be sharing state, in which case the client should fall back to
using the existing connection for the new mount point.
>> Fixes: 05f4c350ee02e ("NFS: Discover NFSv4 server trunking when mounting")
>> Cc: Chuck Lever <[email protected]>
>> Cc: [email protected] # 3.7.x
>> Signed-off-by: Trond Myklebust <[email protected]>
>> ---
>> fs/nfs/nfs4client.c | 17 ++++++++---------
>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>> index 03311259b0c4..d949d0f378ec 100644
>> --- a/fs/nfs/nfs4client.c
>> +++ b/fs/nfs/nfs4client.c
>> @@ -566,20 +566,14 @@ static bool nfs4_match_clientids(struct nfs_client *a, struct nfs_client *b)
>> }
>>
>> /*
>> - * Returns true if the server owners match
>> + * Returns true if the server major ids match
>> */
>> static bool
>> -nfs4_match_serverowners(struct nfs_client *a, struct nfs_client *b)
>> +nfs4_check_clientid_trunking(struct nfs_client *a, struct nfs_client *b)
>> {
>> struct nfs41_server_owner *o1 = a->cl_serverowner;
>> struct nfs41_server_owner *o2 = b->cl_serverowner;
>>
>> - if (o1->minor_id != o2->minor_id) {
>> - dprintk("NFS: --> %s server owner minor IDs do not match\n",
>> - __func__);
>> - return false;
>> - }
>> -
>> if (o1->major_id_sz != o2->major_id_sz)
>> goto out_major_mismatch;
>> if (memcmp(o1->major_id, o2->major_id, o1->major_id_sz) != 0)
>> @@ -654,7 +648,12 @@ int nfs41_walk_client_list(struct nfs_client *new,
>> if (!nfs4_match_clientids(pos, new))
>> continue;
>>
>> - if (!nfs4_match_serverowners(pos, new))
>> + /*
>> + * Note that session trunking is just a special subcase of
>> + * client id trunking. In either case, we want to fall back
>> + * to using the existing nfs_client.
>> + */
>> + if (!nfs4_check_clientid_trunking(pos, new))
>> continue;
>>
>> atomic_inc(&pos->cl_count);
>> --
>> 2.1.0
>>
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Jan 2, 2015, at 5:09 PM, Trond Myklebust <[email protected]> wrote:
> On Fri, Jan 2, 2015 at 4:44 PM, Chuck Lever <[email protected]> wrote:
>>
>> On Jan 2, 2015, at 4:39 PM, Trond Myklebust <[email protected]> wrote:
>>
>>> Currently, our trunking code will check for session trunking, but will
>>> fail to detect client id trunking. This is a problem, because it means
>>> that the client will fail to recognise that the two connections represent
>>> shared state, even if they do not permit a shared session.
>>> By removing the check for the server minor id, and only checking the
>>> major id, we will end up doing the right thing in both cases: we close
>>> down the new nfs_client and fall back to using the existing one.
>>
>> Fair enough.
>>
>> Does this detect the case where two concurrent NFSv4.1 mounts
>> of the same server use different transport protocols?
>
> I'm not sure I understand what you mean. The client should completely
> ignore the transport protocol when doing trunking detection.
Quite right, the problem seems to be earlier than that.
nfs_match_client() checks the transport protocol setting, and
forces creation of a new struct nfs_client if the transport
protocol is not the same as an existing and otherwise compatible
struct nfs_client for the server being mounted.
OK for NFSv2 and NFSv3, which can have a unique struct nfs_clients
each for UDP, TCP, and RDMA mounts.
For NFSv4, RDMA means the nfs_match_client() check forces the
creation of a separate struct nfs_client for RDMA and TCP (if
admin requests both types of mounts).
An RDMA connection and a TCP connection would be created. Both
connections would send the same nfs_client_id4.
This doesn?t seem to be an issue for non-UCS NFSv4.0, but it
probably is a problem with UCS.
> The only
> thing that it cares about is whether or not the server believes we
> should be sharing state, in which case the client should fall back to
> using the existing connection for the new mount point.
While working on the NFSv4.1 on RDMA prototype, I noticed that
trunking detection would allow concurrent mounts of the same
server with both RDMA and TCP transports. My hack-neyed attempt
to fix that was posted back in October:
http://marc.info/?l=linux-nfs&m=141348837927749&w=2
Your patch very likely addresses the issue for NFSv4.1, but I
wonder how to do it for NFSv4.0 with UCS (if it is a problem,
at the time I think I only checked NFSv4.1 behavior).
>>> Fixes: 05f4c350ee02e ("NFS: Discover NFSv4 server trunking when mounting")
>>> Cc: Chuck Lever <[email protected]>
>>> Cc: [email protected] # 3.7.x
>>> Signed-off-by: Trond Myklebust <[email protected]>
>>> ---
>>> fs/nfs/nfs4client.c | 17 ++++++++---------
>>> 1 file changed, 8 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
>>> index 03311259b0c4..d949d0f378ec 100644
>>> --- a/fs/nfs/nfs4client.c
>>> +++ b/fs/nfs/nfs4client.c
>>> @@ -566,20 +566,14 @@ static bool nfs4_match_clientids(struct nfs_client *a, struct nfs_client *b)
>>> }
>>>
>>> /*
>>> - * Returns true if the server owners match
>>> + * Returns true if the server major ids match
>>> */
>>> static bool
>>> -nfs4_match_serverowners(struct nfs_client *a, struct nfs_client *b)
>>> +nfs4_check_clientid_trunking(struct nfs_client *a, struct nfs_client *b)
>>> {
>>> struct nfs41_server_owner *o1 = a->cl_serverowner;
>>> struct nfs41_server_owner *o2 = b->cl_serverowner;
>>>
>>> - if (o1->minor_id != o2->minor_id) {
>>> - dprintk("NFS: --> %s server owner minor IDs do not match\n",
>>> - __func__);
>>> - return false;
>>> - }
>>> -
>>> if (o1->major_id_sz != o2->major_id_sz)
>>> goto out_major_mismatch;
>>> if (memcmp(o1->major_id, o2->major_id, o1->major_id_sz) != 0)
>>> @@ -654,7 +648,12 @@ int nfs41_walk_client_list(struct nfs_client *new,
>>> if (!nfs4_match_clientids(pos, new))
>>> continue;
>>>
>>> - if (!nfs4_match_serverowners(pos, new))
>>> + /*
>>> + * Note that session trunking is just a special subcase of
>>> + * client id trunking. In either case, we want to fall back
>>> + * to using the existing nfs_client.
>>> + */
>>> + if (!nfs4_check_clientid_trunking(pos, new))
>>> continue;
>>>
>>> atomic_inc(&pos->cl_count);
>>> --
>>> 2.1.0
>>>
>>
>> --
>> Chuck Lever
>> chuck[dot]lever[at]oracle[dot]com
>>
>>
>>
>
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> [email protected]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, Jan 2, 2015 at 6:36 PM, Chuck Lever <[email protected]> wrote:
>
> On Jan 2, 2015, at 5:09 PM, Trond Myklebust <[email protected]> wrote:
>
>> On Fri, Jan 2, 2015 at 4:44 PM, Chuck Lever <[email protected]> wrote:
>>>
>>> On Jan 2, 2015, at 4:39 PM, Trond Myklebust <[email protected]> wrote:
>>>
>>>> Currently, our trunking code will check for session trunking, but will
>>>> fail to detect client id trunking. This is a problem, because it means
>>>> that the client will fail to recognise that the two connections represent
>>>> shared state, even if they do not permit a shared session.
>>>> By removing the check for the server minor id, and only checking the
>>>> major id, we will end up doing the right thing in both cases: we close
>>>> down the new nfs_client and fall back to using the existing one.
>>>
>>> Fair enough.
>>>
>>> Does this detect the case where two concurrent NFSv4.1 mounts
>>> of the same server use different transport protocols?
>>
>> I'm not sure I understand what you mean. The client should completely
>> ignore the transport protocol when doing trunking detection.
>
> Quite right, the problem seems to be earlier than that.
>
> nfs_match_client() checks the transport protocol setting, and
> forces creation of a new struct nfs_client if the transport
> protocol is not the same as an existing and otherwise compatible
> struct nfs_client for the server being mounted.
>
> OK for NFSv2 and NFSv3, which can have a unique struct nfs_clients
> each for UDP, TCP, and RDMA mounts.
>
> For NFSv4, RDMA means the nfs_match_client() check forces the
> creation of a separate struct nfs_client for RDMA and TCP (if
> admin requests both types of mounts).
>
> An RDMA connection and a TCP connection would be created. Both
> connections would send the same nfs_client_id4.
>
> This doesn’t seem to be an issue for non-UCS NFSv4.0, but it
> probably is a problem with UCS.
The problem isn't so much the check in nfs_match_client(). The problem
is rather those same checks in nfs4[01]_walk_client_list().
>> The only
>> thing that it cares about is whether or not the server believes we
>> should be sharing state, in which case the client should fall back to
>> using the existing connection for the new mount point.
>
> While working on the NFSv4.1 on RDMA prototype, I noticed that
> trunking detection would allow concurrent mounts of the same
> server with both RDMA and TCP transports. My hack-neyed attempt
> to fix that was posted back in October:
>
> http://marc.info/?l=linux-nfs&m=141348837927749&w=2
>
> Your patch very likely addresses the issue for NFSv4.1, but I
> wonder how to do it for NFSv4.0 with UCS (if it is a problem,
> at the time I think I only checked NFSv4.1 behavior).
There is a third problem, and that is what if someone plays with
-omigration or /sys/module/nfs/parameters/nfs4_unique_id after the
server is already mounted. We know that the server will not identify
us as being the same client in that case, and so we should try to
ascertain that we don't create any false positives.
The new patchset (v2) therefore attempts to address all of the above issues.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]
On Jan 2, 2015, at 4:39 PM, Trond Myklebust <[email protected]> wrote:
> Currently, our trunking code will check for session trunking, but will
> fail to detect client id trunking. This is a problem, because it means
> that the client will fail to recognise that the two connections represent
> shared state, even if they do not permit a shared session.
> By removing the check for the server minor id, and only checking the
> major id, we will end up doing the right thing in both cases: we close
> down the new nfs_client and fall back to using the existing one.
>
> Fixes: 05f4c350ee02e ("NFS: Discover NFSv4 server trunking when mounting")
> Cc: Chuck Lever <[email protected]>
> Cc: [email protected] # 3.7.x
> Signed-off-by: Trond Myklebust <[email protected]>
You can add my Tested-by or Reviewed-by if you like.
> ---
> fs/nfs/nfs4client.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
> index 03311259b0c4..d949d0f378ec 100644
> --- a/fs/nfs/nfs4client.c
> +++ b/fs/nfs/nfs4client.c
> @@ -566,20 +566,14 @@ static bool nfs4_match_clientids(struct nfs_client *a, struct nfs_client *b)
> }
>
> /*
> - * Returns true if the server owners match
> + * Returns true if the server major ids match
> */
> static bool
> -nfs4_match_serverowners(struct nfs_client *a, struct nfs_client *b)
> +nfs4_check_clientid_trunking(struct nfs_client *a, struct nfs_client *b)
> {
> struct nfs41_server_owner *o1 = a->cl_serverowner;
> struct nfs41_server_owner *o2 = b->cl_serverowner;
>
> - if (o1->minor_id != o2->minor_id) {
> - dprintk("NFS: --> %s server owner minor IDs do not match\n",
> - __func__);
> - return false;
> - }
> -
> if (o1->major_id_sz != o2->major_id_sz)
> goto out_major_mismatch;
> if (memcmp(o1->major_id, o2->major_id, o1->major_id_sz) != 0)
> @@ -654,7 +648,12 @@ int nfs41_walk_client_list(struct nfs_client *new,
> if (!nfs4_match_clientids(pos, new))
> continue;
>
> - if (!nfs4_match_serverowners(pos, new))
> + /*
> + * Note that session trunking is just a special subcase of
> + * client id trunking. In either case, we want to fall back
> + * to using the existing nfs_client.
> + */
> + if (!nfs4_check_clientid_trunking(pos, new))
> continue;
>
> atomic_inc(&pos->cl_count);
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com