[pnfs] [PATCH 1/1] Replaced BUG() with BUG_ON()

[Benny Halevy]

Iyer, Rahul wrote:
>  
> 
>> -----Original Message-----
>> From: Benny Halevy [mailto:bhalevy at panasas.com] 
>> Sent: Wednesday, May 02, 2007 1:20 PM
>> To: Iyer, Rahul
>> Cc: pnfs at linux-nfs.org
>> Subject: Re: [pnfs] [PATCH 1/1] Replaced BUG() with BUG_ON()
>>
>> The problem I hit is caused by the fact that that I'm not 
>> using the file layout.
>>
>> nfs_write_rpcsetup() does not set data->session before 
>> calling pnfs_try_to_write_data().
>>
>> When we do the pnfs write via the layout driver there's 
>> indeed no session in my case.
>>
>> The files layout driver sets data->session to the DS session as
>> 		data->session = dserver.dev_item->clp->cl_session;
>>
>> and on the async callback path pnfs4_write_done() is called.
>> [note, without properly going through pnfs4_proc_write_setup 
>> in my case, it is called only when writing to the MDS to to 
>> the DS via calls to nfs_initiate_write()]
>>
>> pnfs4_write_done(), in turn, calls the 
>> NFS_SERVER(inode)->rpc_ops-> sequence_done if not null, 
>> unconditionally in all cases.
>>
>> The fix I tested is attached.  It calls sequence_done only if 
>> data->session is not NULL.
>>
> 
> Yep! The patch looks good.
>  
>> Another question is whether calling 
>> server->rpc_ops->sequence_done is appropriate both for the 
>> MDS and for the DS case.
>>
> Isn't it appropriate? There should always be a session between the MDS
> and the client regardless of the protocol being used between the client
> and the DS.

right but this session might have different rpc_ops than server->rpc_ops
(server is NFS_SERVER(inode))

If you look in pnfs4_proc_write_setup(), setup_sequence
is called differently for the MDS and the DS cases, so 
I thought sequence_done might need similar handling...

	if (session == server->nfs4_state->cl_session) {
		server->rpc_ops->setup_sequence(session,
					data->args.minorversion_info,
					data->res.minorversion_info);
	}
	else {
		_nfs41_proc_setup_sequence(session,
				data->args.minorversion_info,
				data->res.minorversion_info);
	}


> Thanks
> Regards
> Rahul
> 
>  
>> Benny
>>  
>> Benny Halevy wrote:
>>> iyer at netapp.com wrote:
>>>> From: Rahul Iyer <iyer at netapp.com>
>>>>
>>>> Replaced instances of
>>>>
>>>> if (....)
>>>> 	BUG();
>>>>
>>>> with
>>>>
>>>> BUG_on(...);
>>>>
>>>> Signed-off-by: Rahul Iyer <iyer at netapp.com>
>>>> ---
>>>>  fs/nfs/nfs4proc.c |   31 +++++++++++++------------------
>>>>  1 files changed, 13 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c index 
>>>> b4ce093..89702cf 100644
>>>> --- a/fs/nfs/nfs4proc.c
>>>> +++ b/fs/nfs/nfs4proc.c
>>>> @@ -222,10 +222,9 @@ static int 
>> nfs41_proc_sequence_done(struct nfs4_session *session, struct nfs41_s
>>>>  	unsigned long timestamp;
>>>>  	struct nfs4_client *clp;
>>>>  
>>>> -	if (!session || !(clp = session->client)) {
>>>> -		printk(KERN_EMERG "%s is NULL!!!\n", 
>> (!session)?"session":"clp");
>>>> -		BUG();
>>>> -	}
>>>> +	BUG_ON(!session);
>>> Rahul, this BUG_ON introduces a bug :-( Previously the 
>> function just 
>>> returned if !session, now it BUG()'s.
>>>
>>>> +	clp = session->client;
>>>> +	BUG_ON(!clp);
>>>>  
>>> _______________________________________________
>>> pNFS mailing list
>>> pNFS at linux-nfs.org
>>> http://linux-nfs.org/cgi-bin/mailman/listinfo/pnfs
>>