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

[Benny Halevy]

On May. 29, 2008, 2:52 +0300, "David M. Richter" <richterd at citi.umich.edu> wrote:
> Frank Filz's work on the layout_type() and layout_get() export operations,
> with stubs for layout_commit() and layout_return().  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 |  134 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 134 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/gfs2/ops_export.c b/fs/gfs2/ops_export.c
> index 334c7f8..6a50629 100644
> --- a/fs/gfs2/ops_export.c
> +++ b/fs/gfs2/ops_export.c
> @@ -27,6 +27,16 @@
>  #include "rgrp.h"
>  #include "util.h"
>  
> +#if defined(CONFIG_PNFSD)
> +#include <linux/nfs_fs.h>
> +#include <linux/sunrpc/svc.h>
> +#include <linux/nfsd/state.h>
> +#include <linux/nfsd/nfsfh.h>
> +#include <linux/nfsd/pnfsd.h>
> +#include <linux/nfsd/nfs4layoutxdr.h>
> +#include <linux/nfs4_pnfs.h>
> +#endif /* CONFIG_PNFSD */
> +
>  #define GFS2_SMALL_FH_SIZE 4
>  #define GFS2_LARGE_FH_SIZE 8
>  #define GFS2_OLD_FH_SIZE 10
> @@ -294,11 +304,135 @@ static struct dentry *gfs2_fh_to_parent(struct super_block *sb, struct fid *fid,
>  	}
>  }
>  
> +#if defined(CONFIG_PNFSD)
> +static int gfs2_layout_type(void)
> +{
> +	return LAYOUT_NFSV4_FILES;
> +}
> +
> +/* like GPFS does, use a multiple of the underlying fs' blocksize */
> +static int get_stripe_unit(int blocksize)
> +{
> +	if (blocksize >= NFSSVC_MAXBLKSIZE)
> +		return blocksize;
> +
> +	while (NFSSVC_MAXBLKSIZE > blocksize)
> +		blocksize <<= 1;

Why don't you just return the largest multiple
of blocksize that fits in NFSSVC_MAXBLKSIZE?

NFSSVC_MAXBLKSIZE - NFSSVC_MAXBLKSIZE % blocksize

(note that in the current world both ways should
return NFSSVC_MAXBLKSIZE which 1 MB)

> +
> +	return blocksize;
> +}
> +
> +/* increment the fsid type to indicate a DS' fh from a LAYOUTGET */
> +static int setfh(struct knfsd_fh *fh)
> +{
> +	if (fh->fh_size > 8) {
> +		fh->fh_fsid_type += FSID_MAX;
> +		return 0;
> +	} else {

don't need else { } after return.

> +		printk(KERN_WARNING "gfs2_setfh bad fh_size\n");
> +		return -ENOENT;
> +	}

I suggest handling the unlikely exception first:
or would a BUG_ON be more appropriate?
and why check the size, fh_version==1 actually
signifies the new fh structure that includes the fsid type...

	if (unlikely(fh->fh_size <= 8)) {
		printk(KERN_WARNING "gfs2_setfh bad fh_size\n");
		return -ENOENT;
	}

	fh->fh_fsid_type += FSID_MAX;
	return 0;

with a BUG_ON, this function becomes nice and compact...

static void setfh(struct knfsd_fh *fh)
{
	BUG_ON(fh->fh_version != 1);
	fh->fh_fsid_type += FSID_MAX;
}

> +}
> +
> +/*
> + * Retrieve and encode a file layout onto the xdr stream.
> + * @inode: inode for which to retrieve layout
> + * @arg->xdr: xdr stream for encoding
> + * @arg->func: a call into file system to encode the layout on xdr stream.
> + */
> +static int gfs2_layout_get(struct inode *inode, struct pnfs_layoutget_arg *arg)
> +{
> +	int i, left, fhlen;
> +	struct pnfs_filelayout_layout *layout = NULL;
> +	struct knfsd_fh *fh_list = NULL;
> +	int rc = 0;
> +	int num_dests = 1;					/*FSFTEMP*/
> +
> +	printk("%s: LAYOUT_GET\n", __func__);
> +
> +	/* Set layout indept response args */
> +	arg->seg.layout_type = LAYOUT_NFSV4_FILES;
> +	arg->seg.offset = 0;
> +	arg->seg.length = inode->i_sb->s_maxbytes; /* The maximum file size */
> +
> +	fhlen = sizeof(*fh_list) * num_dests;
> +	left = arg->xdr.maxcount - sizeof(*layout) - fhlen;

sizeof(*layout) is irrelevant since it doesn't represent
(in principle) the number of bytes you need on the wire,
e.g. it includes a pointer to struct knfsd_fh * which you
aren't going to encode.

Even if you fix it it's still fragile and will break as
soon as somebody changes struct pnfs_filelayout_layout.
A better strategy would be to check the buffer space
dynamically as you go.

Actually, filelayout_encode_layout already does exactly
this, so why repeat here?

> +	if (left < 0) {
> +		rc = -ETOOSMALL;
> +		goto error;
> +	}
> +
> +	layout = kmalloc(sizeof(*layout), GFP_KERNEL);

kzalloc would be safer
though putting and initializing it on the stack should be ok too.
(it's 56 bytes long, right?)

> +	if (layout == NULL) {
> +		rc = -ENOMEM;
> +		goto error;
> +	}
> +
> +	/* Set file layout response args */
> +	layout->lg_layout_type = LAYOUT_NFSV4_FILES;
> +	layout->lg_stripe_type = STRIPE_SPARSE;
> +	layout->lg_commit_through_mds = true;
> +	layout->lg_stripe_unit = get_stripe_unit(inode->i_sb->s_blocksize);
> +	layout->lg_fh_length = num_dests;
> +	layout->device_id.pnfs_fsid = arg->fsid;
> +	layout->device_id.pnfs_devid = 1;			/*FSFTEMP*/
> +	layout->lg_first_stripe_index = 0;			/*FSFTEMP*/
> +	layout->lg_pattern_offset = 0;
> +
> +	fh_list = kzalloc(fhlen, GFP_KERNEL);

actually here you don't need kzalloc as you're setting
the array's contents anyway.

> +	if (fh_list == NULL) {
> +		rc = -ENOMEM;
> +		goto error;
> +	}
> +
> +	memcpy(&fh_list[0], arg->fh, sizeof(*fh_list));
> +	rc = setfh(&fh_list[0]);

Shouldn't FSID_MAX be applied only to DS filehandles?
At least this seems to be what the code in
nfs4_preprocess_stateid_op expects.

This needs to be documented better and/or abstracted
via a few helper functions to mark, detect, and strip
filehandles appropriately.

E.g.:

static inline int pnfs_fh_is_ds(struct knfsd_fh *fh)
{
	return fh->fh_fsid_type >= FSID_MAX;
}

static inline void pnfs_fh_mark_ds(struct knfsd_fh *fh)
{
	BUG_ON(fh->fh_version != 1);
	BUG_ON(pnfs_fh_is_ds(fh));
	fh->fh_fsid_type += FSID_MAX;
}

static inline int pnfs_fh_fsid_type(struct knfsd_fh *fh)
{
	int fsid_type = fh->fh_fsid_type;

	if (fsid_type > FSID_MAX)
		fsid_type -= FSID_MAX;
	return fsid_type;
}

> +	if (rc != 0)
> +		goto error;
> +
> +	if (num_dests > 1)
> +		for (i = 1; i < num_dests; i++)
> +			memcpy(&fh_list[i], &fh_list[0], sizeof(*fh_list));
> +	layout->lg_fh_list = fh_list;
> +
> +	/* Call nfsd to encode layout */
> +	rc = arg->func(&arg->xdr, layout);
> +exit:
> +	kfree(layout);
> +	kfree(fh_list);
> +	return rc;
> +
> +error:
> +	arg->seg.length = 0;
> +	goto exit;
> +}
> +
> +static int gfs2_layout_commit(struct inode *inode, void *p)
> +{
> +	printk("%s: LAYOUT_COMMIT (unimplemented)\n", __func__);
> +
> +	return 0;
> +}
> +
> +static int gfs2_layout_return(struct inode *inode, void *p)
> +{
> +	printk("%s: LAYOUT_RETURN (unimplemented)\n", __func__);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PNFSD */
> +
>  const struct export_operations gfs2_export_ops = {
>  	.encode_fh = gfs2_encode_fh,
>  	.fh_to_dentry = gfs2_fh_to_dentry,
>  	.fh_to_parent = gfs2_fh_to_parent,
>  	.get_name = gfs2_get_name,
>  	.get_parent = gfs2_get_parent,
> +#if defined(CONFIG_PNFSD)
> +	.layout_type = gfs2_layout_type,
> +	.layout_get = gfs2_layout_get,
> +	.layout_commit = gfs2_layout_commit,
> +	.layout_return = gfs2_layout_return,
> +#endif /* CONFIG_PNFSD */
>  };
>