Re: [PATCH v3 06/25] pnfs: cleanup_layoutcommit

[Boaz Harrosh <bharrosh@panasas.com>]

On 07/27/2011 01:59 PM, Trond Myklebust wrote:
> On Wed, 2011-07-27 at 13:42 -0700, Boaz Harrosh wrote: 
>> On 07/27/2011 01:25 PM, Trond Myklebust wrote:
>>> On Wed, 2011-07-27 at 13:20 -0700, Boaz Harrosh wrote: 
>>>> On 07/27/2011 12:53 PM, Trond Myklebust wrote:
>>>>>> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
>>>>>> index bddd8b9..f271425 100644
>>>>>> --- a/fs/nfs/pnfs.h
>>>>>> +++ b/fs/nfs/pnfs.h
>>>>>> @@ -113,6 +113,9 @@ struct pnfs_layoutdriver_type {
>>>>>>  				     struct xdr_stream *xdr,
>>>>>>  				     const struct nfs4_layoutreturn_args *args);
>>>>>>  
>>>>>> +	void (*cleanup_layoutcommit) (struct pnfs_layout_hdr *layoutid,
>>>>>> +				      struct nfs4_layoutcommit_data *data);
>>>>>> +
>>>>>>  	void (*encode_layoutcommit) (struct pnfs_layout_hdr *layoutid,
>>>>>>  				     struct xdr_stream *xdr,
>>>>>>  				     const struct nfs4_layoutcommit_args *args);
>>>>>
>>>>> This really needs to go. We should have
>>>>>
>>>>>       int (*layoutcommit)()...
>>>>>
>>>>> instead of 'encode' and 'cleanup' methods...
>>>>>
>>>>
>>>> Theoretically it is not possible because the blocks-layout protocol mandates
>>>> different handling depending on the "error" response from the Server which
>>>> will be received on RPC done.
>>>
>>> ???? If the blocks code is in charge of actually doing the RPC call, why
>>> would it not be able to perform its own error handling?
>>>
>>
>> Is it? I thought it was the Generic layer that was Initiating the layoutcommit
>> (From the pnfs_layoutcommit_inode called from nfs_write_inode)
>>
>> The LD only has a chance to encode the payload on rpc-setup and here the blocks
>> code needs cleanup depending on the return-status of rpc-done
>>
>> [I do think that setup and done might be better names to reflect the rpc states)
> 
> I'm suggesting replacing the version in the generic layer with
> per-layout-type variants. When the only thing that is common between the
> 3 variants is a couple of lines of xdr, then it doesn't make sense IMO
> to try to share.
> 

You lost me. What are you suggesting to replace? pnfs_layoutcommit_inode?
nfs4_proc_layoutcommit? nfs4_xdr_enc_layoutcommit? At what level do you want to
switch to a per LD handling.

If I look at the all layoutcommit stack the actual xdr encoding is the least of
the common code. The housekeeping is the most of it. I do not see a clear point
in current code that we can make a clean cut. And that gives us a place that:
1. Already have an xdr buffer to encode into.
2. Also sees the return code from layoutcommit_done

We used to allocated the layoutcommit buffer twice and that didn't solve that
problem either. It is two stages of the rpc-state. I don't see how it can be
briged

Boaz