[pnfs] [PATCH 16/37] pnfs: allocate layout driver data in one piece with pnfs_layout_type

[Dean Hildebrand]

Benny Halevy wrote:
> On Jan. 03, 2008, 0:39 +0200, Dean Hildebrand <seattleplus at gmail.com> wrote:
>   
>>> layout driver data comes right after the pnfs_layout_type fields
>>> and can be accessed with the PNFS_LD_DATA accessor function.
>>>       
>>     
>>>  
>>>  /* Free a filelayout layout structure
>>> @@ -429,17 +418,11 @@ void
>>>  filelayout_free_layout(struct pnfs_layout_type **layoutidp,
>>>  		       struct nfs4_pnfs_layout_segment *range)
>>>  {
>>> -	struct nfs4_filelayout *nfslay = NULL;
>>>  	struct pnfs_layout_type *layoutid;
>>>  
>>>  	dprintk("NFS_FILELAYOUT: freeing layout\n");
>>>  
>>>  	layoutid = *layoutidp;
>>> -	if (layoutid)
>>> -		nfslay = (struct nfs4_filelayout *)layoutid->layoutid;
>>> -	if (nfslay != NULL)
>>> -		kfree(nfslay);
>>> -	kfree(layoutid);
>>>  	*layoutidp = NULL;
>>>  }
>>>  
>>>   
>>>       
>> The above code seems to remove some parts of the function but leaves the 
>> "layoutid = *layoutidp" part for some reason?  Where does the memory get 
>> freed?
>>     
>
> Oops. You're right, kfree(layoutid) should've stayed.
> Thanks!
>
>   
>> There seems to be a lot more happening in this patch than just a 
>> conversion to accessor functions as described at the top.  Does it need 
>> a more expansive comment or is extra code being included?
>>     
>
> What exactly? The patch just replaces the separate allocations of
> pnfslayout and pnfslayout->layoutid with a single allocation of the unified
> structure and replaces all references to pnfslayout->layoutid to
> PNFS_LD_DATA(pnfslayout).
>   
Now this comment is great! :)
Dean
>   
>>     
>>> @@ -459,9 +442,7 @@ filelayout_set_layout(struct pnfs_layout_type *layoutid,
>>>  
>>>  	if (!layoutid)
>>>  		goto nfserr;
>>> -	fl = (struct nfs4_filelayout *)layoutid->layoutid;
>>> -	if (!fl)
>>> -		goto nfserr;
>>> +	fl = PNFS_LD_DATA(layoutid);
>>>  
>>>  	READ32(fl->dev_id);
>>>  	READ32(nfl_util);
>>> @@ -503,7 +484,6 @@ filelayout_commit(struct pnfs_layout_type *layoutid, struct list_head *pages,
>>>  {
>>>  	struct inode *ino = PNFS_INODE(layoutid);
>>>  	struct nfs_write_data   *dsdata = NULL;
>>> -	struct pnfs_layout_type *laytype;
>>>  	struct nfs4_filelayout *nfslay;
>>>  	struct nfs4_pnfs_dev_item *dev;
>>>  	struct nfs4_pnfs_dev *fdev;
>>> @@ -514,8 +494,7 @@ filelayout_commit(struct pnfs_layout_type *layoutid, struct list_head *pages,
>>>  	struct list_head *pos, *tmp;
>>>  	int i;
>>>  
>>> -	laytype = NFS_I(ino)->current_layout;
>>> -	nfslay = (struct nfs4_filelayout *)layoutid->layoutid;
>>> +	nfslay = PNFS_LD_DATA(layoutid);
>>>  
>>>  	dprintk("%s data %p pnfs_client %p nfslay %p\n",
>>>  			__FUNCTION__, data, data->pnfs_client, nfslay);
>>> @@ -599,7 +578,7 @@ out_bad:
>>>  ssize_t
>>>  filelayout_get_stripesize(struct pnfs_layout_type *layoutid)
>>>  {
>>> -	struct nfs4_filelayout *fl = (struct nfs4_filelayout *)layoutid->layoutid;
>>> +	struct nfs4_filelayout *fl = PNFS_LD_DATA(layoutid);
>>>  	ssize_t stripesize = fl->stripe_unit;
>>>  	return stripesize;
>>>  }
>>> diff --git a/include/linux/nfs4_pnfs.h b/include/linux/nfs4_pnfs.h
>>> index 6a98f59..d862aac 100644
>>> --- a/include/linux/nfs4_pnfs.h
>>> +++ b/include/linux/nfs4_pnfs.h
>>> @@ -37,9 +37,9 @@ struct pnfs_mount_type {
>>>   * A reference is stored in the nfs_inode structure.
>>>   */
>>>  struct pnfs_layout_type {
>>> -	void *layoutid;
>>> -	int roc_iomode;	/* iomode to return on close, 0=none */
>>> +	int roc_iomode;			/* iomode to return on close, 0=none */
>>>  	struct inode *inode;
>>> +	u8 ld_data[];			/* layout driver private data */
>>>  };
>>>  
>>>  static inline struct inode *
>>> @@ -66,6 +66,12 @@ PNFS_MOUNTID(struct pnfs_layout_type *lo)
>>>  	return NFS_SERVER(PNFS_INODE(lo))->pnfs_mountid;
>>>  }
>>>  
>>> +static inline void *
>>> +PNFS_LD_DATA(struct pnfs_layout_type *lo)
>>> +{
>>> +	return lo->ld_data;
>>> +}
>>> +
>>>  static inline struct pnfs_layoutdriver_type *
>>>  PNFS_LD(struct pnfs_layout_type *lo)
>>>  {
>>>   
>>>       
>
>