[pnfs] [PATCH 18/37] pnfs: client layout cache: introduce get_alloc_layout

[Benny Halevy]

On Jan. 03, 2008, 0:50 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
> 
> Benny Halevy wrote:
>> get_alloc_layout gets or allocates if needed the pnfs_layout_type structure
>> alloc_init_layout calls the layout driver's alloc_layout function
>> and initializes the allocated pnfs_layout_type struct.
>>
>> Signed-off-by: Benny Halevy <bhalevy at panasas.com>
>> ---
>>  fs/nfs/pnfs.c             |   79 ++++++++++++++++++++++++++++++++-------------
>>  include/linux/nfs4_pnfs.h |    1 +
>>  include/linux/nfs_fs.h    |    2 +-
>>  3 files changed, 58 insertions(+), 24 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index cb8115f..bd10670 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -403,38 +403,65 @@ out:
>>   *     the I/O module has its read/write methods called.
>>   */
>>  static struct pnfs_layout_type *
>> -pnfs_inject_layout(struct nfs_inode *nfsi,
>> -		   struct layoutdriver_io_operations *io_ops,
>> +pnfs_inject_layout(struct pnfs_layout_type *layid,
>>  		   struct nfs4_pnfs_layoutget_res *lgr)
>>  {
>> -	struct pnfs_layout_type *layid;
>> -	struct inode *inode = &nfsi->vfs_inode;
>> -	struct nfs_server *server = NFS_SERVER(inode);
>> +	struct layoutdriver_io_operations *io_ops = PNFS_LD_IO_OPS(layid);
>>  
>>  	dprintk("%s Begin\n", __FUNCTION__);
>>  
>> -	if (!io_ops->alloc_layout || !io_ops->set_layout) {
>> +	if (!io_ops->set_layout) {
>>  		printk(KERN_ERR "%s ERROR! Layout driver lacking pNFS layout ops!!!\n", __FUNCTION__);
>>  		return NULL;
>>  	}
>>  
>> -	if (nfsi->current_layout == NULL) {
>> -		dprintk("%s Alloc'ing layout\n", __FUNCTION__);
>> -		layid = io_ops->alloc_layout(server->pnfs_mountid, inode);
>> -		if (layid)
>> -			layid->inode = inode;
>> -	} else {
>> -		dprintk("%s Adding to current layout\n", __FUNCTION__);
>> -		layid = nfsi->current_layout;
>> -	}
>> +	dprintk("%s Calling set layout\n", __FUNCTION__);
>> +	return io_ops->set_layout(layid, lgr);
>> +}
>> +
>> +static struct pnfs_layout_type *
>> +alloc_init_layout(struct inode *ino, struct layoutdriver_io_operations *io_ops)
>> +{
>> +	struct pnfs_layout_type *lo;
>>  
>> -	if (!layid) {
>> -		printk(KERN_ERR "%s ERROR! Layout id non-existent!!!\n",
>> -		       __FUNCTION__);
>> +	lo = io_ops->alloc_layout(NFS_SERVER(ino)->pnfs_mountid, ino);
>>   
> Since we checked for set_layout in pnfs_inject _layout, maybe we should 
> check for alloc_layout here?

As with free_layout I think we better mandate the implementation of
alloc_layout and free_layout to keep things simpler.
set_layout goes away in a later patch and becomes part of alloc_lseg
which is mandated at pnfs_register_layoutdriver time.

Benny

<snip>
>> +static struct pnfs_layout_type *
>> +get_alloc_layout(struct inode *ino,
>> +		   struct layoutdriver_io_operations *io_ops)
>> +{
>> +	struct nfs_inode *nfsi = NFS_I(ino);
>> +	struct pnfs_layout_type *lo;
>> +
>> +	dprintk("%s Begin\n", __FUNCTION__);
>> +
>> +	lo = nfsi->current_layout;
>> +	if (lo)
>> +		goto out;
>> +
>> +	lo = nfsi->current_layout = alloc_init_layout(ino, io_ops);
>> +	if (!lo) {
>> +		lo = ERR_PTR(-ENOMEM);
>> +		goto err;
>>  	}
>> -	dprintk("%s Calling set layout\n", __FUNCTION__);
>> -	return io_ops->set_layout(layid, lgr);
>> +
>> +out:
>> +	dprintk("%s Return %p\n", __FUNCTION__, lo);
>> +	return lo;
>> +
>> +err:
>> +	dprintk("%s Return error %ld\n", __FUNCTION__, PTR_ERR(lo));
>> +	return lo;
>>  }
>>   
> I like to use goto's if we can save duplicated code, but I'm thinking 
> that these dprintk's could be moved to their respective places within 
> the function since they are each called only once and actually increase 
> the number of lines of code.  

Actually the dprintk after the out: label is reachable via both success
paths (once when the the current_layout is already allocated and the other
is when alloc_init_layout succeeds.  The reason I like having the printk's in
common (success or error) paths is that in many other places the start off in
some specific path and in time other return paths are being implemented and
in many cases the respective dprintk is just not printed in the new paths.
Putting them on the entrance, and on the success and error return paths 
make sure they are always printed which greatly helps debugging via log
printouts.

> Also, if we are having memory problems, 
> I'd be up for using a printk instead of a dprintk.

That's possible to add. Will do.

Benny

> Dean
>>