[pnfs] [PATCH 3/3] pnfs-gfs2: initial GETDEVICE* work for pNFS/GFS2 integration

[Benny Halevy]

On May. 29, 2008, 2:52 +0300, "David M. Richter" <richterd at citi.umich.edu> wrote:
> Initial work on the get_device_iter() and get_device_info() export operations.
> Tested at Connectathon.
> 
> Signed-off-by: David M. Richter <richterd at citi.umich.edu>
> Signed-off-by: Frank Filz <ffilzlnx at us.ibm.com>
> ---
>  fs/gfs2/ops_export.c |  113 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 113 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/gfs2/ops_export.c b/fs/gfs2/ops_export.c
> index 6a50629..5cb1bba 100644
> --- a/fs/gfs2/ops_export.c
> +++ b/fs/gfs2/ops_export.c
> @@ -420,6 +420,117 @@ static int gfs2_layout_return(struct inode *inode, void *p)
>  
>  	return 0;
>  }
> +
> +static int gfs2_get_device_iter(struct super_block *sb,
> +				struct pnfs_deviter_arg *arg)
> +{
> +	if (arg->type != LAYOUT_NFSV4_FILES) {
> +		printk("%s: ERROR: layout type isn't 'file' (type: %x)\n",
> +			__func__, arg->type);
> +		return 1;

you should return a negative error here.
-ENOTSUPP seems to be the most appropriate.

> +	}
> +
> +	if (arg->cookie == 0) {
> +		arg->cookie = 1;
> +		arg->verf = 1;
> +		arg->devid = 1;
> +	} else
> +		arg->eof = 1;
> +
> +	return 0;
> +}
> +
> +static int gfs2_get_device_info(struct super_block *sb,
> +				struct pnfs_devinfo_arg *arg)
> +{
> +	int err, len, i = 0;
> +	struct pnfs_filelayout_device fdev;
> +	struct pnfs_filelayout_devaddr *daddr;
> +	char ds_buf[XXX_PNFS_DS_LISTSZ], *bufp = ds_buf, *bufp2 = ds_buf;

XXX_PNFS_DS_LISTSZ is 256 bytes long and that's too much to put
on the stack.  Please kmalloc(strlen(pnfs_ds_list)+1) below
and then memcpy(ds_buf, pnfs_ds_list, strlen(pnfs_ds_list)+1)
ala strdup. (pnfs_ds_list is properly terminated by proc_do_string)

> +
> +	if (arg->type != LAYOUT_NFSV4_FILES) {
> +		printk("%s: ERROR: layout type isn't 'file' (type: %x)\n",
> +			__func__, arg->type);
> +		return 1;

ditto

> +	}
> +
> +	if (arg->devid.pnfs_devid != 1) {
> +		printk("%s: WARNING: didn't receive a deviceid of 1 "
> +			"(got: 0x%llx)\n", __func__, arg->devid.pnfs_devid);
> +		return 1;

-EINVAL maybe?

> +	}
> +
> +	/* XXX: no notifications yet */
> +	arg->notify_types = 0;
> +
> +	printk("%s: DEBUG: current entire DS list is |%s|\n", __func__,
> +	       pnfs_ds_list);
> +	if (!*pnfs_ds_list) {
> +		printk("%s: ERROR: pnfs_ds_list has no entries!\n", __func__);
> +		return 1;

-EIO maybe?

> +	}
> +	memset(ds_buf, '\0', XXX_PNFS_DS_LISTSZ);
> +	memcpy(ds_buf, pnfs_ds_list, XXX_PNFS_DS_LISTSZ);

Why memset then memcpy?

(though I agree that dining tables better be wiped before
you cover them with a cloth ;-)

Ah, and see comment above regarding dynamically allocating
ds_buf...

> +
> +	/* count the number of comma-delimited DS IPs */
> +	for (fdev.fl_device_length = 1; (bufp = strchr(bufp, ',')); bufp++,
> +			fdev.fl_device_length++);

Please don't do 'for (whatever);'
I'm not sure what Documentation/CodingStyle has to say about this
but it's extremely dangerous and error prone in the long term
when somebody else may work on this code and fails to see
that embedded empty statement.

The for loop "side effects", that aren't used for controlling
the iteration, should be apparent and be placed in the loop's
body, pretty much like this:

	for (fdev.fl_device_length = 1; (bufp = strchr(bufp, ',')); bufp++)
		fdev.fl_device_length++;

another alternative would be to use a while loop:

	fdev.fl_device_length = 1;
	while ((bufp = strchr(bufp, ',')) != NULL) {
		fdev.fl_device_length++;
		bufp++;
	}


> +
> +	len = sizeof(*fdev.fl_device_list) * fdev.fl_device_length;
> +	fdev.fl_device_list = kzalloc(len, GFP_KERNEL);
> +	if (!fdev.fl_device_list) {
> +		printk("%s: ERROR: unable to kmalloc a device list buffer for "
> +			"%d DSes.\n", __func__, i);
> +		return 1;

-ENOMEM

> +	}
> +
> +	fdev.fl_stripeindices_length = fdev.fl_device_length;
> +	fdev.fl_stripeindices_list = kzalloc(sizeof(u32) * fdev.fl_stripeindices_length,
> +						GFP_KERNEL);
> +	if (!fdev.fl_stripeindices_list) {
> +		printk("%s: ERROR: unable to kmalloc a stripeindices list "
> +			"buffer for %d DSes.\n", __func__, i);
> +		goto out_err;
> +	}
> +	for (i = 0; i < fdev.fl_stripeindices_length; i++)
> +		fdev.fl_stripeindices_list[i] = i;
> +
> +	for (i = 0; (bufp = strsep(&bufp2, ",")) != NULL; i++) {

i++, bufp = bufp2 maybe?
I don't see any other place where you skip bufp forward
to the next device address.

> +		printk("%s: DEBUG: encoding DS |%s|\n", __func__, bufp);
> +
> +		daddr = kmalloc(sizeof(*daddr), GFP_KERNEL);
> +		if (!daddr) {
> +			printk("%s: ERROR: unable to kmalloc a device addr "
> +				"buffer.\n", __func__);
> +			goto out_err;
> +		}
> +
> +		daddr->r_netid.data = "tcp";

I wonder if there's a dynamic way of determining that
from the ip address.

> +		daddr->r_netid.len = 3;
> +		len = strlen(bufp);
> +		daddr->r_addr.data = kzalloc(len + 4, GFP_KERNEL);
> +		memcpy(daddr->r_addr.data, bufp, len);
> +		memcpy(daddr->r_addr.data + len, ".8.1", 4);

Please provide a comment for the ".8.1" magic.

> +		daddr->r_addr.len = len + 4;
> +
> +		fdev.fl_device_list[i].fl_multipath_length = 1;
> +		fdev.fl_device_list[i].fl_multipath_list = daddr;
> +	}
> +
> +	/* have nfsd encode the device info */
> +	err = arg->func(&arg->xdr, &fdev);
> +
> +out_free:
> +	for (i = 0; i < fdev.fl_device_length; i++)
> +		kfree(fdev.fl_device_list[i].fl_multipath_list);
> +	kfree(fdev.fl_device_list);
> +	kfree(fdev.fl_stripeindices_list);
> +	return err;
> +out_err:
> +	err = 1;

just set err in the goto'ers scope, or -ENOMEM
if it's common to all error cases that goto here

> +	goto out_free;
> +}
> +
>  #endif /* CONFIG_PNFSD */
>  
>  const struct export_operations gfs2_export_ops = {
> @@ -433,6 +544,8 @@ const struct export_operations gfs2_export_ops = {
>  	.layout_get = gfs2_layout_get,
>  	.layout_commit = gfs2_layout_commit,
>  	.layout_return = gfs2_layout_return,
> +	.get_device_iter = gfs2_get_device_iter,
> +	.get_device_info = gfs2_get_device_info,
>  #endif /* CONFIG_PNFSD */
>  };
>