[PATCH 1/2] nfs41: Move initialization of nfs4_opendata seq_res to nfs4_init_opendata_res

[PATCH 1/2] nfs41: Move initialization of nfs4_opendata seq_res to nfs4_init_opendata_res

[Benny Halevy]

nfs4_open_recover_helper clears opendata->o_res
before calling nfs4_init_opendata_res, thus causing
NFSv4.0 OPEN operations to be sent rather than nfsv4.1.

Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
Trond, please add these two patches to the nfs41-for-2.6.31 series.
Also availble on git://linux-nfs.org/~bhalevy/linux-pnfs.git nfs41-for-2.6.31

 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 57dabb8..04da834 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -680,6 +680,7 @@ static void nfs4_init_opendata_res(struct nfs4_opendata *p)
 	p->o_res.server = p->o_arg.server;
 	nfs_fattr_init(&p->f_attr);
 	nfs_fattr_init(&p->dir_attr);
+	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;
 }
 
 static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
@@ -711,7 +712,6 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
 	p->o_arg.server = server;
 	p->o_arg.bitmask = server->attr_bitmask;
 	p->o_arg.claim = NFS4_OPEN_CLAIM_NULL;
-	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;
 	if (flags & O_EXCL) {
 		u32 *s = (u32 *) p->o_arg.u.verifier.data;
 		s[0] = jiffies;
-- 
1.6.3.1

[PATCH 2/2] nfs41: sunrpc: xprt_alloc_bc_request() should not use spin_lock_bh()

[Benny Halevy]

From: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>

xprt_alloc_bc_request() is always called in soft interrupt context.
Grab the spin_lock instead of the bottom half spin_lock.  Softirqs
do not preempt other softirqs running on the same processor, so there
is no need to disable bottom halves.

Signed-off-by: Ricardo Labiaga <Ricardo.Labiaga@netapp.com>
Signed-off-by: Benny Halevy <bhalevy@panasas.com>
---
 net/sunrpc/backchannel_rqst.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/backchannel_rqst.c b/net/sunrpc/backchannel_rqst.c
index 5a7d342..553621f 100644
--- a/net/sunrpc/backchannel_rqst.c
+++ b/net/sunrpc/backchannel_rqst.c
@@ -211,6 +211,9 @@ EXPORT_SYMBOL(xprt_destroy_backchannel);
  * has been preallocated as well.  Use xprt_alloc_bc_request to allocate
  * to this request.  Use xprt_free_bc_request to return it.
  *
+ * We know that we're called in soft interrupt context, grab the spin_lock
+ * since there is no need to grab the bottom half spin_lock.
+ *
  * Return an available rpc_rqst, otherwise NULL if non are available.
  */
 struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt)
@@ -218,7 +221,7 @@ struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt)
 	struct rpc_rqst *req;
 
 	dprintk("RPC:       allocate a backchannel request\n");
-	spin_lock_bh(&xprt->bc_pa_lock);
+	spin_lock(&xprt->bc_pa_lock);
 	if (!list_empty(&xprt->bc_pa_list)) {
 		req = list_first_entry(&xprt->bc_pa_list, struct rpc_rqst,
 				rq_bc_pa_list);
@@ -226,7 +229,7 @@ struct rpc_rqst *xprt_alloc_bc_request(struct rpc_xprt *xprt)
 	} else {
 		req = NULL;
 	}
-	spin_unlock_bh(&xprt->bc_pa_lock);
+	spin_unlock(&xprt->bc_pa_lock);
 
 	if (req != NULL) {
 		set_bit(RPC_BC_PA_IN_USE, &req->rq_bc_pa_state);
-- 
1.6.3.1

Re: [PATCH 1/2] nfs41: Move initialization of nfs4_opendata seq_res to nfs4_init_opendata_res

[Trond Myklebust]

On Thu, 2009-06-18 at 22:01 -0400, Benny Halevy wrote:
> nfs4_open_recover_helper clears opendata->o_res
> before calling nfs4_init_opendata_res, thus causing
> NFSv4.0 OPEN operations to be sent rather than nfsv4.1.
> 
> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
> ---
> Trond, please add these two patches to the nfs41-for-2.6.31 series.
> Also availble on git://linux-nfs.org/~bhalevy/linux-pnfs.git nfs41-for-2.6.31
> 
>  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 57dabb8..04da834 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -680,6 +680,7 @@ static void nfs4_init_opendata_res(struct nfs4_opendata *p)
>  	p->o_res.server = p->o_arg.server;
>  	nfs_fattr_init(&p->f_attr);
>  	nfs_fattr_init(&p->dir_attr);
> +	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;

This really needs cleaning up. The "magic sr_slotid value"
initialisation is littering the code, and you have to look _very_
carefully in order to figure out what NFS4_MAX_SLOT_TABLE actually
means.

I'm applying this patch for now, but come 2.6.32, I do expect a
changeset that gets rid of the sr_slotid magic value in favour of
something that documents what it is for.

>  }
>  
>  static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
> @@ -711,7 +712,6 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
>  	p->o_arg.server = server;
>  	p->o_arg.bitmask = server->attr_bitmask;
>  	p->o_arg.claim = NFS4_OPEN_CLAIM_NULL;
> -	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;
>  	if (flags & O_EXCL) {
>  		u32 *s = (u32 *) p->o_arg.u.verifier.data;
>  		s[0] = jiffies;

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/2] nfs41: Move initialization of nfs4_opendata seq_res to nfs4_init_opendata_res

[Benny Halevy]

On Jun. 20, 2009, 21:52 +0300, Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> On Thu, 2009-06-18 at 22:01 -0400, Benny Halevy wrote:
>> nfs4_open_recover_helper clears opendata->o_res
>> before calling nfs4_init_opendata_res, thus causing
>> NFSv4.0 OPEN operations to be sent rather than nfsv4.1.
>>
>> Signed-off-by: Benny Halevy <bhalevy@panasas.com>
>> ---
>> Trond, please add these two patches to the nfs41-for-2.6.31 series.
>> Also availble on git://linux-nfs.org/~bhalevy/linux-pnfs.git nfs41-for-2.6.31
>>
>>  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 57dabb8..04da834 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -680,6 +680,7 @@ static void nfs4_init_opendata_res(struct nfs4_opendata *p)
>>  	p->o_res.server = p->o_arg.server;
>>  	nfs_fattr_init(&p->f_attr);
>>  	nfs_fattr_init(&p->dir_attr);
>> +	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;
> 
> This really needs cleaning up. The "magic sr_slotid value"
> initialisation is littering the code, and you have to look _very_
> carefully in order to figure out what NFS4_MAX_SLOT_TABLE actually
> means.
> 
> I'm applying this patch for now, but come 2.6.32, I do expect a
> changeset that gets rid of the sr_slotid magic value in favour of
> something that documents what it is for.

I agree that it is confusing.
I suggest that:
a. we add an helper to initialize struct nfs4_sequence_res
   so that the logic will be implemented in one place rather
   than scattered all over the place.
b. #define NFS4_INVALID_SLOT_ID NFS4_MAX_SLOT_TABLE
   and use as initial value.

Benny

> 
>>  }
>>  
>>  static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
>> @@ -711,7 +712,6 @@ static struct nfs4_opendata *nfs4_opendata_alloc(struct path *path,
>>  	p->o_arg.server = server;
>>  	p->o_arg.bitmask = server->attr_bitmask;
>>  	p->o_arg.claim = NFS4_OPEN_CLAIM_NULL;
>> -	p->o_res.seq_res.sr_slotid = NFS4_MAX_SLOT_TABLE;
>>  	if (flags & O_EXCL) {
>>  		u32 *s = (u32 *) p->o_arg.u.verifier.data;
>>  		s[0] = jiffies;
>