Re: [PATCH] NFS support for btrfs - v2

[David Woodhouse <dwmw2@infradead.org>]

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 */

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation