[10/13,RFC] Bugs in new pnfs write path
diff mbox

Message ID 1305973982-17499-1-git-send-email-bharrosh@panasas.com
State New, archived
Headers show

Commit Message

Boaz Harrosh May 21, 2011, 10:33 a.m. UTC
1. In nfs4_write_done_cb: data->write_done_cb comes with a NULL.
   Just as a guess I call nfs4_write_done_cb() just above it
   it looked like the right thing todo. With that in, I'm able
   to write things to file When converting pnfs.c:258 to a WARN_ON.

   Benny we might want to set data->write_done_cb somewhere in the
   none-rpc path? where is it best to do that?

2. In pnfs_ld_write_done:
	put_lseg(data->lseg);
	data->lseg = NULL;
   was done before the call to pnfs_set_layoutcommit()
   which trys to get_lseg() on that same data->lseg.

3. In pnfs_ld_write_done:
   data->mds_ops->rpc_call_done(NULL, data);
   crashes with a NULL task. Just pass it with &data->task

   Which calls for a cleanup. There is bunch of functions
   with [task, write_data] API. And the task is always
   write_data->task

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/nfs/nfs4proc.c |    3 ++-
 fs/nfs/pnfs.c     |   10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

Benny Halevy May 22, 2011, 6:08 p.m. UTC | #1
On 2011-05-21 13:33, Boaz Harrosh wrote:
> 1. In nfs4_write_done_cb: data->write_done_cb comes with a NULL.

what's the call path?

>    Just as a guess I call nfs4_write_done_cb() just above it
>    it looked like the right thing todo. With that in, I'm able
>    to write things to file When converting pnfs.c:258 to a WARN_ON.
> 
>    Benny we might want to set data->write_done_cb somewhere in the
>    none-rpc path? where is it best to do that?

we're not supposed to get there in the non-rpc path...
The non-rpc drivers must call pnfs_ld_write_done.

> 
> 2. In pnfs_ld_write_done:
> 	put_lseg(data->lseg);
> 	data->lseg = NULL;
>    was done before the call to pnfs_set_layoutcommit()
>    which trys to get_lseg() on that same data->lseg.

good catch, thanks!

> 
> 3. In pnfs_ld_write_done:
>    data->mds_ops->rpc_call_done(NULL, data);
>    crashes with a NULL task. Just pass it with &data->task

As we don't go through nfs_initiate_write data->task is not initialized.
Where's the crash exactly?
We better fix it than fake a task structure...

Benny

> 
>    Which calls for a cleanup. There is bunch of functions
>    with [task, write_data] API. And the task is always
>    write_data->task
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  fs/nfs/nfs4proc.c |    3 ++-
>  fs/nfs/pnfs.c     |   10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 759523a..1a53187 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3250,7 +3250,8 @@ static int nfs4_write_done(struct rpc_task *task, struct nfs_write_data *data)
>  {
>  	if (!nfs4_sequence_done(task, &data->res.seq_res))
>  		return -EAGAIN;
> -	return data->write_done_cb(task, data);
> +	return data->write_done_cb ? data->write_done_cb(task, data) :
> +		nfs4_write_done_cb(task, data);
>  }
>  
>  /* Reset the the nfs_write_data to send the write to the MDS. */
> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
> index 17d0c4c..b04cdb4 100644
> --- a/fs/nfs/pnfs.c
> +++ b/fs/nfs/pnfs.c
> @@ -255,7 +255,7 @@ put_lseg_common(struct pnfs_layout_segment *lseg)
>  {
>  	struct inode *inode = lseg->pls_layout->plh_inode;
>  
> -	BUG_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
> + 	WARN_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
>  	list_del_init(&lseg->pls_list);
>  	if (list_empty(&lseg->pls_layout->plh_segs)) {
>  		set_bit(NFS_LAYOUT_DESTROYED, &lseg->pls_layout->plh_flags);
> @@ -1124,15 +1124,17 @@ pnfs_ld_write_done(struct nfs_write_data *data)
>  {
>  	int status;
>  
> -	put_lseg(data->lseg);
> -	data->lseg = NULL;
>  	if (!data->pnfs_error) {
>  		pnfs_set_layoutcommit(data);
> -		data->mds_ops->rpc_call_done(NULL, data);
> +		data->mds_ops->rpc_call_done(&data->task, data);
>  		data->mds_ops->rpc_release(data);
> +		put_lseg(data->lseg);
> +		data->lseg = NULL;
>  		return 0;
>  	}
>  
> +	put_lseg(data->lseg);
> +	data->lseg = NULL;
>  	dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
>  		data->pnfs_error);
>  	status = nfs_initiate_write(data, NFS_CLIENT(data->inode), data->mds_ops, NFS_FILE_SYNC);

--
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
Boaz Harrosh May 23, 2011, 4:19 a.m. UTC | #2
On 05/22/2011 09:08 PM, Benny Halevy wrote:

This is the old patch please see the one that actually works
I sent it by itself after this one

> On 2011-05-21 13:33, Boaz Harrosh wrote:
>> 1. In nfs4_write_done_cb: data->write_done_cb comes with a NULL.
> 
> what's the call path?
> 

I sent it in my mails
it is eventually called from pnfs_ld_write_done trhough
data->mds_ops->rpc_call_done(&data->task, data);

>>    Just as a guess I call nfs4_write_done_cb() just above it
>>    it looked like the right thing todo. With that in, I'm able
>>    to write things to file When converting pnfs.c:258 to a WARN_ON.
>>
>>    Benny we might want to set data->write_done_cb somewhere in the
>>    none-rpc path? where is it best to do that?
> 
> we're not supposed to get there in the non-rpc path...
> The non-rpc drivers must call pnfs_ld_write_done.
> 

Yep! we do. And please don't touch anything, everything works perfectly
now.

As I said it gets called from pnfs_ld_write_done through 
data->mds_ops->rpc_call_done(&data->task, data);

>>
>> 2. In pnfs_ld_write_done:
>> 	put_lseg(data->lseg);
>> 	data->lseg = NULL;
>>    was done before the call to pnfs_set_layoutcommit()
>>    which trys to get_lseg() on that same data->lseg.
> 
> good catch, thanks!
> 
>>
>> 3. In pnfs_ld_write_done:
>>    data->mds_ops->rpc_call_done(NULL, data);
>>    crashes with a NULL task. Just pass it with &data->task
> 
> As we don't go through nfs_initiate_write data->task is not initialized.
> Where's the crash exactly?
> We better fix it than fake a task structure...
> 
> Benny
> 
>>
>>    Which calls for a cleanup. There is bunch of functions
>>    with [task, write_data] API. And the task is always
>>    write_data->task
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> ---
>>  fs/nfs/nfs4proc.c |    3 ++-
>>  fs/nfs/pnfs.c     |   10 ++++++----
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 759523a..1a53187 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -3250,7 +3250,8 @@ static int nfs4_write_done(struct rpc_task *task, struct nfs_write_data *data)
>>  {
>>  	if (!nfs4_sequence_done(task, &data->res.seq_res))
>>  		return -EAGAIN;
>> -	return data->write_done_cb(task, data);
>> +	return data->write_done_cb ? data->write_done_cb(task, data) :
>> +		nfs4_write_done_cb(task, data);
>>  }
>>  
>>  /* Reset the the nfs_write_data to send the write to the MDS. */
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 17d0c4c..b04cdb4 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -255,7 +255,7 @@ put_lseg_common(struct pnfs_layout_segment *lseg)
>>  {
>>  	struct inode *inode = lseg->pls_layout->plh_inode;
>>  
>> -	BUG_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
>> + 	WARN_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
>>  	list_del_init(&lseg->pls_list);
>>  	if (list_empty(&lseg->pls_layout->plh_segs)) {
>>  		set_bit(NFS_LAYOUT_DESTROYED, &lseg->pls_layout->plh_flags);
>> @@ -1124,15 +1124,17 @@ pnfs_ld_write_done(struct nfs_write_data *data)
>>  {
>>  	int status;
>>  
>> -	put_lseg(data->lseg);
>> -	data->lseg = NULL;
>>  	if (!data->pnfs_error) {
>>  		pnfs_set_layoutcommit(data);
>> -		data->mds_ops->rpc_call_done(NULL, data);
>> +		data->mds_ops->rpc_call_done(&data->task, data);
>>  		data->mds_ops->rpc_release(data);
>> +		put_lseg(data->lseg);
>> +		data->lseg = NULL;
>>  		return 0;
>>  	}
>>  
>> +	put_lseg(data->lseg);
>> +	data->lseg = NULL;
>>  	dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
>>  		data->pnfs_error);
>>  	status = nfs_initiate_write(data, NFS_CLIENT(data->inode), data->mds_ops, NFS_FILE_SYNC);
> 

--
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
Boaz Harrosh May 23, 2011, 4:26 a.m. UTC | #3
On 05/23/2011 07:19 AM, Boaz Harrosh wrote:
>>> 3. In pnfs_ld_write_done:
>>>    data->mds_ops->rpc_call_done(NULL, data);
>>>    crashes with a NULL task. Just pass it with &data->task
>>
>> As we don't go through nfs_initiate_write data->task is not initialized.
>> Where's the crash exactly?
>> We better fix it than fake a task structure...
>>

We better do have a valid data->task because we use it all over
in the objects layout driver, to schedule out of the interrupt.

It works fine I tested it

>> Benny

Boaz
--
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
diff mbox

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 759523a..1a53187 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3250,7 +3250,8 @@  static int nfs4_write_done(struct rpc_task *task, struct nfs_write_data *data)
 {
 	if (!nfs4_sequence_done(task, &data->res.seq_res))
 		return -EAGAIN;
-	return data->write_done_cb(task, data);
+	return data->write_done_cb ? data->write_done_cb(task, data) :
+		nfs4_write_done_cb(task, data);
 }
 
 /* Reset the the nfs_write_data to send the write to the MDS. */
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 17d0c4c..b04cdb4 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -255,7 +255,7 @@  put_lseg_common(struct pnfs_layout_segment *lseg)
 {
 	struct inode *inode = lseg->pls_layout->plh_inode;
 
-	BUG_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
+ 	WARN_ON(test_bit(NFS_LSEG_VALID, &lseg->pls_flags));
 	list_del_init(&lseg->pls_list);
 	if (list_empty(&lseg->pls_layout->plh_segs)) {
 		set_bit(NFS_LAYOUT_DESTROYED, &lseg->pls_layout->plh_flags);
@@ -1124,15 +1124,17 @@  pnfs_ld_write_done(struct nfs_write_data *data)
 {
 	int status;
 
-	put_lseg(data->lseg);
-	data->lseg = NULL;
 	if (!data->pnfs_error) {
 		pnfs_set_layoutcommit(data);
-		data->mds_ops->rpc_call_done(NULL, data);
+		data->mds_ops->rpc_call_done(&data->task, data);
 		data->mds_ops->rpc_release(data);
+		put_lseg(data->lseg);
+		data->lseg = NULL;
 		return 0;
 	}
 
+	put_lseg(data->lseg);
+	data->lseg = NULL;
 	dprintk("%s: pnfs_error=%d, retry via MDS\n", __func__,
 		data->pnfs_error);
 	status = nfs_initiate_write(data, NFS_CLIENT(data->inode), data->mds_ops, NFS_FILE_SYNC);