Thanks, I know some people have been waiting for this for quite a while.
Please make sure Jan Kara who's the new udf maintainer and linux-fsdevel
where we discuss general filesystem related issue for future revisions

isofs might not be the very best example since it's an odd filesystem,
but then so is udf.  I'll go through your patch in a little more detail

Any reason this is not called udf_get_parent to follow the scheme

This if/else block looks little odd, and following the locking is a bit
hard.  What about doing it like the following:

	lock_kernel();
	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))
		goto out_unlock;

	if (fibh.sbh != fibh.ebh)
		brelse(fibh.ebh);
	brelse(fibh.sbh);

	inode = udf_iget(child->d_inode->i_sb,
			 lelb_to_cpu(cfi.icb.extLocation));
	if (!inode)
		goto out_unlock;
	unlock_kernel();

	parent = d_alloc_anon(inode);
	if (!parent) {
		iput(inode);
		parent = ERR_PTR(-ENOMEM);
	}

	return parent;
 out_unlock:
	unlock_kernel();
	return ERR_PTR(-EACCESS);

to follow other filesystems this should be called
udf_nfs_get_inode.  Also please decide if you want to put the static
and return type on the same line or on a separate one.  Having it on
the same one is documented in Documentation/Codingstyle but separate

it would be better to introduce a version of udf_iget that can check
the generation and return an error instead of having to check this
later.  If you don't think you're up to modifying code we could do
this later on, though.  In this case please note this in the patch
description and fix up the above formatting to read something like:

	if (is_bad_inode(inode) ||

It would be useful if you could add symbolic constants for the
two fh types you add and chose values not yet used by other filesystems,
e.g. 0x51 and 0x52.  This will help people sniffing the nfs on the
wire protocol to understand what file handle they're dealing with.

Also you migh want to make the fh_len check more explicit and...
[ message continues ]

Most of the code is copied from udf_lookup(..)
To me the locking is simple. Before the two returns you have an unlock.
It's pretty clear that all control-paths are covered. However changing
Ok.

Attached is a new attempt at a patch.

Signed-off-by: Rasmus Rohde <rohde@duff.dk>

--- fs/udf/namei.c.orig	2007-10-10 16:22:30.000000000 +0200
+++ fs/udf/namei.c	2008-02-05 18:28:13.000000000 +0100
@@ -31,6 +31,7 @@
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/sched.h>
+#include <linux/exportfs.h>
 
 static inline int udf_match(int len1, const char *name1, int len2,
 			    const char *name2)
@@ -315,9 +316,8 @@ static struct dentry *udf_lookup(struct 
 		}
 	}
 	unlock_kernel();
-	d_add(dentry, inode);
 
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,
@@ -1231,6 +1231,135 @@ end_rename:
 	return retval;
 }
 
+static struct dentry *udf_get_parent(struct dentry *child)
+{
+	struct dentry *parent;
+	struct inode *inode = NULL;
+	struct dentry dotdot;
+	struct fileIdentDesc cfi;
+	struct udf_fileident_bh fibh;
+
+	dotdot.d_name.name = "..";
+	dotdot.d_name.len = 2;
+
+	lock_kernel();
+	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))
+		goto out_unlock;
+	
+	if (fibh.sbh != fibh.ebh)
+		brelse(fibh.ebh);
+	brelse(fibh.sbh);
+		
+	inode = udf_iget(child->d_inode->i_sb,
+			 lelb_to_cpu(cfi.icb.extLocation));
+	if (!inode)
+		goto out_unlock;
+	unlock_kernel();
+
+	parent = d_alloc_anon(inode);
+	if (!parent) {
+		iput(inode);
+		parent = ERR_PTR(-ENOMEM);
+	}
+
+	return parent;
+out_unlock:
+	unlock_kernel();
+	return ERR_PTR(-EACCES);
+}
+
+
+static struct dentry *udf_nfs_get_inode(struct super_block *sb, u32 block,
+					u16 partref, __u32 generation)
+{
+	struct inode *inode;
+	struct dentry *result;
+	kernel_lb_addr loc;
+
+	if (block == 0)
+		return ERR_PTR(-ESTALE);
+
+...
[ message continues ]

Have you ever tried this? I think this could never work. UDF doesn't have
entry named .. in a directory. You have to search for an entry that has
in fileCharacteristics set bit FID_FILE_CHAR_PARENT. Maybe you could
hack-around udf_find_entry() to recognize .. dentry and do the search
  Otherwise the patch looks fine. But please rediff the patch against
Andrew's development tree (or -mm) because there are some cleanups there...
Thanks.

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
-

Probably not. I just tested that I could read files and navigate the
directory structure. However looking into UDF I think you are right - it
will fail.
I have extended udf_find_entry() to do an explicit check based on
fileCharacteristics as you propose.
Certainly there are. New patch against 2.6.24-mm1:

Signed-off-by: Rasmus Rohde <rohde@duff.dk>

diff -uprN -X linux-2.6.24-mm1-vanilla/Documentation/dontdiff linux-2.6.24-mm1-vanilla/fs/udf/namei.c linux-2.6.24-mm1/fs/udf/namei.c
--- linux-2.6.24-mm1-vanilla/fs/udf/namei.c	2008-02-06 21:23:36.000000000 +0100
+++ linux-2.6.24-mm1/fs/udf/namei.c	2008-02-06 21:41:38.000000000 +0100
@@ -31,6 +31,7 @@
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/sched.h>
+#include <linux/exportfs.h>
 
 static inline int udf_match(int len1, const char *name1, int len2,
 			    const char *name2)
@@ -159,6 +160,8 @@ static struct fileIdentDesc *udf_find_en
 	sector_t offset;
 	struct extent_position epos = {};
 	struct udf_inode_info *dinfo = UDF_I(dir);
+	int isdotdot = dentry->d_name.len == 2 &&
+		dentry->d_name.name[0] == '.' && dentry->d_name.name[1] == '.';
 
 	size = udf_ext0_offset(dir) + dir->i_size;
 	f_pos = udf_ext0_offset(dir);
@@ -232,6 +235,12 @@ static struct fileIdentDesc *udf_find_en
 				continue;
 		}
 
+		if ((cfi->fileCharacteristics & FID_FILE_CHAR_PARENT) &&
+		    isdotdot) {
+			brelse(epos.bh);
+			return fi;
+		}
+
 		if (!lfi)
 			continue;
 
@@ -324,9 +333,8 @@ static struct dentry *udf_lookup(struct 
 		}
 	}
 	unlock_kernel();
-	d_add(dentry, inode);
 
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,
@@ -1298,6 +1306,134 @@ end_rename:
 	return retval;
 }
 
+static struct dentry *udf_get_parent(struct dentry *child)
+{
+	struct dentry *parent;
+	struct inode *inode = NULL;
+	struct dentry dotdot;
+	struct fileIdentDe...
[ message continues ]

There's still a few trivial warnings from scripts/checkpatch.pl that
should be fixed up:

ERROR: trailing whitespace
#88: FILE: fs/udf/namei.c:1323:
+^I$

ERROR: trailing whitespace
#92: FILE: fs/udf/namei.c:1327:
+^I^I$

ERROR: trailing whitespace
#185: FILE: fs/udf/namei.c:1420:
+^I^Ifid->udf.parent_partref = location.partitionReferenceNum;^I$

WARNING: externs should be avoided in .c files
#212: FILE: fs/udf/super.c:79:
+extern struct export_operations udf_export_ops;

total: 3 errors, 1 warnings, 218 lines checked

-

Ok - I have checked get_parent and it works as expected.
I used the "Neil Brown"-test mentioned elsewhere in this thread and
Fixed that. Sorry for not running checkpatch.pl before submitting.

Before posting the last and hopefully final patch I'd like to know what
Jan says about open coding the lookup for ..
It will mean a lot of code duplication and I think it makes good sense
for udf_find_entry to be able to handle ..

-

Yes, I think opencoding it would really lead to larger code duplication
than I'd like so please keep the change in udf_find_entry(). Thanks.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
-

Great - then I think we are hopefully at a patch that can be accepted.

Signed-off-by: Rasmus Rohde <rohde@duff.dk>

diff -uprN -X linux-2.6.24-mm1-vanilla/Documentation/dontdiff linux-2.6.24-mm1-vanilla/fs/udf/namei.c linux-2.6.24-mm1/fs/udf/namei.c
--- linux-2.6.24-mm1-vanilla/fs/udf/namei.c	2008-02-06 21:23:36.000000000 +0100
+++ linux-2.6.24-mm1/fs/udf/namei.c	2008-02-07 07:13:04.000000000 +0100
@@ -31,6 +31,7 @@
 #include <linux/smp_lock.h>
 #include <linux/buffer_head.h>
 #include <linux/sched.h>
+#include <linux/exportfs.h>
 
 static inline int udf_match(int len1, const char *name1, int len2,
 			    const char *name2)
@@ -159,6 +160,8 @@ static struct fileIdentDesc *udf_find_en
 	sector_t offset;
 	struct extent_position epos = {};
 	struct udf_inode_info *dinfo = UDF_I(dir);
+	int isdotdot = dentry->d_name.len == 2 &&
+		dentry->d_name.name[0] == '.' && dentry->d_name.name[1] == '.';
 
 	size = udf_ext0_offset(dir) + dir->i_size;
 	f_pos = udf_ext0_offset(dir);
@@ -232,6 +235,12 @@ static struct fileIdentDesc *udf_find_en
 				continue;
 		}
 
+		if ((cfi->fileCharacteristics & FID_FILE_CHAR_PARENT) &&
+		    isdotdot) {
+			brelse(epos.bh);
+			return fi;
+		}
+
 		if (!lfi)
 			continue;
 
@@ -324,9 +333,8 @@ static struct dentry *udf_lookup(struct 
 		}
 	}
 	unlock_kernel();
-	d_add(dentry, inode);
 
-	return NULL;
+	return d_splice_alias(inode, dentry);
 }
 
 static struct fileIdentDesc *udf_add_entry(struct inode *dir,
@@ -1298,6 +1306,134 @@ end_rename:
 	return retval;
 }
 
+static struct dentry *udf_get_parent(struct dentry *child)
+{
+	struct dentry *parent;
+	struct inode *inode = NULL;
+	struct dentry dotdot;
+	struct fileIdentDesc cfi;
+	struct udf_fileident_bh fibh;
+
+	dotdot.d_name.name = "..";
+	dotdot.d_name.len = 2;
+
+	lock_kernel();
+	if (!udf_find_entry(child->d_inode, &dotdot, &fibh, &cfi))
+		goto out_unlock;
+
+	if (fibh.sbh !=...
[ message continues ]

Jan, any reason this patch didn't go in with the last merge?  I'd really
---end quoted text---
--

Thanks for catching this. It seems I missed the patch when merging all
the UDF patches I had in my mailbox. I have it merged in my git tree and
will push it to Linus with other patches.

-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
--

Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR
-

Testing this is pretty hard.  You export a filesystem, then cd somewhere
deep into a directory hiearchy in there.  Then unexport the filesystem
and unmount on the server.  mount it back on the server, export it again
and do something with a file from the directory you've cd into before
the unmount.  Make sure you have a printk in your get_parent method
to make sure you're really hitting it.

Btw, I think it would be nicer to opencode the .. lookup in get_parent
instead of changing udf_find_entry.  The lookup for .. is not needed
by anything else, and get_parent only looks for it so it's a natural
place to opencode it there.

-

^^^^^^^

Argh - this should have been WITH so the line reads:

if (fh_len != 5 || fh_type != FILEID_UDF_WITH_PARENT)

I'll just hold back a little reposting a new patch if other comments
should happen to show up.

-