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@xxxxxxxxxxxxxxx/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@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
