diff mbox

[06/14] NFS: Treat NFS4ERR_CLID_INUSE as a fatal error

Message ID 20120709154435.1604.50788.stgit@degas.1015granger.net (mailing list archive)
State New, archived
Headers show

Commit Message

Chuck Lever July 9, 2012, 3:44 p.m. UTC 
For NFSv4 minor version 0, currently the cl_id_uniquifier allows the
Linux client to generate a unique nfs_client_id4 string whenever a
server replies with NFS4ERR_CLID_INUSE.

This implementation seems to be based on a flawed reading of RFC
3530.  NFS4ERR_CLID_INUSE actually means that the client has presented
this nfs_client_id4 string with a different principal at some time in
the past, and that lease is still in use on the server.

For a Linux client this might be rather difficult to achieve: the
authentication flavor is named right in the nfs_client_id4.id
string.  If we change flavors, we change strings automatically.

So, practically speaking, NFS4ERR_CLID_INUSE means there is some other
client using our string.  There is not much that can be done to
recover automatically.  Let's make it a permanent error.

Remove the recovery logic in nfs4_proc_setclientid(), and remove the
cl_id_uniquifier field from the nfs_client data structure.  And,
remove the authentication flavor from the nfs_client_id4 string.

Keeping the authentication flavor in the nfs_client_id4.id string
means that we could have a separate lease for each authentication
flavor used by mounts on the client.  But we want just one lease for
all the mounts on this client.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 fs/nfs/nfs4proc.c         |   54 ++++++++++++++++++---------------------------
 fs/nfs/nfs4state.c        |    7 +++++-
 include/linux/nfs_fs_sb.h |    3 +--
 3 files changed, 29 insertions(+), 35 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Trond Myklebust July 9, 2012, 8:10 p.m. UTC | #1 
On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote:
> For NFSv4 minor version 0, currently the cl_id_uniquifier allows the

> Linux client to generate a unique nfs_client_id4 string whenever a

> server replies with NFS4ERR_CLID_INUSE.

> 

> This implementation seems to be based on a flawed reading of RFC

> 3530.  NFS4ERR_CLID_INUSE actually means that the client has presented

> this nfs_client_id4 string with a different principal at some time in

> the past, and that lease is still in use on the server.

> 

> For a Linux client this might be rather difficult to achieve: the

> authentication flavor is named right in the nfs_client_id4.id

> string.  If we change flavors, we change strings automatically.

> 

> So, practically speaking, NFS4ERR_CLID_INUSE means there is some other

> client using our string.  There is not much that can be done to

> recover automatically.  Let's make it a permanent error.

> 

> Remove the recovery logic in nfs4_proc_setclientid(), and remove the

> cl_id_uniquifier field from the nfs_client data structure.  And,

> remove the authentication flavor from the nfs_client_id4 string.

> 

> Keeping the authentication flavor in the nfs_client_id4.id string

> means that we could have a separate lease for each authentication

> flavor used by mounts on the client.  But we want just one lease for

> all the mounts on this client.

> 

> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

> ---

> 

>  fs/nfs/nfs4proc.c         |   54 ++++++++++++++++++---------------------------

>  fs/nfs/nfs4state.c        |    7 +++++-

>  include/linux/nfs_fs_sb.h |    3 +--

>  3 files changed, 29 insertions(+), 35 deletions(-)

> 

> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c

> index 7adb705..63a3cf0 100644

> --- a/fs/nfs/nfs4proc.c

> +++ b/fs/nfs/nfs4proc.c

> @@ -259,7 +259,12 @@ static int nfs4_wait_clnt_recover(struct nfs_client *clp)

>  

>  	res = wait_on_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,

>  			nfs_wait_bit_killable, TASK_KILLABLE);

> -	return res;

> +	if (res)

> +		return res;

> +

> +	if (clp->cl_cons_state < 0)

> +		return clp->cl_cons_state;

> +	return 0;

>  }


A) This should be a separate patch.

B) Why is it needed in the first place? All places that call
nfs4_wait_clnt_recover lie outside the mount code path.


>  static int nfs4_delay(struct rpc_clnt *clnt, long *timeout)

> @@ -4038,42 +4043,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,

>  		.rpc_resp = res,

>  		.rpc_cred = cred,

>  	};

> -	int loop = 0;

> -	int status;

>  

> +	/* nfs_client_id4 */

>  	nfs4_init_boot_verifier(clp, &sc_verifier);

> -

> -	for(;;) {

> -		rcu_read_lock();

> -		setclientid.sc_name_len = scnprintf(setclientid.sc_name,

> -				sizeof(setclientid.sc_name), "%s/%s %s %s %u",

> -				clp->cl_ipaddr,

> -				rpc_peeraddr2str(clp->cl_rpcclient,

> -							RPC_DISPLAY_ADDR),

> -				rpc_peeraddr2str(clp->cl_rpcclient,

> -							RPC_DISPLAY_PROTO),

> -				clp->cl_rpcclient->cl_auth->au_ops->au_name,

> -				clp->cl_id_uniquifier);

> -		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,

> +	rcu_read_lock();

> +	setclientid.sc_name_len = scnprintf(setclientid.sc_name,

> +			sizeof(setclientid.sc_name), "%s/%s %s",

> +			clp->cl_ipaddr,

> +			rpc_peeraddr2str(clp->cl_rpcclient,

> +						RPC_DISPLAY_ADDR),

> +			rpc_peeraddr2str(clp->cl_rpcclient,

> +						RPC_DISPLAY_PROTO));

> +	/* cb_client4 */

> +	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,

>  				sizeof(setclientid.sc_netid),

>  				rpc_peeraddr2str(clp->cl_rpcclient,

>  							RPC_DISPLAY_NETID));

> -		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,

> +	rcu_read_unlock();

> +	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,

>  				sizeof(setclientid.sc_uaddr), "%s.%u.%u",

>  				clp->cl_ipaddr, port >> 8, port & 255);

> -		rcu_read_unlock();

>  

> -		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);

> -		if (status != -NFS4ERR_CLID_INUSE)

> -			break;

> -		if (loop != 0) {

> -			++clp->cl_id_uniquifier;

> -			break;

> -		}

> -		++loop;

> -		ssleep(clp->cl_lease_time / HZ + 1);

> -	}

> -	return status;

> +	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);

>  }

>  

>  int nfs4_proc_setclientid_confirm(struct nfs_client *clp,

> @@ -5275,10 +5266,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)

>  	nfs4_init_boot_verifier(clp, &verifier);

>  

>  	args.id_len = scnprintf(args.id, sizeof(args.id),

> -				"%s/%s/%u",

> +				"%s/%s",

>  				clp->cl_ipaddr,

> -				clp->cl_rpcclient->cl_nodename,

> -				clp->cl_rpcclient->cl_auth->au_flavor);

> +				clp->cl_rpcclient->cl_nodename);

>  

>  	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),

>  					GFP_NOFS);

> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c

> index f38300e..a2c1153 100644

> --- a/fs/nfs/nfs4state.c

> +++ b/fs/nfs/nfs4state.c

> @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)

>  			return -ESERVERFAULT;

>  		/* Lease confirmation error: retry after purging the lease */

>  		ssleep(1);

> -	case -NFS4ERR_CLID_INUSE:

>  	case -NFS4ERR_STALE_CLIENTID:

>  		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);

>  		break;

> +	case -NFS4ERR_CLID_INUSE:

> +		printk(KERN_ERR "NFS: Server %s: client ID already in use\n",

> +			clp->cl_hostname);

> +		nfs_mark_client_ready(clp, -EPERM);

> +		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);

> +		return -EPERM;


How do we guarantee that this won't occur on an existing mount?

>  	case -EACCES:

>  		if (clp->cl_machine_cred == NULL)

>  			return -EACCES;

> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h

> index f58325a..6532765 100644

> --- a/include/linux/nfs_fs_sb.h

> +++ b/include/linux/nfs_fs_sb.h

> @@ -69,10 +69,9 @@ struct nfs_client {

>  	struct idmap *		cl_idmap;

>  

>  	/* Our own IP address, as a null-terminated string.

> -	 * This is used to generate the clientid, and the callback address.

> +	 * This is used to generate the mv0 callback address.

>  	 */

>  	char			cl_ipaddr[48];

> -	unsigned char		cl_id_uniquifier;

>  	u32			cl_cb_ident;	/* v4.0 callback identifier */

>  	const struct nfs4_minor_version_ops *cl_mvops;

>  

> 


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com
Chuck Lever July 9, 2012, 8:12 p.m. UTC | #2 
On Jul 9, 2012, at 4:10 PM, Myklebust, Trond wrote:

> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote:
>> For NFSv4 minor version 0, currently the cl_id_uniquifier allows the
>> Linux client to generate a unique nfs_client_id4 string whenever a
>> server replies with NFS4ERR_CLID_INUSE.
>> 
>> This implementation seems to be based on a flawed reading of RFC
>> 3530.  NFS4ERR_CLID_INUSE actually means that the client has presented
>> this nfs_client_id4 string with a different principal at some time in
>> the past, and that lease is still in use on the server.
>> 
>> For a Linux client this might be rather difficult to achieve: the
>> authentication flavor is named right in the nfs_client_id4.id
>> string.  If we change flavors, we change strings automatically.
>> 
>> So, practically speaking, NFS4ERR_CLID_INUSE means there is some other
>> client using our string.  There is not much that can be done to
>> recover automatically.  Let's make it a permanent error.
>> 
>> Remove the recovery logic in nfs4_proc_setclientid(), and remove the
>> cl_id_uniquifier field from the nfs_client data structure.  And,
>> remove the authentication flavor from the nfs_client_id4 string.
>> 
>> Keeping the authentication flavor in the nfs_client_id4.id string
>> means that we could have a separate lease for each authentication
>> flavor used by mounts on the client.  But we want just one lease for
>> all the mounts on this client.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> 
>> fs/nfs/nfs4proc.c         |   54 ++++++++++++++++++---------------------------
>> fs/nfs/nfs4state.c        |    7 +++++-
>> include/linux/nfs_fs_sb.h |    3 +--
>> 3 files changed, 29 insertions(+), 35 deletions(-)
>> 
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 7adb705..63a3cf0 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -259,7 +259,12 @@ static int nfs4_wait_clnt_recover(struct nfs_client *clp)
>> 
>> 	res = wait_on_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,
>> 			nfs_wait_bit_killable, TASK_KILLABLE);
>> -	return res;
>> +	if (res)
>> +		return res;
>> +
>> +	if (clp->cl_cons_state < 0)
>> +		return clp->cl_cons_state;
>> +	return 0;
>> }
> 
> A) This should be a separate patch.

OK.

> 
> B) Why is it needed in the first place? All places that call
> nfs4_wait_clnt_recover lie outside the mount code path.

The problem is that is no longer the case once we have trunking discovery in the mount path.

We discussed this.  This is what you told me to do.

>> static int nfs4_delay(struct rpc_clnt *clnt, long *timeout)
>> @@ -4038,42 +4043,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
>> 		.rpc_resp = res,
>> 		.rpc_cred = cred,
>> 	};
>> -	int loop = 0;
>> -	int status;
>> 
>> +	/* nfs_client_id4 */
>> 	nfs4_init_boot_verifier(clp, &sc_verifier);
>> -
>> -	for(;;) {
>> -		rcu_read_lock();
>> -		setclientid.sc_name_len = scnprintf(setclientid.sc_name,
>> -				sizeof(setclientid.sc_name), "%s/%s %s %s %u",
>> -				clp->cl_ipaddr,
>> -				rpc_peeraddr2str(clp->cl_rpcclient,
>> -							RPC_DISPLAY_ADDR),
>> -				rpc_peeraddr2str(clp->cl_rpcclient,
>> -							RPC_DISPLAY_PROTO),
>> -				clp->cl_rpcclient->cl_auth->au_ops->au_name,
>> -				clp->cl_id_uniquifier);
>> -		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
>> +	rcu_read_lock();
>> +	setclientid.sc_name_len = scnprintf(setclientid.sc_name,
>> +			sizeof(setclientid.sc_name), "%s/%s %s",
>> +			clp->cl_ipaddr,
>> +			rpc_peeraddr2str(clp->cl_rpcclient,
>> +						RPC_DISPLAY_ADDR),
>> +			rpc_peeraddr2str(clp->cl_rpcclient,
>> +						RPC_DISPLAY_PROTO));
>> +	/* cb_client4 */
>> +	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
>> 				sizeof(setclientid.sc_netid),
>> 				rpc_peeraddr2str(clp->cl_rpcclient,
>> 							RPC_DISPLAY_NETID));
>> -		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
>> +	rcu_read_unlock();
>> +	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
>> 				sizeof(setclientid.sc_uaddr), "%s.%u.%u",
>> 				clp->cl_ipaddr, port >> 8, port & 255);
>> -		rcu_read_unlock();
>> 
>> -		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>> -		if (status != -NFS4ERR_CLID_INUSE)
>> -			break;
>> -		if (loop != 0) {
>> -			++clp->cl_id_uniquifier;
>> -			break;
>> -		}
>> -		++loop;
>> -		ssleep(clp->cl_lease_time / HZ + 1);
>> -	}
>> -	return status;
>> +	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>> }
>> 
>> int nfs4_proc_setclientid_confirm(struct nfs_client *clp,
>> @@ -5275,10 +5266,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
>> 	nfs4_init_boot_verifier(clp, &verifier);
>> 
>> 	args.id_len = scnprintf(args.id, sizeof(args.id),
>> -				"%s/%s/%u",
>> +				"%s/%s",
>> 				clp->cl_ipaddr,
>> -				clp->cl_rpcclient->cl_nodename,
>> -				clp->cl_rpcclient->cl_auth->au_flavor);
>> +				clp->cl_rpcclient->cl_nodename);
>> 
>> 	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),
>> 					GFP_NOFS);
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index f38300e..a2c1153 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)
>> 			return -ESERVERFAULT;
>> 		/* Lease confirmation error: retry after purging the lease */
>> 		ssleep(1);
>> -	case -NFS4ERR_CLID_INUSE:
>> 	case -NFS4ERR_STALE_CLIENTID:
>> 		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>> 		break;
>> +	case -NFS4ERR_CLID_INUSE:
>> +		printk(KERN_ERR "NFS: Server %s: client ID already in use\n",
>> +			clp->cl_hostname);
>> +		nfs_mark_client_ready(clp, -EPERM);
>> +		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>> +		return -EPERM;
> 
> How do we guarantee that this won't occur on an existing mount?
> 
>> 	case -EACCES:
>> 		if (clp->cl_machine_cred == NULL)
>> 			return -EACCES;
>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>> index f58325a..6532765 100644
>> --- a/include/linux/nfs_fs_sb.h
>> +++ b/include/linux/nfs_fs_sb.h
>> @@ -69,10 +69,9 @@ struct nfs_client {
>> 	struct idmap *		cl_idmap;
>> 
>> 	/* Our own IP address, as a null-terminated string.
>> -	 * This is used to generate the clientid, and the callback address.
>> +	 * This is used to generate the mv0 callback address.
>> 	 */
>> 	char			cl_ipaddr[48];
>> -	unsigned char		cl_id_uniquifier;
>> 	u32			cl_cb_ident;	/* v4.0 callback identifier */
>> 	const struct nfs4_minor_version_ops *cl_mvops;
>> 
>> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Chuck Lever July 9, 2012, 8:25 p.m. UTC | #3 
On Jul 9, 2012, at 4:12 PM, Chuck Lever wrote:

> 
> On Jul 9, 2012, at 4:10 PM, Myklebust, Trond wrote:
> 
>> On Mon, 2012-07-09 at 11:44 -0400, Chuck Lever wrote:
>>> For NFSv4 minor version 0, currently the cl_id_uniquifier allows the
>>> Linux client to generate a unique nfs_client_id4 string whenever a
>>> server replies with NFS4ERR_CLID_INUSE.
>>> 
>>> This implementation seems to be based on a flawed reading of RFC
>>> 3530.  NFS4ERR_CLID_INUSE actually means that the client has presented
>>> this nfs_client_id4 string with a different principal at some time in
>>> the past, and that lease is still in use on the server.
>>> 
>>> For a Linux client this might be rather difficult to achieve: the
>>> authentication flavor is named right in the nfs_client_id4.id
>>> string.  If we change flavors, we change strings automatically.
>>> 
>>> So, practically speaking, NFS4ERR_CLID_INUSE means there is some other
>>> client using our string.  There is not much that can be done to
>>> recover automatically.  Let's make it a permanent error.
>>> 
>>> Remove the recovery logic in nfs4_proc_setclientid(), and remove the
>>> cl_id_uniquifier field from the nfs_client data structure.  And,
>>> remove the authentication flavor from the nfs_client_id4 string.
>>> 
>>> Keeping the authentication flavor in the nfs_client_id4.id string
>>> means that we could have a separate lease for each authentication
>>> flavor used by mounts on the client.  But we want just one lease for
>>> all the mounts on this client.
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> 
>>> fs/nfs/nfs4proc.c         |   54 ++++++++++++++++++---------------------------
>>> fs/nfs/nfs4state.c        |    7 +++++-
>>> include/linux/nfs_fs_sb.h |    3 +--
>>> 3 files changed, 29 insertions(+), 35 deletions(-)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index 7adb705..63a3cf0 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -259,7 +259,12 @@ static int nfs4_wait_clnt_recover(struct nfs_client *clp)
>>> 
>>> 	res = wait_on_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,
>>> 			nfs_wait_bit_killable, TASK_KILLABLE);
>>> -	return res;
>>> +	if (res)
>>> +		return res;
>>> +
>>> +	if (clp->cl_cons_state < 0)
>>> +		return clp->cl_cons_state;
>>> +	return 0;
>>> }
>> 
>> A) This should be a separate patch.
> 
> OK.
> 
>> 
>> B) Why is it needed in the first place? All places that call
>> nfs4_wait_clnt_recover lie outside the mount code path.
> 
> The problem is that is no longer the case once we have trunking discovery in the mount path.

Alright, that's not the problem.  The problem is we decided to make CLID_INUSE a permanent error during state recovery.  If the state manager doesn't cause waiting operations to fail, then we get an infinite loop, right?

> We discussed this.  This is what you told me to do.
> 
>>> static int nfs4_delay(struct rpc_clnt *clnt, long *timeout)
>>> @@ -4038,42 +4043,28 @@ int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
>>> 		.rpc_resp = res,
>>> 		.rpc_cred = cred,
>>> 	};
>>> -	int loop = 0;
>>> -	int status;
>>> 
>>> +	/* nfs_client_id4 */
>>> 	nfs4_init_boot_verifier(clp, &sc_verifier);
>>> -
>>> -	for(;;) {
>>> -		rcu_read_lock();
>>> -		setclientid.sc_name_len = scnprintf(setclientid.sc_name,
>>> -				sizeof(setclientid.sc_name), "%s/%s %s %s %u",
>>> -				clp->cl_ipaddr,
>>> -				rpc_peeraddr2str(clp->cl_rpcclient,
>>> -							RPC_DISPLAY_ADDR),
>>> -				rpc_peeraddr2str(clp->cl_rpcclient,
>>> -							RPC_DISPLAY_PROTO),
>>> -				clp->cl_rpcclient->cl_auth->au_ops->au_name,
>>> -				clp->cl_id_uniquifier);
>>> -		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
>>> +	rcu_read_lock();
>>> +	setclientid.sc_name_len = scnprintf(setclientid.sc_name,
>>> +			sizeof(setclientid.sc_name), "%s/%s %s",
>>> +			clp->cl_ipaddr,
>>> +			rpc_peeraddr2str(clp->cl_rpcclient,
>>> +						RPC_DISPLAY_ADDR),
>>> +			rpc_peeraddr2str(clp->cl_rpcclient,
>>> +						RPC_DISPLAY_PROTO));
>>> +	/* cb_client4 */
>>> +	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
>>> 				sizeof(setclientid.sc_netid),
>>> 				rpc_peeraddr2str(clp->cl_rpcclient,
>>> 							RPC_DISPLAY_NETID));
>>> -		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
>>> +	rcu_read_unlock();
>>> +	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
>>> 				sizeof(setclientid.sc_uaddr), "%s.%u.%u",
>>> 				clp->cl_ipaddr, port >> 8, port & 255);
>>> -		rcu_read_unlock();
>>> 
>>> -		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>>> -		if (status != -NFS4ERR_CLID_INUSE)
>>> -			break;
>>> -		if (loop != 0) {
>>> -			++clp->cl_id_uniquifier;
>>> -			break;
>>> -		}
>>> -		++loop;
>>> -		ssleep(clp->cl_lease_time / HZ + 1);
>>> -	}
>>> -	return status;
>>> +	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
>>> }
>>> 
>>> int nfs4_proc_setclientid_confirm(struct nfs_client *clp,
>>> @@ -5275,10 +5266,9 @@ int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
>>> 	nfs4_init_boot_verifier(clp, &verifier);
>>> 
>>> 	args.id_len = scnprintf(args.id, sizeof(args.id),
>>> -				"%s/%s/%u",
>>> +				"%s/%s",
>>> 				clp->cl_ipaddr,
>>> -				clp->cl_rpcclient->cl_nodename,
>>> -				clp->cl_rpcclient->cl_auth->au_flavor);
>>> +				clp->cl_rpcclient->cl_nodename);
>>> 
>>> 	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),
>>> 					GFP_NOFS);
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index f38300e..a2c1153 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -1606,10 +1606,15 @@ static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)
>>> 			return -ESERVERFAULT;
>>> 		/* Lease confirmation error: retry after purging the lease */
>>> 		ssleep(1);
>>> -	case -NFS4ERR_CLID_INUSE:
>>> 	case -NFS4ERR_STALE_CLIENTID:
>>> 		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>> 		break;
>>> +	case -NFS4ERR_CLID_INUSE:
>>> +		printk(KERN_ERR "NFS: Server %s: client ID already in use\n",
>>> +			clp->cl_hostname);
>>> +		nfs_mark_client_ready(clp, -EPERM);
>>> +		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
>>> +		return -EPERM;
>> 
>> How do we guarantee that this won't occur on an existing mount?
>> 
>>> 	case -EACCES:
>>> 		if (clp->cl_machine_cred == NULL)
>>> 			return -EACCES;
>>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
>>> index f58325a..6532765 100644
>>> --- a/include/linux/nfs_fs_sb.h
>>> +++ b/include/linux/nfs_fs_sb.h
>>> @@ -69,10 +69,9 @@ struct nfs_client {
>>> 	struct idmap *		cl_idmap;
>>> 
>>> 	/* Our own IP address, as a null-terminated string.
>>> -	 * This is used to generate the clientid, and the callback address.
>>> +	 * This is used to generate the mv0 callback address.
>>> 	 */
>>> 	char			cl_ipaddr[48];
>>> -	unsigned char		cl_id_uniquifier;
>>> 	u32			cl_cb_ident;	/* v4.0 callback identifier */
>>> 	const struct nfs4_minor_version_ops *cl_mvops;
>>> 
>>> 
>> 
>> -- 
>> Trond Myklebust
>> Linux NFS client maintainer
>> 
>> NetApp
>> Trond.Myklebust@netapp.com
>> www.netapp.com
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Trond Myklebust July 9, 2012, 8:34 p.m. UTC | #4 
T24gTW9uLCAyMDEyLTA3LTA5IGF0IDE2OjEyIC0wNDAwLCBDaHVjayBMZXZlciB3cm90ZToNCj4g
T24gSnVsIDksIDIwMTIsIGF0IDQ6MTAgUE0sIE15a2xlYnVzdCwgVHJvbmQgd3JvdGU6DQo+IA0K
PiA+IE9uIE1vbiwgMjAxMi0wNy0wOSBhdCAxMTo0NCAtMDQwMCwgQ2h1Y2sgTGV2ZXIgd3JvdGU6
DQo+ID4+IEZvciBORlN2NCBtaW5vciB2ZXJzaW9uIDAsIGN1cnJlbnRseSB0aGUgY2xfaWRfdW5p
cXVpZmllciBhbGxvd3MgdGhlDQo+ID4+IExpbnV4IGNsaWVudCB0byBnZW5lcmF0ZSBhIHVuaXF1
ZSBuZnNfY2xpZW50X2lkNCBzdHJpbmcgd2hlbmV2ZXIgYQ0KPiA+PiBzZXJ2ZXIgcmVwbGllcyB3
aXRoIE5GUzRFUlJfQ0xJRF9JTlVTRS4NCj4gPj4gDQo+ID4+IFRoaXMgaW1wbGVtZW50YXRpb24g
c2VlbXMgdG8gYmUgYmFzZWQgb24gYSBmbGF3ZWQgcmVhZGluZyBvZiBSRkMNCj4gPj4gMzUzMC4g
IE5GUzRFUlJfQ0xJRF9JTlVTRSBhY3R1YWxseSBtZWFucyB0aGF0IHRoZSBjbGllbnQgaGFzIHBy
ZXNlbnRlZA0KPiA+PiB0aGlzIG5mc19jbGllbnRfaWQ0IHN0cmluZyB3aXRoIGEgZGlmZmVyZW50
IHByaW5jaXBhbCBhdCBzb21lIHRpbWUgaW4NCj4gPj4gdGhlIHBhc3QsIGFuZCB0aGF0IGxlYXNl
IGlzIHN0aWxsIGluIHVzZSBvbiB0aGUgc2VydmVyLg0KPiA+PiANCj4gPj4gRm9yIGEgTGludXgg
Y2xpZW50IHRoaXMgbWlnaHQgYmUgcmF0aGVyIGRpZmZpY3VsdCB0byBhY2hpZXZlOiB0aGUNCj4g
Pj4gYXV0aGVudGljYXRpb24gZmxhdm9yIGlzIG5hbWVkIHJpZ2h0IGluIHRoZSBuZnNfY2xpZW50
X2lkNC5pZA0KPiA+PiBzdHJpbmcuICBJZiB3ZSBjaGFuZ2UgZmxhdm9ycywgd2UgY2hhbmdlIHN0
cmluZ3MgYXV0b21hdGljYWxseS4NCj4gPj4gDQo+ID4+IFNvLCBwcmFjdGljYWxseSBzcGVha2lu
ZywgTkZTNEVSUl9DTElEX0lOVVNFIG1lYW5zIHRoZXJlIGlzIHNvbWUgb3RoZXINCj4gPj4gY2xp
ZW50IHVzaW5nIG91ciBzdHJpbmcuICBUaGVyZSBpcyBub3QgbXVjaCB0aGF0IGNhbiBiZSBkb25l
IHRvDQo+ID4+IHJlY292ZXIgYXV0b21hdGljYWxseS4gIExldCdzIG1ha2UgaXQgYSBwZXJtYW5l
bnQgZXJyb3IuDQo+ID4+IA0KPiA+PiBSZW1vdmUgdGhlIHJlY292ZXJ5IGxvZ2ljIGluIG5mczRf
cHJvY19zZXRjbGllbnRpZCgpLCBhbmQgcmVtb3ZlIHRoZQ0KPiA+PiBjbF9pZF91bmlxdWlmaWVy
IGZpZWxkIGZyb20gdGhlIG5mc19jbGllbnQgZGF0YSBzdHJ1Y3R1cmUuICBBbmQsDQo+ID4+IHJl
bW92ZSB0aGUgYXV0aGVudGljYXRpb24gZmxhdm9yIGZyb20gdGhlIG5mc19jbGllbnRfaWQ0IHN0
cmluZy4NCj4gPj4gDQo+ID4+IEtlZXBpbmcgdGhlIGF1dGhlbnRpY2F0aW9uIGZsYXZvciBpbiB0
aGUgbmZzX2NsaWVudF9pZDQuaWQgc3RyaW5nDQo+ID4+IG1lYW5zIHRoYXQgd2UgY291bGQgaGF2
ZSBhIHNlcGFyYXRlIGxlYXNlIGZvciBlYWNoIGF1dGhlbnRpY2F0aW9uDQo+ID4+IGZsYXZvciB1
c2VkIGJ5IG1vdW50cyBvbiB0aGUgY2xpZW50LiAgQnV0IHdlIHdhbnQganVzdCBvbmUgbGVhc2Ug
Zm9yDQo+ID4+IGFsbCB0aGUgbW91bnRzIG9uIHRoaXMgY2xpZW50Lg0KPiA+PiANCj4gPj4gU2ln
bmVkLW9mZi1ieTogQ2h1Y2sgTGV2ZXIgPGNodWNrLmxldmVyQG9yYWNsZS5jb20+DQo+ID4+IC0t
LQ0KPiA+PiANCj4gPj4gZnMvbmZzL25mczRwcm9jLmMgICAgICAgICB8ICAgNTQgKysrKysrKysr
KysrKysrKysrLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+ID4+IGZzL25mcy9uZnM0c3Rh
dGUuYyAgICAgICAgfCAgICA3ICsrKysrLQ0KPiA+PiBpbmNsdWRlL2xpbnV4L25mc19mc19zYi5o
IHwgICAgMyArLS0NCj4gPj4gMyBmaWxlcyBjaGFuZ2VkLCAyOSBpbnNlcnRpb25zKCspLCAzNSBk
ZWxldGlvbnMoLSkNCj4gPj4gDQo+ID4+IGRpZmYgLS1naXQgYS9mcy9uZnMvbmZzNHByb2MuYyBi
L2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4+IGluZGV4IDdhZGI3MDUuLjYzYTNjZjAgMTAwNjQ0DQo+
ID4+IC0tLSBhL2ZzL25mcy9uZnM0cHJvYy5jDQo+ID4+ICsrKyBiL2ZzL25mcy9uZnM0cHJvYy5j
DQo+ID4+IEBAIC0yNTksNyArMjU5LDEyIEBAIHN0YXRpYyBpbnQgbmZzNF93YWl0X2NsbnRfcmVj
b3ZlcihzdHJ1Y3QgbmZzX2NsaWVudCAqY2xwKQ0KPiA+PiANCj4gPj4gCXJlcyA9IHdhaXRfb25f
Yml0KCZjbHAtPmNsX3N0YXRlLCBORlM0Q0xOVF9NQU5BR0VSX1JVTk5JTkcsDQo+ID4+IAkJCW5m
c193YWl0X2JpdF9raWxsYWJsZSwgVEFTS19LSUxMQUJMRSk7DQo+ID4+IC0JcmV0dXJuIHJlczsN
Cj4gPj4gKwlpZiAocmVzKQ0KPiA+PiArCQlyZXR1cm4gcmVzOw0KPiA+PiArDQo+ID4+ICsJaWYg
KGNscC0+Y2xfY29uc19zdGF0ZSA8IDApDQo+ID4+ICsJCXJldHVybiBjbHAtPmNsX2NvbnNfc3Rh
dGU7DQo+ID4+ICsJcmV0dXJuIDA7DQo+ID4+IH0NCj4gPiANCj4gPiBBKSBUaGlzIHNob3VsZCBi
ZSBhIHNlcGFyYXRlIHBhdGNoLg0KPiANCj4gT0suDQo+IA0KPiA+IA0KPiA+IEIpIFdoeSBpcyBp
dCBuZWVkZWQgaW4gdGhlIGZpcnN0IHBsYWNlPyBBbGwgcGxhY2VzIHRoYXQgY2FsbA0KPiA+IG5m
czRfd2FpdF9jbG50X3JlY292ZXIgbGllIG91dHNpZGUgdGhlIG1vdW50IGNvZGUgcGF0aC4NCj4g
DQo+IFRoZSBwcm9ibGVtIGlzIHRoYXQgaXMgbm8gbG9uZ2VyIHRoZSBjYXNlIG9uY2Ugd2UgaGF2
ZSB0cnVua2luZyBkaXNjb3ZlcnkgaW4gdGhlIG1vdW50IHBhdGguDQo+IA0KPiBXZSBkaXNjdXNz
ZWQgdGhpcy4gIFRoaXMgaXMgd2hhdCB5b3UgdG9sZCBtZSB0byBkby4NCg0KVGhlIG9ubHkgbmV3
IGVycm9yIGNvbmRpdGlvbiB5b3VyIHRydW5raW5nIGRldGVjdGlvbiBpcyBwb3RlbnRpYWxseQ0K
aW50cm9kdWNpbmcgaGVyZSBpcyBDTElEX0lOVVNFLiBXaHkgZG9lcyB0aGF0IHdhcnJhbnQgdGhp
cyBjaGFuZ2U/DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50
YWluZXINCg0KTmV0QXBwDQpUcm9uZC5NeWtsZWJ1c3RAbmV0YXBwLmNvbQ0Kd3d3Lm5ldGFwcC5j
b20NCg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 7adb705..63a3cf0 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -259,7 +259,12 @@  static int nfs4_wait_clnt_recover(struct nfs_client *clp)
 
 	res = wait_on_bit(&clp->cl_state, NFS4CLNT_MANAGER_RUNNING,
 			nfs_wait_bit_killable, TASK_KILLABLE);
-	return res;
+	if (res)
+		return res;
+
+	if (clp->cl_cons_state < 0)
+		return clp->cl_cons_state;
+	return 0;
 }
 
 static int nfs4_delay(struct rpc_clnt *clnt, long *timeout)
@@ -4038,42 +4043,28 @@  int nfs4_proc_setclientid(struct nfs_client *clp, u32 program,
 		.rpc_resp = res,
 		.rpc_cred = cred,
 	};
-	int loop = 0;
-	int status;
 
+	/* nfs_client_id4 */
 	nfs4_init_boot_verifier(clp, &sc_verifier);
-
-	for(;;) {
-		rcu_read_lock();
-		setclientid.sc_name_len = scnprintf(setclientid.sc_name,
-				sizeof(setclientid.sc_name), "%s/%s %s %s %u",
-				clp->cl_ipaddr,
-				rpc_peeraddr2str(clp->cl_rpcclient,
-							RPC_DISPLAY_ADDR),
-				rpc_peeraddr2str(clp->cl_rpcclient,
-							RPC_DISPLAY_PROTO),
-				clp->cl_rpcclient->cl_auth->au_ops->au_name,
-				clp->cl_id_uniquifier);
-		setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
+	rcu_read_lock();
+	setclientid.sc_name_len = scnprintf(setclientid.sc_name,
+			sizeof(setclientid.sc_name), "%s/%s %s",
+			clp->cl_ipaddr,
+			rpc_peeraddr2str(clp->cl_rpcclient,
+						RPC_DISPLAY_ADDR),
+			rpc_peeraddr2str(clp->cl_rpcclient,
+						RPC_DISPLAY_PROTO));
+	/* cb_client4 */
+	setclientid.sc_netid_len = scnprintf(setclientid.sc_netid,
 				sizeof(setclientid.sc_netid),
 				rpc_peeraddr2str(clp->cl_rpcclient,
 							RPC_DISPLAY_NETID));
-		setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
+	rcu_read_unlock();
+	setclientid.sc_uaddr_len = scnprintf(setclientid.sc_uaddr,
 				sizeof(setclientid.sc_uaddr), "%s.%u.%u",
 				clp->cl_ipaddr, port >> 8, port & 255);
-		rcu_read_unlock();
 
-		status = rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
-		if (status != -NFS4ERR_CLID_INUSE)
-			break;
-		if (loop != 0) {
-			++clp->cl_id_uniquifier;
-			break;
-		}
-		++loop;
-		ssleep(clp->cl_lease_time / HZ + 1);
-	}
-	return status;
+	return rpc_call_sync(clp->cl_rpcclient, &msg, RPC_TASK_TIMEOUT);
 }
 
 int nfs4_proc_setclientid_confirm(struct nfs_client *clp,
@@ -5275,10 +5266,9 @@  int nfs4_proc_exchange_id(struct nfs_client *clp, struct rpc_cred *cred)
 	nfs4_init_boot_verifier(clp, &verifier);
 
 	args.id_len = scnprintf(args.id, sizeof(args.id),
-				"%s/%s/%u",
+				"%s/%s",
 				clp->cl_ipaddr,
-				clp->cl_rpcclient->cl_nodename,
-				clp->cl_rpcclient->cl_auth->au_flavor);
+				clp->cl_rpcclient->cl_nodename);
 
 	res.server_owner = kzalloc(sizeof(struct nfs41_server_owner),
 					GFP_NOFS);
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index f38300e..a2c1153 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -1606,10 +1606,15 @@  static int nfs4_handle_reclaim_lease_error(struct nfs_client *clp, int status)
 			return -ESERVERFAULT;
 		/* Lease confirmation error: retry after purging the lease */
 		ssleep(1);
-	case -NFS4ERR_CLID_INUSE:
 	case -NFS4ERR_STALE_CLIENTID:
 		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
 		break;
+	case -NFS4ERR_CLID_INUSE:
+		printk(KERN_ERR "NFS: Server %s: client ID already in use\n",
+			clp->cl_hostname);
+		nfs_mark_client_ready(clp, -EPERM);
+		clear_bit(NFS4CLNT_LEASE_CONFIRM, &clp->cl_state);
+		return -EPERM;
 	case -EACCES:
 		if (clp->cl_machine_cred == NULL)
 			return -EACCES;
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index f58325a..6532765 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -69,10 +69,9 @@  struct nfs_client {
 	struct idmap *		cl_idmap;
 
 	/* Our own IP address, as a null-terminated string.
-	 * This is used to generate the clientid, and the callback address.
+	 * This is used to generate the mv0 callback address.
 	 */
 	char			cl_ipaddr[48];
-	unsigned char		cl_id_uniquifier;
 	u32			cl_cb_ident;	/* v4.0 callback identifier */
 	const struct nfs4_minor_version_ops *cl_mvops;