Re: [PATCH] NFS support for btrfs - v2

[Balaji Rao <balajirrao@gmail.com>]

On Sunday 17 August 2008 06:26:03 pm David Woodhouse wrote:
> On Sun, 2008-08-17 at 18:21 +0530, Balaji Rao wrote:
> > On Sunday 17 August 2008 05:23:22 pm David Woodhouse wrote:
> > > On Mon, 2008-07-21 at 02:01 +0530, Balaji Rao wrote:
> > > > Here's an implementation of NFS support for btrfs. It does not work
> > > > in one particular case as described in
> > > > http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg00298.html
> > > >.
> > >
> > > I can't get it to work properly. The simple case of exporting and using
> > > a file system works -- but that doesn't exercise the get_parent() or
> > > dentry_to_fh() code paths. To test those, you need to clear the
> > > server's dcache completely -- I prefer just rebooting the server.
> >
> > OK. I was not very sure if NFS worked across server reboots.
>
> It definitely should.
>
> > > Note that the first problem you'll hit is the lack of stable fsid --
> > > because btrfs uses an anonymous superblock, it might cause stale file
> > > handles after a reboot, unless your test setup mounts exactly the same
> > > anonymous file systems each time. That bit me when I context switched
> > > from something else and had debugfs mounted before btrfs, then rebooted
> > > and didn't mount debugfs first. I'll deal with the fsid problem
> > > separately; just be aware of it and avoid it for now.
> >
> > Hmmm.. how do we deal with that ?
>
> I'm not entirely sure yet. I had a patch to make the kernel
> automatically set a uuid on the export, which was simple enough.
>
> Unfortunately that isn't sufficient, because although we then return an
> appropriate root fh to the mount request, we then aren't capable of
> _interpreting_ that fh when the client immediately gives it back to us
> in a subsequent fsinfo request. Because we're relying on mountd to do
> the initial fh->dentry conversion for us, and mountd is very limited in
> its uuid handling -- it assumes that
>  - there is a block device
>  - there is a 1:1 mapping between block device and uuid
>  - libuuid can cope with new file systems.
>
> > > The second problem is that btrfs_fh_to_dentry() fails -- it seems we
> > > need to set the key type BTRFS_INODE_ITEM_KEY in btrfs_get_dentry(),
> > > rather than BTRFS_INODE_REF_KEY. I still haven't learned enough to know
> > > precisely what the implications of that are -- but unless I make that
> > > change, btrfs_lookup_inode() fails and btrfs_read_locked_inode() marks
> > > the resulting inode as bad.
> >
> > Right. Again, my bad. That's a silly mistake, which didn't surface
> > because of bad testing!
> >
> > > You can test that by mounting the exported file system, then rebooting
> > > the server, then running 'ls' in the NFS-mounted file system.
> > >
> > > My next test was to make a deep directory path: mkdir -p a/b/c/d/e/f/g
> > > and then change directory into it. Again reboot the server and run
> > > 'ls'. This time, I see 'ls: cannot open directory .'. In this csse, it
> > > seems to be because btrfs_get_parent() is failing. In my case, it seems
> > > to be because the 'if (slot >= nritems)' check is triggering -- both
> > > are set to 14. I'm now trying to work out precisely what that means...
> >
> > I think this means that the inode ref is not found, which is weird. I
> > remember that it worked with it a really deep path. I'll try to reproduce
> > the problem and see what's going on..
>
> See below... it seems to be working now.
>
> diff --git a/export.c b/export.c
> index 1b8875c..6209f35 100644
> --- a/export.c
> +++ b/export.c
> @@ -72,7 +72,7 @@ static struct dentry *btrfs_get_dentry(struct super_block
> *sb, u64 objectid, struct btrfs_key key;
>
>  	key.objectid = objectid;
> -	btrfs_set_key_type(&key, BTRFS_INODE_REF_KEY);
> +	btrfs_set_key_type(&key, BTRFS_INODE_ITEM_KEY);
>  	key.offset = 0;
>
>  	root = btrfs_lookup_fs_root(btrfs_sb(sb)->fs_info, root_objectid);
> @@ -164,14 +164,22 @@ static struct dentry *btrfs_get_parent(struct dentry
> *child) leaf = path->nodes[0];
>  	slot = path->slots[0];
>  	nritems = btrfs_header_nritems(leaf);
> -	if (slot >= nritems)
> -		goto out;
> +	if (slot >= nritems) {
> +		ret = btrfs_next_leaf(root, path);
> +		if (ret) {
> +			btrfs_free_path(path);
> +			goto out;
> +		}
> +		leaf = path->nodes[0];
> +		slot = path->slots[0];
> +	}
> +
> +	btrfs_free_path(path);
>
>  	btrfs_item_key_to_cpu(leaf, &key, slot);
>  	if (key.objectid != dir->i_ino || key.type != BTRFS_INODE_REF_KEY)
>  		goto out;
>
> -	btrfs_free_path(path);
>  	objectid = key.offset;
>
>  	/* Build a new key for the inode item */

OK. I had copied over this code snippet from inode.c:btrfs_inode_by_name, 
which has the condition 'if (slot >= nritems)' removed now by this.

changeset:   631:87490dc3bb59
user:        "Yan Zheng" <yanzheng@21cn.com>
date:        Thu Jul 24 12:19:32 2008 -0400
summary:     Fix .. lookup corner case

I think we should refactor btrfs_inode_by_name into something we can use ?

-- 
Thanks,
Balaji