Re: [PATCH 09/12] SQUASHME: pnfs-obj: Bugs in new global-device-cache code

[Benny Halevy <bhalevy@panasas.com>]

On 2011-05-24 18:07, Boaz Harrosh wrote:
> Fix BUGs in the new "Use global-device-cache".
> 
> One thing I don't understand is why the compiler did
> not complain when the code was returning the wrong
> type of structure
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  fs/nfs/objlayout/objio_osd.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/objlayout/objio_osd.c b/fs/nfs/objlayout/objio_osd.c
> index 167cd1e..faacde2 100644
> --- a/fs/nfs/objlayout/objio_osd.c
> +++ b/fs/nfs/objlayout/objio_osd.c
> @@ -56,6 +56,7 @@ objio_free_deviceid_node(struct nfs4_deviceid_node *d)
>  {
>  	struct objio_dev_ent *de = container_of(d, struct objio_dev_ent, id_node);
>  
> +	dprintk("%s: free od=%p\n", __func__, de->od);
>  	osduld_put_device(de->od);
>  	kfree(de);
>  }
> @@ -64,14 +65,18 @@ static struct objio_dev_ent *_dev_list_find(const struct nfs_server *nfss,
>  	const struct nfs4_deviceid *d_id)
>  {
>  	struct nfs4_deviceid_node *d;
> +	struct objio_dev_ent *de;
>  
>  	d = nfs4_find_get_deviceid(nfss->pnfs_curr_ld, nfss->nfs_client, d_id);
>  	if (!d)
>  		return NULL;
> -	return container_of(d, struct objio_dev_ent, id_node);
> +
> +	de = container_of(d, struct objio_dev_ent, id_node);
> +	return de;

That's not really required as container_of() does the type casting
for you.

>  }
>  
> -static int _dev_list_add(const struct nfs_server *nfss,
> +static struct objio_dev_ent *
> +_dev_list_add(const struct nfs_server *nfss,
>  	const struct nfs4_deviceid *d_id, struct osd_dev *od,
>  	gfp_t gfp_flags)
>  {
> @@ -79,9 +84,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
>  	struct objio_dev_ent *de = kzalloc(sizeof(*de), gfp_flags);
>  	struct objio_dev_ent *n;
>  
> -	if (!de)
> -		return -ENOMEM;
> +	if (!de) {
> +		dprintk("%s: -ENOMEM od=%p\n", __func__, od);
> +		return NULL;

better return ERR_PTR(-ENOMEM)
that will percolate up the stack via _device_lookup.

Thanks!

Benny

> +	}
>  
> +	dprintk("%s: Adding od=%p\n", __func__, od);
>  	nfs4_init_deviceid_node(&de->id_node,
>  				nfss->pnfs_curr_ld,
>  				nfss->nfs_client,
> @@ -91,11 +99,12 @@ static int _dev_list_add(const struct nfs_server *nfss,
>  	d = nfs4_insert_deviceid_node(&de->id_node);
>  	n = container_of(d, struct objio_dev_ent, id_node);
>  	if (n != de) {
> -		BUG_ON(n->od != od);
> +		dprintk("%s: Race with other n->od=%p\n", __func__, n->od);
>  		objio_free_deviceid_node(&de->id_node);
> +		de = n;
>  	}
>  
> -	return 0;
> +	return de;
>  }
>  
>  struct caps_buffers {
> @@ -117,7 +126,7 @@ struct objio_segment {
>  	unsigned comps_index;
>  	unsigned num_comps;
>  	/* variable length */
> -	struct objio_dev_ent *ods[0];
> +	struct objio_dev_ent *ods[];
>  };
>  
>  static inline struct objio_segment *
> @@ -176,12 +185,13 @@ static struct objio_dev_ent *_device_lookup(struct pnfs_layout_hdr *pnfslay,
>  		goto out;
>  	}
>  
> -	_dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od, gfp_flags);
> +	ode = _dev_list_add(NFS_SERVER(pnfslay->plh_inode), d_id, od,
> +			    gfp_flags);
>  
>  out:
>  	dprintk("%s: return=%d\n", __func__, err);
>  	objlayout_put_deviceinfo(deviceaddr);
> -	return err ? ERR_PTR(err) : od;
> +	return err ? ERR_PTR(err) : ode;
>  }
>  
>  static int objio_devices_lookup(struct pnfs_layout_hdr *pnfslay,