[pnfs] Status meeting pNFS

[Marc Eshel]

"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.
 
> 
> 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
>