Re: [RFC] NFS Support

[Josef Bacik <jbacik@redhat.com>]

On Sun, Jun 29, 2008 at 05:05:31AM +0530, Balaji Rao wrote:
> Hello,
> 
> Here's a quick implementation of NFS support for btrfs. It works mostly. But
> its still a bit buggy. I suspect it has to do something with inode locking.
> When I unmount and mount a couple of times, it stops working i.e, when I do an 
> 'ls', it keeps waiting indefinitely. This is seen when accessing the filesystem
> from a local mount point as well.
> 
> Can anybody please review and tell me what stupid mistake I'm committing ? 
>

You may want to send things in patch form.  I'd recommend using the hg quilt
plugin, its nice for adding patches easily.
 
> 
> #include <linux/fs.h>
> #include <linux/types.h>
> #include "ctree.h"
> #include "disk-io.h"
> #include "btrfs_inode.h"
> #include "print-tree.h"
> #include "export.h"
> 
> static int btrfs_encode_fh(struct dentry *dentry, u32 *fh_in, int *max_len,
> 			int connectable)
> {	struct inode *inode = dentry->d_inode;
> 	int len = *max_len;
> 	u64 objectid, root_objectid;
> 	u32 generation;
> 	__le32 *fh = (__force __le32 *) fh_in;
> 
> 	if ((len < 5 )|| (connectable && len < 8)) {
> 		return 255;
> 	}

Magic numbers are a no-no.

> 	
> 	objectid = BTRFS_I(inode)->location.objectid;
> 	root_objectid = BTRFS_I(inode)->root->objectid;
> 	generation = inode->i_generation;
> 
> 	len  = 5;
> 	fh[0] = cpu_to_le32((u32)(objectid >> 32));
> 	fh[1] = cpu_to_le32((u32)(objectid & 0xffffffff));
> 	fh[2] = cpu_to_le32((u32)(root_objectid >> 32));
> 	fh[3] = cpu_to_le32((u32)(root_objectid & 0xffffffff));
> 	fh[4] = cpu_to_le32(generation);
> 
> 	if (connectable && !S_ISDIR(inode->i_mode)) {
> 		struct inode *parent;
> 
> 		spin_lock(&dentry->d_lock);
> 
> 		parent = dentry->d_parent->d_inode;
> 		objectid = BTRFS_I(parent)->location.objectid;
> 		generation = parent->i_generation;
> 
> 		fh[5] = cpu_to_le32((u32)(objectid >> 32));
> 		fh[6] = cpu_to_le32((u32)(objectid & 0xffffffff));
> 		fh[7] = cpu_to_le32(generation);
> 
> 		spin_unlock(&dentry->d_lock);
> 
> 		len += 3;

there is no need to take the d_lock here.

> 	}
> 	
> 	*max_len = len;
> 	return len;
> 
> }
> 
> static struct dentry * btrfs_get_dentry(struct super_block *sb,
> 			u64 objectid, u64 root_objectid, u32 generation)
> {
> 	struct inode *inode;
> 	struct dentry *result;
> 
> 	inode = btrfs_ilookup(sb, objectid, root_objectid);
> 	if (IS_ERR(inode))
> 		return (void *)inode;
> 
> 	if(generation != inode->i_generation) {
> 		iput(inode);
> 		return ERR_PTR(-ESTALE);
> 	}
> 
> 	result = d_alloc_anon(inode);
> 	if(!result) {
> 		iput(inode);
> 		return ERR_PTR(-ENOMEM);
> 	}
> 
> 	return result;
> }
> 
> static struct dentry *btrfs_fh_to_parent(struct super_block *sb,
> 		struct fid *fid, int fh_len, int fh_type)
> {
> 	u64 objectid, root_objectid;
> 	u32 generation;
> 
> 	if (fh_len < 8 || fh_type != 8) {
> 		return NULL;
> 	}
> 
> 
> 	root_objectid = (u64)le32_to_cpu(fid->raw[2]) << 32;
> 	root_objectid |= (u64)le32_to_cpu(fid->raw[3]);
> 	objectid = (u64)le32_to_cpu(fid->raw[5]) << 32;
> 	objectid |= (u64)le32_to_cpu(fid->raw[6]);	
> 	generation = le32_to_cpu(fid->raw[7]);
> 
> 	return btrfs_get_dentry(sb, objectid, root_objectid, generation);
> }
> 
> static struct dentry *btrfs_fh_to_dentry(struct super_block *sb,
> 		struct fid *fid, int fh_len, int fh_type)
> {
> 	u64 objectid, root_objectid;
> 	u32 generation;
> 
> 	if (fh_len < 5 || fh_type > 5) {
> 		return NULL;
> 	}
> 
> 	objectid = (u64)le32_to_cpu(fid->raw[0]) << 32;
> 	objectid |= (u64)le32_to_cpu(fid->raw[1]);	
> 	root_objectid = (u64)le32_to_cpu(fid->raw[2]) << 32;
> 	root_objectid |= (u64)le32_to_cpu(fid->raw[3]);
> 	generation = le32_to_cpu(fid->raw[4]);
> 
> 	return btrfs_get_dentry(sb, objectid, root_objectid, generation);
> }
> 
> static struct dentry *btrfs_get_parent(struct dentry *child)
> {
> 	struct inode *dir = child->d_inode;
> 	struct inode *inode;
> 	struct dentry *parent;
> 	struct btrfs_root *root = BTRFS_I(dir)->root;
> 	struct btrfs_key key;
> 	struct btrfs_path *path;
> 	struct extent_buffer *leaf;	
> 	u32 nritems;
> 	int slot;
> 	u64 objectid;
> 	int ret;
> 
> 	path = btrfs_alloc_path();
> 
> 	key.objectid = dir->i_ino;
> 	btrfs_set_key_type(&key, BTRFS_INODE_REF_KEY);
> 	key.offset = 0;
> 	ret = btrfs_search_slot(NULL, root, &key, path, 0, 0);
> 	BUG_ON(ret == 0);
> 	ret = 0;
> 
> 	leaf = path->nodes[0];
> 	slot = path->slots[0];
> 	nritems = btrfs_header_nritems(leaf);
> 	if (slot >= nritems)
> 		return ERR_PTR(-EINVAL);
> 
> 	btrfs_item_key_to_cpu(leaf, &key, slot);
> 	if (key.objectid != dir->i_ino ||
> 	    key.type != BTRFS_INODE_REF_KEY) {
> 		return ERR_PTR(-EINVAL);
> 	}
>

in both of the above error cases you don't free the path, you need to do that.
 
> 	btrfs_free_path(path);
> 	objectid = key.offset;
> 	inode = btrfs_iget_locked(root->fs_info->sb, objectid, root);
> 	
> 	parent = d_alloc_anon(inode);
> 	if (!parent) {
> 		iput(inode);
> 		parent = ERR_PTR(-ENOMEM);
> 	}
> 	
> 	return parent;	
> }
> 
> const struct export_operations btrfs_export_ops = {
> 	.encode_fh	= btrfs_encode_fh,
> 	.fh_to_dentry	= btrfs_fh_to_dentry,
> 	.fh_to_parent	= btrfs_fh_to_parent,
> 	.get_parent	= btrfs_get_parent,
> };

I don't know much about NFS so I can't really comment on much of anything else
other than the obvious problems, but that should point you in a general
direction.

Josef