[pnfs] Status meeting pNFS

[Benny Halevy]

Marc Eshel wrote:
> "William A.(Andy) Adamson" <andros at citi.umich.edu> wrote on 08/30/2006 
> 11:11:41 AM:
>
>   
>> hi benny
>>
>>
>>     
>>> I'd like to discuss some nfsd internal API issues:
>>>   - first parameter for layout_encode is currently unsigned int 
>>>       
> **resp, 
>   
>>> shouldn't it be
>>>     struct nfsd4_compoundres *resp like for the filelayout_encode 
>>>       
> methods?
>   
>> dean and i looked - and indeed this is a bug.
>>
>> what the encode functions want is to dereference the u32 * resp->p which 
>>     
>
>   
>> points to the location into the encode xdr buf.
>>
>> the current code works ONLY because the nfsd4_compoundres struct has 'p' 
>>     
> as 
>   
>> the first element in the structure:
>>
>>
>> struct nfsd4_compoundres {
>>         /* scratch variables for XDR encode */
>>         u32 *                           p;
>>         u32 *                           end;
>>         struct xdr_buf *                xbuf;
>>
>> and therefore, dereferencing the *resp as an unsigned int ** gives 
>> you resp->p.
>>
>> this is a bug. the declaration is cool, but fs/nfsd/nfs4xdr.c: 
>> nfsd4_encode_layoutget() needs to be changed as follows
>>
>>     
>
> Another alternative is to use *resp in the prototype and extract 'p' in 
> the routine that we call. This would be cleaner allowing the routine to 
> also update 'p' so the caller can tell where we are in the buffer without 
> any extra parameter that return the length, and we can use the returned 
> int for error code instead.
>
> Marc.
>  
>   

I also like this approach better, especially since it allows using 
semi-standard XDR
macros RESERVE_SPACE, WRITE* or ADJUST_ARGS.
If we want to provide a solution which is independent from xdr I think
we should also provide a size_t parameter with the maximum allowed 
buffer size.
the declaration would look like layout_get(unsigned int **p, size_t 
maxlength, void *layout);

>> diff -puN fs/nfsd/nfs4xdr.c~fix-pnfsd-encode-layout fs/nfsd/nfs4xdr.c
>> --- pnfs/fs/nfsd/nfs4xdr.c~fix-pnfsd-encode-layout      2006-08-30 
>> 13:37:09.000000000 -0400
>> +++ pnfs-andros/fs/nfsd/nfs4xdr.c       2006-08-30 13:37:33.000000000 
>>     
> -0400
>   
>> @@ -2785,11 +2785,11 @@ nfsd4_encode_layoutget(struct nfsd4_comp
>>                 if (lgp->lg_ops->layout_encode == NULL &&
>>                                 lgp->lg_type == LAYOUT_NFSV4_FILES)
>>                 {
>> -                       filelayout_encode_layout(resp, lgp->lg_layout);
>> +                       filelayout_encode_layout(&resp->p, 
>>     
> lgp->lg_layout);
>   
>>                         filelayout_free_layout(lgp->lg_layout);
>>                 }
>>                 else {
>> -                       lgp->lg_ops->layout_encode((void *)resp, lgp->
>> lg_layout);+                       lgp->lg_ops->layout_encode(&resp->p, 
>>     
> lgp->
>   
>> lg_layout);
>>                         lgp->lg_ops->layout_free(lgp->lg_layout);
>>                 }
>>         }
>>
>>
>>
>>
>>     
>>>   - similarly for devaddr_encode (currently it's void *)
>>>       
>> dean and i think that devaddr_encode should be changed to
>>
>>         void (*devaddr_encode)(unsigned int **, void * devaddr);
>>
>> and have &resp->p passed as the first argument, just as in the fix 
>> above. this 
>> is so layout modules will not have to include nfs .h files in order 
>> to compile.
>>
>>
>>     
>>>   - can the encode functions return int status rather than void?
>>>     It's possible in our case to get an internal failure here.
>>>       
>> yes. good idea.
>>
>>     
>>>   - layout_get and layout_return API get a void * as second parameter
>>>     but actually a struct nfsd4_pnfs_layout{get,return} * pointer is 
>>>       
> passed
>   
>>>     and they need to typecast the void * internally.  Can't the API be 
>>>       
>
>   
>>> formalized?
>>>       
>> we could follow the pattern of include/linux/nfs_fs_i.h which is 
>>     
> included in 
>   
>> include/linux/fs.h and formalize the API. ok by me.
>>
>>
>> as far as the rest of your message concerning pnfsd layoutget error 
>> processing 
>> - i'll respond later after some thought..
>>
>> -->Andy
>>
>>