On 08/25/2011 04:46 PM, Chuck Lever wrote:
>
> On Aug 25, 2011, at 4:25 PM, Steve Dickson wrote:
>
>> First of all let me apologize for taking so long
>> to get to this... I did take some time off....
>>
>> On 08/06/2011 06:11 AM, Luk Claes wrote:
>>> If NFS port (2049) is supplied explicitly, don't ignore this setting by requesting it to portmapper again. Thanks to Ben Hutchings <ben@xxxxxxxxxxxxxxx> for the patch.
>>>
>>> Signed-off-by: Luk Claes <luk@xxxxxxxxxx>
>>> ---
>>> utils/mount/stropts.c | 4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index f1aa503..8b2799c 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -437,8 +437,8 @@ static int nfs_construct_new_options(struct mount_options *options,
>>> if (po_append(options, new_option) == PO_FAILED)
>>> return 0;
>>>
>>> - po_remove_all(options, "port");
>>> - if (nfs_pmap->pm_port != NFS_PORT) {
>>> + if(po_remove_all(options, "port") == PO_FOUND ||
>>> + nfs_pmap->pm_port != NFS_PORT) {
>>> snprintf(new_option, sizeof(new_option) - 1,
>>> "port=%lu", nfs_pmap->pm_port);
>>> if (po_append(options, new_option) == PO_FAILED)
>> Now I like the idea of not sending the pmap inquire for the
>> port when the port is specified on the command because its
>> a TCP connection. During automount storms, wasting TCP connections
>> is not a good thing...
>>
>> But your patch does not seem to do that since all the port
>> negotiation is done well before this part of the code.
>
> I thought the idea was to eliminate the port query during REnegotiation
> if "port=" was specified. That seems like exactly what the patch does.
Ah, I did miss the renegotiation aspect... Its been a long day... ;-)
> Does the initial port negotiation also have this problem?
Well I was thinking why even do the initial port negotiation when
the port= is set...
>
> But we should be careful here: mount.nfs will do an rpcbind query if a port=
> was specified if there is also some doubt about whether to use TCP or UDP,
> or what NFS version is available. The only time rpcbind queries should be
> completely squashed is when the mount parameters are specified completely
> (transport, version, and port).
I agree with being careful here... But if some admin is specifying
a particular port I'm thinking one, they have a clue and two, 99% of
time the port is correct. So lets just verify the port by using it
NFS ping. So I'm thinking %99 of the time there is no need to create
that second TCP connection when a port is specified.
But, here is were we have to be careful. That 1% of the
time the ping fails, for whatever reason... That is when I think
we to consult with the server's rpcbind and the second TCP
connection is justified.
I just thinking making that assuming the specified info
is as good way to save a TCP connection...
>
>> I'm thinking some code has to change in nfs_probe_port()
>> something like:
>>
>> mount.nfs: Do not send pmap inquire when port is specified
>>
>> When the port is specified on the command line do not
>> send a pmap inquire asking for the port. Instead use
>> the given port in the NFS ping. If the ping fails,
>> assume a bad port was given and now go ask the server
>> for the correct port.
>
> If the server has more than one NFS port enabled, and the client is
> requesting a port that isn't registered with the server's rpcbind, it
> shouldn't fall back to the registered port IMO.
True. With this patch there is no fall back. When the NFS ping fails,
the registered port is obtained; compared to the specified port.
If they are different the mount will fail as it does to today.
steved.
>
>> Signed-off-by: Steve Dickson <steved@xxxxxxxxxx>
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index d1f91dc..405c320 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -545,17 +545,18 @@ static int nfs_probe_port(const struct sockaddr *sap, const socklen_t salen,
>> const unsigned int prot = (u_int)pmap->pm_prot, *p_prot;
>> const u_short port = (u_short) pmap->pm_port;
>> unsigned long vers = pmap->pm_vers;
>> - unsigned short p_port;
>> + unsigned short p_port = port;
>> + int once = 1;
>>
>> memcpy(saddr, sap, salen);
>> p_prot = prot ? &prot : protos;
>> p_vers = vers ? &vers : versions;
>> -
>> for (;;) {
>> if (verbose)
>> printf(_("%s: prog %lu, trying vers=%lu, prot=%u\n"),
>> progname, prog, *p_vers, *p_prot);
>> - p_port = nfs_getport(saddr, salen, prog, *p_vers, *p_prot);
>> + if (!p_port)
>> + p_port = nfs_getport(saddr, salen, prog, *p_vers, *p_prot);
>> if (p_port) {
>> if (!port || port == p_port) {
>> nfs_set_port(saddr, p_port);
>> @@ -564,6 +565,15 @@ static int nfs_probe_port(const struct sockaddr *sap, const socklen_t salen,
>> if (nfs_rpc_ping(saddr, salen, prog,
>> *p_vers, *p_prot, NULL))
>> goto out_ok;
>> + if (port == p_port && once) {
>> + /*
>> + * Could be a bad port was specified. This
>> + * time ask the server for the port but only
>> + * do it once.
>> + */
>> + p_port = once = 0;
>> + continue;
>> + }
>> } else
>> rpc_createerr.cf_stat = RPC_PROGNOTREGISTERED;
>> }
>> @@ -589,6 +599,7 @@ static int nfs_probe_port(const struct sockaddr *sap, const socklen_t salen,
>> }
>>
>> nfs_pp_debug2("failed");
>> return 0;
>>
>> out_ok:
>>
>>
>> I'm not sure this is best way either....
>>
>> steved.
>>
>>
>>
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
