[RFC][PATCH] pnfs: unlock lo_lock spinlock before calling specific layoutdriver's setup_layoutcommit() operation.

[RFC][PATCH] pnfs: unlock lo_lock spinlock before calling specific layoutdriver's setup_layoutcommit() operation.

[Tao Guo]

So in blocklayoutdriver, we can use GFP_KERNEL to do memory
allocation in bl_setup_layoutcommit().

Signed-off-by: Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org>
---
 fs/nfs/pnfs.c |   42 +++++++++++++++++++++---------------------
 1 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index aedda1e..8474731 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -2073,15 +2073,23 @@ pnfs_layoutcommit_done(struct pnfs_layoutcommit_data *data)
  * Set up the argument/result storage required for the RPC call.
  */
 static int
-pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
+pnfs_layoutcommit_setup(struct inode *inode,
+			struct pnfs_layoutcommit_data *data, int sync)
 {
-	struct nfs_inode *nfsi = NFS_I(data->args.inode);
-	struct nfs_server *nfss = NFS_SERVER(data->args.inode);
+	struct nfs_inode *nfsi = NFS_I(inode);
+	struct nfs_server *nfss = NFS_SERVER(inode);
 	int result = 0;
 
 	dprintk("%s Begin (sync:%d)\n", __func__, sync);
+	data->is_sync = sync;
+	data->cred  = nfsi->layoutcommit_ctx->cred;
+	data->ctx = nfsi->layoutcommit_ctx;
+	data->args.inode = inode;
 	data->args.fh = NFS_FH(data->args.inode);
 	data->args.layout_type = nfss->pnfs_curr_ld->id;
+	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
+	data->res.fattr = &data->fattr;
+	nfs_fattr_init(&data->fattr);
 
 	/* TODO: Need to determine the correct values */
 	data->args.time_modify_changed = 0;
@@ -2095,6 +2103,7 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
 					i_size_read(&nfsi->vfs_inode) - 1);
 	data->args.bitmask = nfss->attr_bitmask;
 	data->res.server = nfss;
+	spin_unlock(&nfsi->lo_lock);
 
 	/* Call layout driver to set the arguments.
 	 */
@@ -2104,10 +2113,6 @@ pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sync)
 		if (result)
 			goto out;
 	}
-	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
-	data->res.fattr = &data->fattr;
-	nfs_fattr_init(&data->fattr);
-
 out:
 	dprintk("%s End Status %d\n", __func__, result);
 	return result;
@@ -2131,36 +2136,31 @@ pnfs_layoutcommit_inode(struct inode *inode, int sync)
 		return -ENOMEM;
 
 	spin_lock(&nfsi->lo_lock);
-	if (!nfsi->layoutcommit_ctx)
-		goto out_unlock;
-
-	data->args.inode = inode;
-	data->cred  = nfsi->layoutcommit_ctx->cred;
-	data->ctx = nfsi->layoutcommit_ctx;
+	if (!nfsi->layoutcommit_ctx) {
+		spin_unlock(&nfsi->lo_lock);
+		goto out_free;
+	}
 
-	/* Set up layout commit args*/
-	status = pnfs_layoutcommit_setup(data, sync);
+	/* Set up layout commit args */
+	status = pnfs_layoutcommit_setup(inode, data, sync);
 	if (status)
-		goto out_unlock;
+		goto out_free;
 
 	/* Clear layoutcommit properties in the inode so
 	 * new lc info can be generated
 	 */
+	spin_lock(&nfsi->lo_lock);
 	nfsi->pnfs_write_begin_pos = 0;
 	nfsi->pnfs_write_end_pos = 0;
 	nfsi->layoutcommit_ctx = NULL;
-
-	/* release lock on pnfs layoutcommit attrs */
 	spin_unlock(&nfsi->lo_lock);
 
-	data->is_sync = sync;
 	status = pnfs4_proc_layoutcommit(data);
 out:
 	dprintk("%s end (err:%d)\n", __func__, status);
 	return status;
-out_unlock:
+out_free:
 	pnfs_layoutcommit_free(data);
-	spin_unlock(&nfsi->lo_lock);
 	goto out;
 }
 
-- 
1.6.3.3

Re: [RFC][PATCH] pnfs: unlock lo_lock spinlock before calling specific layoutdriver's setup_layoutcommit() operation.

[Andy Adamson]

On May 20, 2010, at 6:25 AM, Tao Guo wrote:

> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
> allocation in bl_setup_layoutcommit().
>
> Signed-off-by: Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org>
> ---
> fs/nfs/pnfs.c |   42 +++++++++++++++++++++---------------------
> 1 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index aedda1e..8474731 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -2073,15 +2073,23 @@ pnfs_layoutcommit_done(struct  
> pnfs_layoutcommit_data *data)
>  * Set up the argument/result storage required for the RPC call.
>  */
> static int
> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int  
> sync)
> +pnfs_layoutcommit_setup(struct inode *inode,
> +			struct pnfs_layoutcommit_data *data, int sync)
> {
> -	struct nfs_inode *nfsi = NFS_I(data->args.inode);
> -	struct nfs_server *nfss = NFS_SERVER(data->args.inode);
> +	struct nfs_inode *nfsi = NFS_I(inode);
> +	struct nfs_server *nfss = NFS_SERVER(inode);
> 	int result = 0;
>
> 	dprintk("%s Begin (sync:%d)\n", __func__, sync);
> +	data->is_sync = sync;
> +	data->cred  = nfsi->layoutcommit_ctx->cred;
> +	data->ctx = nfsi->layoutcommit_ctx;
> +	data->args.inode = inode;
> 	data->args.fh = NFS_FH(data->args.inode);
> 	data->args.layout_type = nfss->pnfs_curr_ld->id;
> +	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> +	data->res.fattr = &data->fattr;
> +	nfs_fattr_init(&data->fattr);
>
> 	/* TODO: Need to determine the correct values */
> 	data->args.time_modify_changed = 0;
> @@ -2095,6 +2103,7 @@ pnfs_layoutcommit_setup(struct  
> pnfs_layoutcommit_data *data, int sync)
> 					i_size_read(&nfsi->vfs_inode) - 1);
> 	data->args.bitmask = nfss->attr_bitmask;
> 	data->res.server = nfss;
> +	spin_unlock(&nfsi->lo_lock);
>
> 	/* Call layout driver to set the arguments.
> 	 */
> @@ -2104,10 +2113,6 @@ pnfs_layoutcommit_setup(struct  
> pnfs_layoutcommit_data *data, int sync)
> 		if (result)
> 			goto out;
> 	}
> -	pnfs_get_layout_stateid(&data->args.stateid, &nfsi->layout);
> -	data->res.fattr = &data->fattr;
> -	nfs_fattr_init(&data->fattr);
> -
> out:
> 	dprintk("%s End Status %d\n", __func__, result);
> 	return result;
> @@ -2131,36 +2136,31 @@ pnfs_layoutcommit_inode(struct inode *inode,  
> int sync)
> 		return -ENOMEM;
>
> 	spin_lock(&nfsi->lo_lock);
> -	if (!nfsi->layoutcommit_ctx)
> -		goto out_unlock;
> -
> -	data->args.inode = inode;
> -	data->cred  = nfsi->layoutcommit_ctx->cred;
> -	data->ctx = nfsi->layoutcommit_ctx;
> +	if (!nfsi->layoutcommit_ctx) {
> +		spin_unlock(&nfsi->lo_lock);
> +		goto out_free;
> +	}
>
> -	/* Set up layout commit args*/
> -	status = pnfs_layoutcommit_setup(data, sync);
> +	/* Set up layout commit args */
> +	status = pnfs_layoutcommit_setup(inode, data, sync);
> 	if (status)
> -		goto out_unlock;
> +		goto out_free;
>
> 	/* Clear layoutcommit properties in the inode so
> 	 * new lc info can be generated
> 	 */
> +	spin_lock(&nfsi->lo_lock);
> 	nfsi->pnfs_write_begin_pos = 0;
> 	nfsi->pnfs_write_end_pos = 0;
> 	nfsi->layoutcommit_ctx = NULL;

These should be cleared before the spin_lock is released in  
pnfs_layoutcommit_setup.
They should be cleared (and the layoutcommit_cxt put) upon a  
layoutdriver setup_layoutcommit failure.

-->Andy
> -
> -	/* release lock on pnfs layoutcommit attrs */
> 	spin_unlock(&nfsi->lo_lock);
>
> -	data->is_sync = sync;
> 	status = pnfs4_proc_layoutcommit(data);
> out:
> 	dprintk("%s end (err:%d)\n", __func__, status);
> 	return status;
> -out_unlock:
> +out_free:
> 	pnfs_layoutcommit_free(data);
> -	spin_unlock(&nfsi->lo_lock);
> 	goto out;
> }
>
> -- 
> 1.6.3.3
>
> --
> 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: [RFC][PATCH] pnfs: unlock lo_lock spinlock before calling specific layoutdriver's setup_layoutcommit() operation.

[Tao Guo]

On Fri, May 21, 2010 at 12:13 AM, Andy Adamson <andros@netapp.com> wrot=
e:
>
> On May 20, 2010, at 6:25 AM, Tao Guo wrote:
>
>> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
>> allocation in bl_setup_layoutcommit().
>>
>> Signed-off-by: Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org>
>> ---
>> fs/nfs/pnfs.c | =C2=A0 42 +++++++++++++++++++++---------------------
>> 1 files changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index aedda1e..8474731 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -2073,15 +2073,23 @@ pnfs_layoutcommit_done(struct
>> pnfs_layoutcommit_data *data)
>> =C2=A0* Set up the argument/result storage required for the RPC call=
=2E
>> =C2=A0*/
>> static int
>> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int sy=
nc)
>> +pnfs_layoutcommit_setup(struct inode *inode,
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 struct pnfs_layoutcommit_data *data, int sync)
>> {
>> - =C2=A0 =C2=A0 =C2=A0 struct nfs_inode *nfsi =3D NFS_I(data->args.i=
node);
>> - =C2=A0 =C2=A0 =C2=A0 struct nfs_server *nfss =3D NFS_SERVER(data->=
args.inode);
>> + =C2=A0 =C2=A0 =C2=A0 struct nfs_inode *nfsi =3D NFS_I(inode);
>> + =C2=A0 =C2=A0 =C2=A0 struct nfs_server *nfss =3D NFS_SERVER(inode)=
;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0int result =3D 0;
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s Begin (sync:%d)\n", __func__,=
 sync);
>> + =C2=A0 =C2=A0 =C2=A0 data->is_sync =3D sync;
>> + =C2=A0 =C2=A0 =C2=A0 data->cred =C2=A0=3D nfsi->layoutcommit_ctx->=
cred;
>> + =C2=A0 =C2=A0 =C2=A0 data->ctx =3D nfsi->layoutcommit_ctx;
>> + =C2=A0 =C2=A0 =C2=A0 data->args.inode =3D inode;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->args.fh =3D NFS_FH(data->args.inode=
);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->args.layout_type =3D nfss->pnfs_cur=
r_ld->id;
>> + =C2=A0 =C2=A0 =C2=A0 pnfs_get_layout_stateid(&data->args.stateid, =
&nfsi->layout);
>> + =C2=A0 =C2=A0 =C2=A0 data->res.fattr =3D &data->fattr;
>> + =C2=A0 =C2=A0 =C2=A0 nfs_fattr_init(&data->fattr);
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* TODO: Need to determine the correct va=
lues */
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->args.time_modify_changed =3D 0;
>> @@ -2095,6 +2103,7 @@ pnfs_layoutcommit_setup(struct
>> pnfs_layoutcommit_data *data, int sync)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0i=
_size_read(&nfsi->vfs_inode) - 1);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->args.bitmask =3D nfss->attr_bitmask=
;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0data->res.server =3D nfss;
>> + =C2=A0 =C2=A0 =C2=A0 spin_unlock(&nfsi->lo_lock);
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Call layout driver to set the argument=
s.
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>> @@ -2104,10 +2113,6 @@ pnfs_layoutcommit_setup(struct
>> pnfs_layoutcommit_data *data, int sync)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (result)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0goto out;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0}
>> - =C2=A0 =C2=A0 =C2=A0 pnfs_get_layout_stateid(&data->args.stateid, =
&nfsi->layout);
>> - =C2=A0 =C2=A0 =C2=A0 data->res.fattr =3D &data->fattr;
>> - =C2=A0 =C2=A0 =C2=A0 nfs_fattr_init(&data->fattr);
>> -
>> out:
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s End Status %d\n", __func__, r=
esult);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0return result;
>> @@ -2131,36 +2136,31 @@ pnfs_layoutcommit_inode(struct inode *inode,=
 int
>> sync)
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOME=
M;
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock(&nfsi->lo_lock);
>> - =C2=A0 =C2=A0 =C2=A0 if (!nfsi->layoutcommit_ctx)
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_unlock;
>> -
>> - =C2=A0 =C2=A0 =C2=A0 data->args.inode =3D inode;
>> - =C2=A0 =C2=A0 =C2=A0 data->cred =C2=A0=3D nfsi->layoutcommit_ctx->=
cred;
>> - =C2=A0 =C2=A0 =C2=A0 data->ctx =3D nfsi->layoutcommit_ctx;
>> + =C2=A0 =C2=A0 =C2=A0 if (!nfsi->layoutcommit_ctx) {
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 spin_unlock(&nfsi=
->lo_lock);
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_free;
>> + =C2=A0 =C2=A0 =C2=A0 }
>>
>> - =C2=A0 =C2=A0 =C2=A0 /* Set up layout commit args*/
>> - =C2=A0 =C2=A0 =C2=A0 status =3D pnfs_layoutcommit_setup(data, sync=
);
>> + =C2=A0 =C2=A0 =C2=A0 /* Set up layout commit args */
>> + =C2=A0 =C2=A0 =C2=A0 status =3D pnfs_layoutcommit_setup(inode, dat=
a, sync);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (status)
>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_unlock;
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out_free;
>>
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Clear layoutcommit properties in the i=
node so
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 * new lc info can be generated
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 */
>> + =C2=A0 =C2=A0 =C2=A0 spin_lock(&nfsi->lo_lock);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_begin_pos =3D 0;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->pnfs_write_end_pos =3D 0;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0nfsi->layoutcommit_ctx =3D NULL;
>
> These should be cleared before the spin_lock is released in
> pnfs_layoutcommit_setup.
> They should be cleared (and the layoutcommit_cxt put) upon a layoutdr=
iver
> setup_layoutcommit failure.
>
> -->Andy
I just followed the old code's logic: if a layoutdriver's
setup_layoutcommit call failed(like return -ENOMEM), we just keep
nfs_inode's everything unchanged, so maybe next time we can try again
later. However, this will lead to lock lo_lock two times as in the
above code.
Maybe I should break the code's logic...
>>
>> -
>> - =C2=A0 =C2=A0 =C2=A0 /* release lock on pnfs layoutcommit attrs */
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_unlock(&nfsi->lo_lock);
>>
>> - =C2=A0 =C2=A0 =C2=A0 data->is_sync =3D sync;
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0status =3D pnfs4_proc_layoutcommit(data);
>> out:
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0dprintk("%s end (err:%d)\n", __func__, st=
atus);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0return status;
>> -out_unlock:
>> +out_free:
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0pnfs_layoutcommit_free(data);
>> - =C2=A0 =C2=A0 =C2=A0 spin_unlock(&nfsi->lo_lock);
>> =C2=A0 =C2=A0 =C2=A0 =C2=A0goto out;
>> }
>>
>> --
>> 1.6.3.3



--=20
tao.

Re: [RFC][PATCH] pnfs: unlock lo_lock spinlock before calling specific layoutdriver's setup_layoutcommit() operation.

[William A. (Andy) Adamson]

On Thu, May 20, 2010 at 1:13 PM, Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org> wrote:
> On Fri, May 21, 2010 at 12:13 AM, Andy Adamson <andros@netapp.com> wr=
ote:
>>
>> On May 20, 2010, at 6:25 AM, Tao Guo wrote:
>>
>>> So in blocklayoutdriver, we can use GFP_KERNEL to do memory
>>> allocation in bl_setup_layoutcommit().
>>>
>>> Signed-off-by: Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org>
>>> ---
>>> fs/nfs/pnfs.c | =A0 42 +++++++++++++++++++++---------------------
>>> 1 files changed, 21 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>>> index aedda1e..8474731 100644
>>> --- a/fs/nfs/pnfs.c
>>> +++ b/fs/nfs/pnfs.c
>>> @@ -2073,15 +2073,23 @@ pnfs_layoutcommit_done(struct
>>> pnfs_layoutcommit_data *data)
>>> =A0* Set up the argument/result storage required for the RPC call.
>>> =A0*/
>>> static int
>>> -pnfs_layoutcommit_setup(struct pnfs_layoutcommit_data *data, int s=
ync)
>>> +pnfs_layoutcommit_setup(struct inode *inode,
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct pnfs_layoutcom=
mit_data *data, int sync)
>>> {
>>> - =A0 =A0 =A0 struct nfs_inode *nfsi =3D NFS_I(data->args.inode);
>>> - =A0 =A0 =A0 struct nfs_server *nfss =3D NFS_SERVER(data->args.ino=
de);
>>> + =A0 =A0 =A0 struct nfs_inode *nfsi =3D NFS_I(inode);
>>> + =A0 =A0 =A0 struct nfs_server *nfss =3D NFS_SERVER(inode);
>>> =A0 =A0 =A0 =A0int result =3D 0;
>>>
>>> =A0 =A0 =A0 =A0dprintk("%s Begin (sync:%d)\n", __func__, sync);
>>> + =A0 =A0 =A0 data->is_sync =3D sync;
>>> + =A0 =A0 =A0 data->cred =A0=3D nfsi->layoutcommit_ctx->cred;
>>> + =A0 =A0 =A0 data->ctx =3D nfsi->layoutcommit_ctx;
>>> + =A0 =A0 =A0 data->args.inode =3D inode;
>>> =A0 =A0 =A0 =A0data->args.fh =3D NFS_FH(data->args.inode);
>>> =A0 =A0 =A0 =A0data->args.layout_type =3D nfss->pnfs_curr_ld->id;
>>> + =A0 =A0 =A0 pnfs_get_layout_stateid(&data->args.stateid, &nfsi->l=
ayout);
>>> + =A0 =A0 =A0 data->res.fattr =3D &data->fattr;
>>> + =A0 =A0 =A0 nfs_fattr_init(&data->fattr);
>>>
>>> =A0 =A0 =A0 =A0/* TODO: Need to determine the correct values */
>>> =A0 =A0 =A0 =A0data->args.time_modify_changed =3D 0;
>>> @@ -2095,6 +2103,7 @@ pnfs_layoutcommit_setup(struct
>>> pnfs_layoutcommit_data *data, int sync)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0i_size_read(&nfsi->vfs_inode) - 1);
>>> =A0 =A0 =A0 =A0data->args.bitmask =3D nfss->attr_bitmask;
>>> =A0 =A0 =A0 =A0data->res.server =3D nfss;
>>> + =A0 =A0 =A0 spin_unlock(&nfsi->lo_lock);
>>>
>>> =A0 =A0 =A0 =A0/* Call layout driver to set the arguments.
>>> =A0 =A0 =A0 =A0 */
>>> @@ -2104,10 +2113,6 @@ pnfs_layoutcommit_setup(struct
>>> pnfs_layoutcommit_data *data, int sync)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (result)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out;
>>> =A0 =A0 =A0 =A0}
>>> - =A0 =A0 =A0 pnfs_get_layout_stateid(&data->args.stateid, &nfsi->l=
ayout);
>>> - =A0 =A0 =A0 data->res.fattr =3D &data->fattr;
>>> - =A0 =A0 =A0 nfs_fattr_init(&data->fattr);
>>> -
>>> out:
>>> =A0 =A0 =A0 =A0dprintk("%s End Status %d\n", __func__, result);
>>> =A0 =A0 =A0 =A0return result;
>>> @@ -2131,36 +2136,31 @@ pnfs_layoutcommit_inode(struct inode *inode=
, int
>>> sync)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -ENOMEM;
>>>
>>> =A0 =A0 =A0 =A0spin_lock(&nfsi->lo_lock);
>>> - =A0 =A0 =A0 if (!nfsi->layoutcommit_ctx)
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_unlock;
>>> -
>>> - =A0 =A0 =A0 data->args.inode =3D inode;
>>> - =A0 =A0 =A0 data->cred =A0=3D nfsi->layoutcommit_ctx->cred;
>>> - =A0 =A0 =A0 data->ctx =3D nfsi->layoutcommit_ctx;
>>> + =A0 =A0 =A0 if (!nfsi->layoutcommit_ctx) {
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock(&nfsi->lo_lock);
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_free;
>>> + =A0 =A0 =A0 }
>>>
>>> - =A0 =A0 =A0 /* Set up layout commit args*/
>>> - =A0 =A0 =A0 status =3D pnfs_layoutcommit_setup(data, sync);
>>> + =A0 =A0 =A0 /* Set up layout commit args */
>>> + =A0 =A0 =A0 status =3D pnfs_layoutcommit_setup(inode, data, sync)=
;
>>> =A0 =A0 =A0 =A0if (status)
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_unlock;
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out_free;
>>>
>>> =A0 =A0 =A0 =A0/* Clear layoutcommit properties in the inode so
>>> =A0 =A0 =A0 =A0 * new lc info can be generated
>>> =A0 =A0 =A0 =A0 */
>>> + =A0 =A0 =A0 spin_lock(&nfsi->lo_lock);
>>> =A0 =A0 =A0 =A0nfsi->pnfs_write_begin_pos =3D 0;
>>> =A0 =A0 =A0 =A0nfsi->pnfs_write_end_pos =3D 0;
>>> =A0 =A0 =A0 =A0nfsi->layoutcommit_ctx =3D NULL;
>>
>> These should be cleared before the spin_lock is released in
>> pnfs_layoutcommit_setup.
>> They should be cleared (and the layoutcommit_cxt put) upon a layoutd=
river
>> setup_layoutcommit failure.
>>
>> -->Andy
> I just followed the old code's logic: if a layoutdriver's
> setup_layoutcommit call failed(like return -ENOMEM), we just keep
> nfs_inode's everything unchanged, so maybe next time we can try again
> later.

With a chance of another call immediately which will probably fail and =
so on

> However, this will lead to lock lo_lock two times as in the
> above code.

Note that when the setup_layoutcommit call succeeds, this code changes
the logic in that the old code did not give up the spin lock before
resetting the layout properties.

> Maybe I should break the code's logic...

I think so. If the layoutdriver has an error that it can recover from,
it should do so before returning. An ENOMEM error means that things
are really bad, and that the layoutdriver won't be able to do much of
anything.

Generally, we need to review the response to layoutcommit errors.

-->Andy


>>>
>>> -
>>> - =A0 =A0 =A0 /* release lock on pnfs layoutcommit attrs */
>>> =A0 =A0 =A0 =A0spin_unlock(&nfsi->lo_lock);
>>>
>>> - =A0 =A0 =A0 data->is_sync =3D sync;
>>> =A0 =A0 =A0 =A0status =3D pnfs4_proc_layoutcommit(data);
>>> out:
>>> =A0 =A0 =A0 =A0dprintk("%s end (err:%d)\n", __func__, status);
>>> =A0 =A0 =A0 =A0return status;
>>> -out_unlock:
>>> +out_free:
>>> =A0 =A0 =A0 =A0pnfs_layoutcommit_free(data);
>>> - =A0 =A0 =A0 spin_unlock(&nfsi->lo_lock);
>>> =A0 =A0 =A0 =A0goto out;
>>> }
>>>
>>> --
>>> 1.6.3.3
>
>
>
> --
> tao.
> --
> 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 =A0http://vger.kernel.org/majordomo-info.html
>