Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Benny Halevy]

On 2010-10-07 15:37, andros@netapp.com wrote:
> From: Andy Adamson <andros@netapp.com>
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>  fs/nfs/nfs4state.c         |    5 +++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
> index e0edf93..1f0ab62 100644
> --- a/fs/nfs/nfs4filelayoutdev.c
> +++ b/fs/nfs/nfs4filelayoutdev.c
> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>  		goto out_put;
>  	}
>  	/*
> -	 * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
> -	 * The is_ds_only_session depends on this.
> -	 */
> -	clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
> -	/*
>  	 * Set DS lease equal to the MDS lease, renewal is scheduled in
>  	 * create_session
>  	 */
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index 91584ad..e2fc175 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>  {
>  	int status;
> +	u32 req_exchange_flags = clp->cl_exchange_flags;
>  
>  	nfs4_begin_drain_session(clp);
>  	status = nfs4_proc_exchange_id(clp, cred);
> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>  	status = nfs4_proc_create_session(clp);
>  	if (status != 0)
>  		goto out;
> +	if (is_ds_only_session(req_exchange_flags))
> +		/* Mask the (possibly) returned MDS and non-pNFS roles */

This comment does not really add anything substantial that the code doesn't tell you :)

> +		clp->cl_exchange_flags &=
> +		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);

I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
If the server is not a DS why not just return an error?

Benny

>  	nfs41_setup_state_renewal(clp);
>  	nfs_mark_client_ready(clp, NFS_CS_READY);
>  out:

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Benny Halevy]

On 2010-10-07 13:06, Benny Halevy wrote:
> On 2010-10-07 15:37, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>>  fs/nfs/nfs4state.c         |    5 +++++
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index e0edf93..1f0ab62 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>  		goto out_put;
>>  	}
>>  	/*
>> -	 * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>> -	 * The is_ds_only_session depends on this.
>> -	 */
>> -	clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>> -	/*
>>  	 * Set DS lease equal to the MDS lease, renewal is scheduled in
>>  	 * create_session
>>  	 */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 91584ad..e2fc175 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>  {
>>  	int status;
>> +	u32 req_exchange_flags = clp->cl_exchange_flags;
>>  
>>  	nfs4_begin_drain_session(clp);
>>  	status = nfs4_proc_exchange_id(clp, cred);
>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>  	status = nfs4_proc_create_session(clp);
>>  	if (status != 0)
>>  		goto out;
>> +	if (is_ds_only_session(req_exchange_flags))
>> +		/* Mask the (possibly) returned MDS and non-pNFS roles */
> 
> This comment does not really add anything substantial that the code doesn't tell you :)
> 
>> +		clp->cl_exchange_flags &=
>> +		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
> 
> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
> If the server is not a DS why not just return an error?

So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
is a valid response.
However, if USE_PNFS_DS is unset in the response in this case
we need not create the client and better return an error.
I'll send a patch that does that.

Benny

> 
> Benny
> 
>>  	nfs41_setup_state_renewal(clp);
>>  	nfs_mark_client_ready(clp, NFS_CS_READY);
>>  out:
> --
> 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

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Fred Isaman]

On Thu, Oct 7, 2010 at 1:06 PM, Benny Halevy <bhalevy@panasas.com> wrote:
> On 2010-10-07 15:37, andros@netapp.com wrote:
>> From: Andy Adamson <andros@netapp.com>
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>>  fs/nfs/nfs4state.c         |    5 +++++
>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>> index e0edf93..1f0ab62 100644
>> --- a/fs/nfs/nfs4filelayoutdev.c
>> +++ b/fs/nfs/nfs4filelayoutdev.c
>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>               goto out_put;
>>       }
>>       /*
>> -      * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>> -      * The is_ds_only_session depends on this.
>> -      */
>> -     clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>> -     /*
>>        * Set DS lease equal to the MDS lease, renewal is scheduled in
>>        * create_session
>>        */
>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>> index 91584ad..e2fc175 100644
>> --- a/fs/nfs/nfs4state.c
>> +++ b/fs/nfs/nfs4state.c
>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>  {
>>       int status;
>> +     u32 req_exchange_flags = clp->cl_exchange_flags;
>>
>>       nfs4_begin_drain_session(clp);
>>       status = nfs4_proc_exchange_id(clp, cred);
>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>       status = nfs4_proc_create_session(clp);
>>       if (status != 0)
>>               goto out;
>> +     if (is_ds_only_session(req_exchange_flags))
>> +             /* Mask the (possibly) returned MDS and non-pNFS roles */
>
> This comment does not really add anything substantial that the code doesn't tell you :)
>
>> +             clp->cl_exchange_flags &=
>> +                  ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>
> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
> If the server is not a DS why not just return an error?

We *know* _USE_PNFS_DS is set.  We just want to mask out
_USE_NON_PNFS, which  would indicate it is also a 4.0 server.

Fred

>
> Benny
>
>>       nfs41_setup_state_renewal(clp);
>>       nfs_mark_client_ready(clp, NFS_CS_READY);
>>  out:
> --
> 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
>

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Fred Isaman]

On Thu, Oct 7, 2010 at 2:10 PM, Fred Isaman <iisaman@netapp.com> wrote:
> On Thu, Oct 7, 2010 at 1:06 PM, Benny Halevy <bhalevy@panasas.com> wrote:
>> On 2010-10-07 15:37, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>>>  fs/nfs/nfs4state.c         |    5 +++++
>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>> index e0edf93..1f0ab62 100644
>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>>               goto out_put;
>>>       }
>>>       /*
>>> -      * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>> -      * The is_ds_only_session depends on this.
>>> -      */
>>> -     clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>> -     /*
>>>        * Set DS lease equal to the MDS lease, renewal is scheduled in
>>>        * create_session
>>>        */
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 91584ad..e2fc175 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>  {
>>>       int status;
>>> +     u32 req_exchange_flags = clp->cl_exchange_flags;
>>>
>>>       nfs4_begin_drain_session(clp);
>>>       status = nfs4_proc_exchange_id(clp, cred);
>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>       status = nfs4_proc_create_session(clp);
>>>       if (status != 0)
>>>               goto out;
>>> +     if (is_ds_only_session(req_exchange_flags))
>>> +             /* Mask the (possibly) returned MDS and non-pNFS roles */
>>
>> This comment does not really add anything substantial that the code doesn't tell you :)
>>
>>> +             clp->cl_exchange_flags &=
>>> +                  ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>
>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>> If the server is not a DS why not just return an error?
>
> We *know* _USE_PNFS_DS is set.  We just want to mask out
> _USE_NON_PNFS, which  would indicate it is also a 4.0 server.
>

Oops...we *know* _PNFS_DS is set in req_exchange_flags.  As you point
out, we would like to know it is in the reply.

Fred

> Fred
>
>>
>> Benny
>>
>>>       nfs41_setup_state_renewal(clp);
>>>       nfs_mark_client_ready(clp, NFS_CS_READY);
>>>  out:
>> --
>> 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
>>
>

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Benny Halevy]

On 2010-10-07 14:08, Benny Halevy wrote:
> On 2010-10-07 13:06, Benny Halevy wrote:
>> On 2010-10-07 15:37, andros@netapp.com wrote:
>>> From: Andy Adamson <andros@netapp.com>
>>>
>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>> ---
>>>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>>>  fs/nfs/nfs4state.c         |    5 +++++
>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>> index e0edf93..1f0ab62 100644
>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>>  		goto out_put;
>>>  	}
>>>  	/*
>>> -	 * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>> -	 * The is_ds_only_session depends on this.
>>> -	 */
>>> -	clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>> -	/*
>>>  	 * Set DS lease equal to the MDS lease, renewal is scheduled in
>>>  	 * create_session
>>>  	 */
>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>> index 91584ad..e2fc175 100644
>>> --- a/fs/nfs/nfs4state.c
>>> +++ b/fs/nfs/nfs4state.c
>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>  {
>>>  	int status;
>>> +	u32 req_exchange_flags = clp->cl_exchange_flags;
>>>  
>>>  	nfs4_begin_drain_session(clp);
>>>  	status = nfs4_proc_exchange_id(clp, cred);
>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>  	status = nfs4_proc_create_session(clp);
>>>  	if (status != 0)
>>>  		goto out;
>>> +	if (is_ds_only_session(req_exchange_flags))
>>> +		/* Mask the (possibly) returned MDS and non-pNFS roles */
>>
>> This comment does not really add anything substantial that the code doesn't tell you :)
>>
>>> +		clp->cl_exchange_flags &=
>>> +		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>
>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>> If the server is not a DS why not just return an error?
> 
> So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
> is a valid response.
> However, if USE_PNFS_DS is unset in the response in this case
> we need not create the client and better return an error.
> I'll send a patch that does that.

So this is what I have in mind:

diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index e2fc175..4723c41 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -197,10 +197,14 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
 	status = nfs4_proc_create_session(clp);
 	if (status != 0)
 		goto out;
-	if (is_ds_only_session(req_exchange_flags))
-		/* Mask the (possibly) returned MDS and non-pNFS roles */
+	if (is_ds_only_session(req_exchange_flags)) {
 		clp->cl_exchange_flags &=
 		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
+		if (!is_ds_only_session(clp->cl_exchange_flags)) {
+			nfs4_proc_destroy_session(clp);
+			status = -ENOTSUPP;
+		}
+	}
 	nfs41_setup_state_renewal(clp);
 	nfs_mark_client_ready(clp, NFS_CS_READY);
 out:

> 
> Benny
> 
>>
>> Benny
>>
>>>  	nfs41_setup_state_renewal(clp);
>>>  	nfs_mark_client_ready(clp, NFS_CS_READY);
>>>  out:
>> --
>> 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
> --
> 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


-- 
Benny Halevy
Software Architect
Panasas, Inc.
bhalevy@panasas.com
Tel/Fax: +972-3-647-8340
Mobile: +972-54-802-8340

Panasas: The Leader in Parallel Storage
www.panasas.com

Re: [PATCH 1/3] pnfs_submit: move layout segment valid test

[Benny Halevy]

On 2010-10-07 15:37, andros@netapp.com wrote:
> From: Andy Adamson <andros@rhel6-1.androsmac.org>
> 
> Do not get_lseg for a non-valid lseg.
> Prepare for calling put_lseg outside of inode i_lock.
> 
> Signed-off-by: Andy Adamson <andros@netapp.com>
> ---
>  fs/nfs/pnfs.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 6b2a95d..24620cf 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
>  	list_for_each_entry(lseg, &lo->segs, fi_list) {
>  		if (is_matching_lseg(lseg, range)) {
>  			ret = lseg;
> -			get_lseg(ret);
> +			if (lseg->valid)
> +				get_lseg(ret);

We shouldn't be hiding this inside pnfs_has_layout
and have different side effects in the different cases.
Since we're called under the lock, pnfs_has_layout
can just return the lseg and never get a reference and its
caller can take it if necessary before releasing the lock.

Also, it doesn't need to be EXPORT_SYMBOL_GPLed as it's
not used outside of the nfs client module.

Benny

>  			break;
>  		}
>  		if (cmp_layout(range, &lseg->range) > 0)
> @@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
>  	/* Check to see if the layout for the given range already exists */
>  	lseg = pnfs_has_layout(lo, &arg);
>  	if (lseg && !lseg->valid) {
> -		put_lseg_locked(lseg);
>  		/* someone is cleaning the layout */
>  		lseg = NULL;
>  		goto out_unlock;

[PATCH 1/1] SQUASHME: pnfs-submit: do not get_lseg in pnfs_has_layout

[Benny Halevy]

let its caller do that if necessary

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfs/pnfs.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 40d1dc6..a27df2b 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -844,8 +844,6 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
 	list_for_each_entry(lseg, &lo->segs, fi_list) {
 		if (is_matching_lseg(lseg, range)) {
 			ret = lseg;
-			if (lseg->valid)
-				get_lseg(ret);
 			break;
 		}
 		if (cmp_layout(range, &lseg->range) > 0)
@@ -857,7 +855,6 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
 		ret ? ret->valid : 0);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(pnfs_has_layout);
 
 /*
  * Layout segment is retreived from the server if not cached.
@@ -897,6 +894,7 @@ pnfs_update_layout(struct inode *ino,
 	if (lseg) {
 		dprintk("%s: Using cached lseg %p for iomode %d)\n",
 			__func__, lseg, iomode);
+		get_lseg(lseg);
 		goto out_unlock;
 	}
 
-- 
1.7.2.3

[PATCH 2/2] SQUASHME: pnfs: get_lseg in nfs4_layoutget_prepare rather than in pnfs_has_layout

[Benny Halevy]

pnfs_has_layout does not get_lref on its return value anymore

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 fs/nfs/nfs4proc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 25bc169..97cc539 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -5442,12 +5442,12 @@ nfs4_layoutget_prepare(struct rpc_task *task, void *calldata)
 		return;
 	}
 	if (!lseg->valid) {
-		put_lseg_locked(lseg);
 		spin_unlock(&ino->i_lock);
 		dprintk("%s: invalid lseg found, waiting\n", __func__);
 		rpc_sleep_on(&nfsi->lo_rpcwaitq, task, NULL);
 		return;
 	}
+	get_lseg(lseg);
 	*lgp->lsegpp = lseg;
 	spin_unlock(&ino->i_lock);
 	dprintk("%s: valid lseg found, no rpc required\n", __func__);
-- 
1.7.2.3

Re: [PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[Benny Halevy]

On 2010-10-07 14:41, Benny Halevy wrote:
> On 2010-10-07 14:08, Benny Halevy wrote:
>> On 2010-10-07 13:06, Benny Halevy wrote:
>>> On 2010-10-07 15:37, andros@netapp.com wrote:
>>>> From: Andy Adamson <andros@netapp.com>
>>>>
>>>> Signed-off-by: Andy Adamson <andros@netapp.com>
>>>> ---
>>>>  fs/nfs/nfs4filelayoutdev.c |    5 -----
>>>>  fs/nfs/nfs4state.c         |    5 +++++
>>>>  2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
>>>> index e0edf93..1f0ab62 100644
>>>> --- a/fs/nfs/nfs4filelayoutdev.c
>>>> +++ b/fs/nfs/nfs4filelayoutdev.c
>>>> @@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
>>>>  		goto out_put;
>>>>  	}
>>>>  	/*
>>>> -	 * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
>>>> -	 * The is_ds_only_session depends on this.
>>>> -	 */
>>>> -	clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
>>>> -	/*
>>>>  	 * Set DS lease equal to the MDS lease, renewal is scheduled in
>>>>  	 * create_session
>>>>  	 */
>>>> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
>>>> index 91584ad..e2fc175 100644
>>>> --- a/fs/nfs/nfs4state.c
>>>> +++ b/fs/nfs/nfs4state.c
>>>> @@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
>>>>  int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>>  {
>>>>  	int status;
>>>> +	u32 req_exchange_flags = clp->cl_exchange_flags;
>>>>  
>>>>  	nfs4_begin_drain_session(clp);
>>>>  	status = nfs4_proc_exchange_id(clp, cred);
>>>> @@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>>>>  	status = nfs4_proc_create_session(clp);
>>>>  	if (status != 0)
>>>>  		goto out;
>>>> +	if (is_ds_only_session(req_exchange_flags))
>>>> +		/* Mask the (possibly) returned MDS and non-pNFS roles */
>>>
>>> This comment does not really add anything substantial that the code doesn't tell you :)
>>>
>>>> +		clp->cl_exchange_flags &=
>>>> +		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
>>>
>>> I'm not why you want to mask out EXCHGID4_FLAG_USE_NON_PNFS.
>>> If the server is not a DS why not just return an error?
>>
>> So Andy convinced me and the spec. says that USE_PNFS_DS | USE_NON_PNFS
>> is a valid response.
>> However, if USE_PNFS_DS is unset in the response in this case
>> we need not create the client and better return an error.
>> I'll send a patch that does that.
> 
> So this is what I have in mind:
> 
> diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
> index e2fc175..4723c41 100644
> --- a/fs/nfs/nfs4state.c
> +++ b/fs/nfs/nfs4state.c
> @@ -197,10 +197,14 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
>  	status = nfs4_proc_create_session(clp);
>  	if (status != 0)
>  		goto out;
> -	if (is_ds_only_session(req_exchange_flags))
> -		/* Mask the (possibly) returned MDS and non-pNFS roles */
> +	if (is_ds_only_session(req_exchange_flags)) {
>  		clp->cl_exchange_flags &=
>  		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
> +		if (!is_ds_only_session(clp->cl_exchange_flags)) {
> +			nfs4_proc_destroy_session(clp);

This should be {
	nfs4_destroy_session(clp->cl_session);
	clp->cl_session = NULL;
}


> +			status = -ENOTSUPP;
> +		}
> +	}
>  	nfs41_setup_state_renewal(clp);
>  	nfs_mark_client_ready(clp, NFS_CS_READY);
>  out:
> 
>>
>> Benny
>>
>>>
>>> Benny
>>>
>>>>  	nfs41_setup_state_renewal(clp);
>>>>  	nfs_mark_client_ready(clp, NFS_CS_READY);
>>>>  out:
>>> --
>>> 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
>> --
>> 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
> 
> 

Re: [PATCH 1/3] pnfs_submit: move layout segment valid test

[William A. (Andy) Adamson]

On Thu, Oct 7, 2010 at 2:52 PM, Benny Halevy <bhalevy@panasas.com> wrote:
> On 2010-10-07 15:37, andros@netapp.com wrote:
>> From: Andy Adamson <andros@rhel6-1.androsmac.org>
>>
>> Do not get_lseg for a non-valid lseg.
>> Prepare for calling put_lseg outside of inode i_lock.
>>
>> Signed-off-by: Andy Adamson <andros@netapp.com>
>> ---
>>  fs/nfs/pnfs.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 6b2a95d..24620cf 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
>>       list_for_each_entry(lseg, &lo->segs, fi_list) {
>>               if (is_matching_lseg(lseg, range)) {
>>                       ret = lseg;
>> -                     get_lseg(ret);
>> +                     if (lseg->valid)
>> +                             get_lseg(ret);
>
> We shouldn't be hiding this inside pnfs_has_layout
> and have different side effects in the different cases.

Sorry - I just looked at the pnfs-submit branch. I should have looked
at the pnfs-all branch and seen the other callers of pnfs_has_layout.

> Since we're called under the lock, pnfs_has_layout
> can just return the lseg and never get a reference and its
> caller can take it if necessary before releasing the lock.
>
> Also, it doesn't need to be EXPORT_SYMBOL_GPLed as it's
> not used outside of the nfs client module.

Right - Fred just removed the call in the file layout driver.

I'll fix this.

-->Andy

>
> Benny
>
>>                       break;
>>               }
>>               if (cmp_layout(range, &lseg->range) > 0)
>> @@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
>>       /* Check to see if the layout for the given range already exists */
>>       lseg = pnfs_has_layout(lo, &arg);
>>       if (lseg && !lseg->valid) {
>> -             put_lseg_locked(lseg);
>>               /* someone is cleaning the layout */
>>               lseg = NULL;
>>               goto out_unlock;
> --
> 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
>

[PATCH 1/3] pnfs_submit: move layout segment valid test

[andros]

From: Andy Adamson <andros@rhel6-1.androsmac.org>

Do not get_lseg for a non-valid lseg.
Prepare for calling put_lseg outside of inode i_lock.

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/pnfs.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 6b2a95d..24620cf 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -845,7 +845,8 @@ pnfs_has_layout(struct pnfs_layout_hdr *lo,
 	list_for_each_entry(lseg, &lo->segs, fi_list) {
 		if (is_matching_lseg(lseg, range)) {
 			ret = lseg;
-			get_lseg(ret);
+			if (lseg->valid)
+				get_lseg(ret);
 			break;
 		}
 		if (cmp_layout(range, &lseg->range) > 0)
@@ -889,7 +890,6 @@ pnfs_update_layout(struct inode *ino,
 	/* Check to see if the layout for the given range already exists */
 	lseg = pnfs_has_layout(lo, &arg);
 	if (lseg && !lseg->valid) {
-		put_lseg_locked(lseg);
 		/* someone is cleaning the layout */
 		lseg = NULL;
 		goto out_unlock;
-- 
1.6.6

[PATCH 2/3] pnfs_submit: fix deadlock in pnfs_clear_lseg_list

[andros]

From: Andy Adamson <andros@rhel6-1.androsmac.org>

The file layout free_lseg i/o operation called by destroy_lseg under the
inode->i_lock can call nfs_put_client() when a data
server is no longer referenced. nfs_put_client can end up taking the
i_mutex called in rpc_unlink (called from nfs_idmap_delete from
nfs_free_client) which can result in a deadlock.

Use a temporary list to hold layout segments to be freed, and free them outside
the inode->i_lock.

Reported-by: Fred Isaman <iisaman@netapp.com>
Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/pnfs.c |   53 ++++++++++++++++++++++++++---------------------------
 fs/nfs/pnfs.h |    1 -
 2 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 24620cf..06fcc92 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -275,6 +275,7 @@ init_lseg(struct pnfs_layout_hdr *lo, struct pnfs_layout_segment *lseg)
 	lseg->layout = lo;
 }
 
+/* Called without i_lock held */
 static void
 destroy_lseg(struct kref *kref)
 {
@@ -285,29 +286,10 @@ destroy_lseg(struct kref *kref)
 	dprintk("--> %s\n", __func__);
 	NFS_SERVER(local->inode)->pnfs_curr_ld->free_lseg(lseg);
 	/* Matched by get_layout_hdr_locked in pnfs_insert_layout */
-	put_layout_hdr_locked(local);
+	put_layout_hdr(local->inode);
 }
 
 void
-put_lseg_locked(struct pnfs_layout_segment *lseg)
-{
-	bool do_wake_up;
-	struct nfs_inode *nfsi;
-
-	if (!lseg)
-		return;
-
-	dprintk("%s: lseg %p ref %d valid %d\n", __func__, lseg,
-		atomic_read(&lseg->kref.refcount), lseg->valid);
-	do_wake_up = !lseg->valid;
-	nfsi = NFS_I(lseg->layout->inode);
-	kref_put(&lseg->kref, destroy_lseg);
-	if (do_wake_up)
-		wake_up(&nfsi->lo_waitq);
-}
-EXPORT_SYMBOL_GPL(put_lseg_locked);
-
-void
 put_lseg(struct pnfs_layout_segment *lseg)
 {
 	bool do_wake_up;
@@ -320,9 +302,7 @@ put_lseg(struct pnfs_layout_segment *lseg)
 		atomic_read(&lseg->kref.refcount), lseg->valid);
 	do_wake_up = !lseg->valid;
 	nfsi = NFS_I(lseg->layout->inode);
-	spin_lock(&nfsi->vfs_inode.i_lock);
 	kref_put(&lseg->kref, destroy_lseg);
-	spin_unlock(&nfsi->vfs_inode.i_lock);
 	if (do_wake_up)
 		wake_up(&nfsi->lo_waitq);
 }
@@ -354,10 +334,11 @@ _pnfs_can_return_lseg(struct pnfs_layout_segment *lseg)
 }
 
 static void
-pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
+pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo, struct list_head *tmp_list,
 		     struct pnfs_layout_range *range)
 {
 	struct pnfs_layout_segment *lseg, *next;
+
 	dprintk("%s:Begin lo %p offset %llu length %llu iomode %d\n",
 		__func__, lo, range->offset, range->length, range->iomode);
 
@@ -370,8 +351,7 @@ pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
 			"offset %llu length %llu\n", __func__,
 			lseg, lseg->range.iomode, lseg->range.offset,
 			lseg->range.length);
-		list_del(&lseg->fi_list);
-		put_lseg_locked(lseg);
+		list_move(&lseg->fi_list, tmp_list);
 	}
 	if (list_empty(&lo->segs)) {
 		struct nfs_client *clp;
@@ -387,6 +367,21 @@ pnfs_clear_lseg_list(struct pnfs_layout_hdr *lo,
 	dprintk("%s:Return\n", __func__);
 }
 
+static void
+pnfs_free_lseg_list(struct list_head *tmp_list)
+{
+	struct pnfs_layout_segment *lseg;
+
+	while (!list_empty(tmp_list)) {
+		lseg = list_entry(tmp_list->next, struct pnfs_layout_segment,
+				fi_list);
+		dprintk("%s calling put_lseg on %p\n", __func__, lseg);
+		list_del(&lseg->fi_list);
+		put_lseg(lseg);
+	}
+}
+
+
 void
 pnfs_layoutget_release(struct pnfs_layout_hdr *lo)
 {
@@ -403,12 +398,14 @@ pnfs_layoutreturn_release(struct pnfs_layout_hdr *lo,
 			  struct pnfs_layout_range *range)
 {
 	struct nfs_inode *nfsi = NFS_I(lo->inode);
+	LIST_HEAD(tmp_list);
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	if (range)
-		pnfs_clear_lseg_list(lo, range);
+		pnfs_clear_lseg_list(lo, &tmp_list, range);
 	put_layout_hdr_locked(lo); /* Matched in _pnfs_return_layout */
 	spin_unlock(&nfsi->vfs_inode.i_lock);
+	pnfs_free_lseg_list(&tmp_list);
 	wake_up_all(&nfsi->lo_waitq);
 }
 
@@ -421,11 +418,12 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
 		.offset = 0,
 		.length = NFS4_MAX_UINT64,
 	};
+	LIST_HEAD(tmp_list);
 
 	spin_lock(&nfsi->vfs_inode.i_lock);
 	lo = nfsi->layout;
 	if (lo) {
-		pnfs_clear_lseg_list(lo, &range);
+		pnfs_clear_lseg_list(lo, &tmp_list,  &range);
 		WARN_ON(!list_empty(&nfsi->layout->segs));
 		WARN_ON(!list_empty(&nfsi->layout->layouts));
 		WARN_ON(nfsi->layout->refcount != 1);
@@ -434,6 +432,7 @@ pnfs_destroy_layout(struct nfs_inode *nfsi)
 		put_layout_hdr_locked(lo);
 	}
 	spin_unlock(&nfsi->vfs_inode.i_lock);
+	pnfs_free_lseg_list(&tmp_list);
 }
 
 /*
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 1b1efcd..51f717d 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -177,7 +177,6 @@ extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp, bool wait);
 
 /* pnfs.c */
 void put_lseg(struct pnfs_layout_segment *lseg);
-void put_lseg_locked(struct pnfs_layout_segment *lseg);
 struct pnfs_layout_segment *
 pnfs_has_layout(struct pnfs_layout_hdr *lo, struct pnfs_layout_range *range);
 struct pnfs_layout_segment *
-- 
1.6.6

[PATCH 3/3] pnfs_submit: enforce requested DS only pNFS role

[andros]

From: Andy Adamson <andros@netapp.com>

Signed-off-by: Andy Adamson <andros@netapp.com>
---
 fs/nfs/nfs4filelayoutdev.c |    5 -----
 fs/nfs/nfs4state.c         |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/nfs4filelayoutdev.c b/fs/nfs/nfs4filelayoutdev.c
index e0edf93..1f0ab62 100644
--- a/fs/nfs/nfs4filelayoutdev.c
+++ b/fs/nfs/nfs4filelayoutdev.c
@@ -183,11 +183,6 @@ nfs4_pnfs_ds_create(struct nfs_server *mds_srv, struct nfs4_pnfs_ds *ds)
 		goto out_put;
 	}
 	/*
-	 * Mask the (possibly) returned EXCHGID4_FLAG_USE_PNFS_MDS pNFS role
-	 * The is_ds_only_session depends on this.
-	 */
-	clp->cl_exchange_flags &= ~EXCHGID4_FLAG_USE_PNFS_MDS;
-	/*
 	 * Set DS lease equal to the MDS lease, renewal is scheduled in
 	 * create_session
 	 */
diff --git a/fs/nfs/nfs4state.c b/fs/nfs/nfs4state.c
index 91584ad..e2fc175 100644
--- a/fs/nfs/nfs4state.c
+++ b/fs/nfs/nfs4state.c
@@ -188,6 +188,7 @@ static int nfs4_begin_drain_session(struct nfs_client *clp)
 int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
 {
 	int status;
+	u32 req_exchange_flags = clp->cl_exchange_flags;
 
 	nfs4_begin_drain_session(clp);
 	status = nfs4_proc_exchange_id(clp, cred);
@@ -196,6 +197,10 @@ int nfs41_init_clientid(struct nfs_client *clp, struct rpc_cred *cred)
 	status = nfs4_proc_create_session(clp);
 	if (status != 0)
 		goto out;
+	if (is_ds_only_session(req_exchange_flags))
+		/* Mask the (possibly) returned MDS and non-pNFS roles */
+		clp->cl_exchange_flags &=
+		     ~(EXCHGID4_FLAG_USE_PNFS_MDS | EXCHGID4_FLAG_USE_NON_PNFS);
 	nfs41_setup_state_renewal(clp);
 	nfs_mark_client_ready(clp, NFS_CS_READY);
 out:
-- 
1.6.6