Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org>]

Andreas Dilger <adilger@clusterfs.com> wrote:

> +     if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')){

The patch was supposed to affect only '..'.

Maybe you can add a 'unlikely', too.
-- 
A "sucking chest wound" is nature's way of telling you to slow down. 

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Stephen C. Tweedie]

Hi,

On Mon, 2005-05-09 at 15:24, Bodo Eggert  wrote:
> Andreas Dilger <adilger@clusterfs.com> wrote:
> 
> > +     if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')){
> 
> The patch was supposed to affect only '..'.

"." is an equivalent special case in htree.  It seems reasonable to
include it here for completeness.

--Stephen

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Bodo Eggert]

On Mon, 9 May 2005, Stephen C. Tweedie wrote:
> On Mon, 2005-05-09 at 15:24, Bodo Eggert  wrote:
> > Andreas Dilger <adilger@clusterfs.com> wrote:
> > 
> > > +     if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')){
> > 
> > The patch was supposed to affect only '..'.
> 
> "." is an equivalent special case in htree.  It seems reasonable to
> include it here for completeness.

OK, but the comment should be adjusted.
-- 
Backups? We doan NEED no steenking baX%^~,VbKx NO CARRIER 

[PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Henrik Grubbström]

The 2.6.10 ext3_get_parent attempts to use ext3_find_entry to look up the
entry "..", which fails for dx directories since ".." is not present in
the directory hash table. The patch below solves this by looking up the
dotdot entry in the dx_root block.

Typical symptoms of the above bug are intermittent claims by nfsd that
files or directories are missing on exported ext3 filesystems.

cf https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=150759
and https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=144556

Signed-off-by: Henrik Grubbström <grubba@grubba.org>

--- linux/fs/ext3/namei.c.orig	2005-05-06 17:08:31.000000000 +0200
+++ linux/fs/ext3/namei.c	2005-05-07 17:57:24.000000000 +0200
@@ -999,24 +999,90 @@ static struct dentry *ext3_lookup(struct
 
 struct dentry *ext3_get_parent(struct dentry *child)
 {
-	unsigned long ino;
+	unsigned long ino = 0;
 	struct dentry *parent;
 	struct inode *inode;
-	struct dentry dotdot;
-	struct ext3_dir_entry_2 * de;
 	struct buffer_head *bh;
+	int err;
+	struct inode *dir = child->d_inode;
+	struct super_block *sb = dir->i_sb;
 
-	dotdot.d_name.name = "..";
-	dotdot.d_name.len = 2;
-	dotdot.d_parent = child; /* confusing, isn't it! */
+	/* In all cases the ".." entry is in block #0, so read it. */
+	bh = ext3_bread(NULL, dir, 0, 0, &err);
+	if (!bh) return ERR_PTR(err);
+
+#ifdef CONFIG_EXT3_INDEX
+	if (is_dx(dir)) {
+		/* Block #0 is a dx_root. */
+		struct dx_root *root = (struct dx_root *)bh->b_data;
+		if (root->info.hash_version == DX_HASH_TEA ||
+		    root->info.hash_version == DX_HASH_HALF_MD4 ||
+		    root->info.hash_version == DX_HASH_LEGACY) {
+			/* Is this paranoia actually needed? */
+			ino = le32_to_cpu(root->dotdot.inode);
+		} else {
+			ext3_error(sb, __FUNCTION__,
+				"unsupported dx hash_version (%d) for #%lu\n",
+				root->info.hash_version, dir->i_ino);
+		}
+	}
+	if (!ino)
+#endif
+	{
+		/* Block #0 is a sequence of ext3_dir_entry_2.
+		 * ".." is the second entry.
+		 */
+		struct ext3_dir_entry_2 *de =
+			(struct ext3_dir_entry_2 *)bh->b_data;
+		int len = le16_to_cpu(de->rec_len);
+		if (len > 0) {
+			/* The first entry should be ".". */
+			de = (struct ext3_dir_entry_2 *)((char *)de + len);
+			if ((de->name_len == 2) &&
+			    (de->name[0] == '.') &&
+			    (de->name[1] == '.')) {
+				/* The second entry should be "..". */
+				ino = le32_to_cpu(de->inode);
+			} else {
+				ext3_warning(sb, __FUNCTION__,
+					"second entry of #%lu not parent",
+					dir->i_ino);
+			}
+		} else {
+			ext3_error(sb, __FUNCTION__,
+				"first entry of #%lu has bad rec_len: %d",
+				dir->i_ino, len);
+		}
+#if 0
+		/* Fallback code to enable if the ".." entry ever can
+		 * be somewhere else than in the second entry of the
+		 * first block.
+		 */
+		if (!ino) {
+			struct dentry dotdot;
+
+			ext3_warning(sb, __FUNCTION__,
+				"falling back to ext3_find_entry for #%lu\n",
+				dir->i_ino);
+			brelse(bh);
+			dotdot.d_name.name = "..";
+			dotdot.d_name.len = 2;
+			dotdot.d_parent = child; /* confusing, isn't it! */
+
+			bh = ext3_find_entry(&dotdot, &de);
+			if (!bh)
+				return ERR_PTR(-ENOENT);
+			ino = le32_to_cpu(de->inode);
+		}
+#endif /* 0 */
+	}
 
-	bh = ext3_find_entry(&dotdot, &de);
-	inode = NULL;
-	if (!bh)
-		return ERR_PTR(-ENOENT);
-	ino = le32_to_cpu(de->inode);
 	brelse(bh);
-	inode = iget(child->d_inode->i_sb, ino);
+
+	if (!ino)
+		return ERR_PTR(-ENOENT);
+
+	inode = iget(sb, ino);
 
 	if (!inode)
 		return ERR_PTR(-EACCES);

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]

On May 09, 2005  10:57 +0200, Henrik Grubbström wrote:
> The 2.6.10 ext3_get_parent attempts to use ext3_find_entry to look up the
> entry "..", which fails for dx directories since ".." is not present in
> the directory hash table. The patch below solves this by looking up the
> dotdot entry in the dx_root block.
> 
> Typical symptoms of the above bug are intermittent claims by nfsd that
> files or directories are missing on exported ext3 filesystems.
> 
> cf https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=150759
> and https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=144556
> 
> Signed-off-by: Henrik Grubbström <grubba@grubba.org>

ext3_get_parent() is IMHO the wrong place to fix this bug as it introduces
a lot of internals from htree into that function.  Instead, I think this
should be fixed in ext3_find_entry() as in the below patch.  This has the
added advantage that it works for any callers of ext3_find_entry() and not
just ext3_lookup_parent().

I thought I submitted this patch to l-k at one point, but now I can't find
any mention of that in the archives...

Signed-off-by: Andreas Diler <adilger@clusterfs.com>

--- linux-2.6.orig/fs/ext3/namei.c	2005-04-04 05:06:46.000000000 -0600
+++ linux-2.6/fs/ext3/namei.c	2005-04-04 05:09:18.000000000 -0600
@@ -926,8 +926,16 @@
 	struct inode *dir = dentry->d_parent->d_inode;
 
 	sb = dir->i_sb;
-	if (!(frame = dx_probe(dentry, NULL, &hinfo, frames, err)))
-		return NULL;
+	/* NFS may look up ".." - look at dx_root directory block */
+	if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')){
+		if (!(frame = dx_probe(dentry, NULL, &hinfo, frames, err)))
+			return NULL;
+	} else {
+		frame = frames;
+		frame->bh = NULL;			/* for dx_release() */
+		frame->at = (struct dx_entry *)frames;	/* hack for zero entry*/
+		dx_set_block(frame->at, 0);		/* dx_root block is 0 */
+	}
 	hash = hinfo.hash;
 	do {
 		block = dx_get_block(frame->at);

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Henrik Grubbström]

On Mon, 9 May 2005, Andreas Dilger wrote:

> On May 09, 2005  10:57 +0200, Henrik Grubbström wrote:
> > The 2.6.10 ext3_get_parent attempts to use ext3_find_entry to look up the
> > entry "..", which fails for dx directories since ".." is not present in
> > the directory hash table. The patch below solves this by looking up the
> > dotdot entry in the dx_root block.
> 
> ext3_get_parent() is IMHO the wrong place to fix this bug as it introduces
> a lot of internals from htree into that function.  Instead, I think this
> should be fixed in ext3_find_entry() as in the below patch.  This has the
> added advantage that it works for any callers of ext3_find_entry() and not
> just ext3_lookup_parent().

The reason I didn't put it there is that handling of ".." is usually
performed by fs/namei.c:link_path_walk() and putting it in
ext3_find_entry() or one of the functions it calls would slow down the
common case.

> Cheers, Andreas

--
Henrik Grubbström					grubba@roxen.com
Roxen Internet Software AB

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Stephen C. Tweedie]

Hi,

On Mon, 2005-05-09 at 10:46, Henrik Grubbström wrote:

> > ext3_get_parent() is IMHO the wrong place to fix this bug as it introduces
> > a lot of internals from htree into that function.  Instead, I think this
> > should be fixed in ext3_find_entry() as in the below patch.  This has the
> > added advantage that it works for any callers of ext3_find_entry() and not
> > just ext3_lookup_parent().
> 
> The reason I didn't put it there is that handling of ".." is usually
> performed by fs/namei.c:link_path_walk() and putting it in
> ext3_find_entry() or one of the functions it calls would slow down the
> common case.

True, but the extra cost is to evaluate

if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')

as false.  That's not going to take long most of the time.  And this
solution has two other big advantages: it fixes things for all lookups,
not just NFS, and hence is safer against this bug cropping up again in
the future in unexpected places; and it includes less dependency on
htree internals, as Andreas said.

I'll have a closer review of the patch, but for now I think I like
Andreas's version better.

Cheers,
 Stephen

Re: [PATCH] Support for dx directories in ext3_get_parent (NFSD)

[Henrik Grubbström]

On Mon, 9 May 2005, Stephen C. Tweedie wrote:

> On Mon, 2005-05-09 at 10:46, Henrik Grubbström wrote:
> 
> > > ext3_get_parent() is IMHO the wrong place to fix this bug as it introduces
> > > a lot of internals from htree into that function.  Instead, I think this
> > > should be fixed in ext3_find_entry() as in the below patch.  This has the
> > > added advantage that it works for any callers of ext3_find_entry() and not
> > > just ext3_lookup_parent().
> > 
> > The reason I didn't put it there is that handling of ".." is usually
> > performed by fs/namei.c:link_path_walk() and putting it in
> > ext3_find_entry() or one of the functions it calls would slow down the
> > common case.
> 
> True, but the extra cost is to evaluate
> 
> if (namelen > 2 || name[0] != '.'||(name[1] != '.' && name[1] != '\0')
> 
> as false.  That's not going to take long most of the time.  And this
> solution has two other big advantages: it fixes things for all lookups,
> not just NFS, and hence is safer against this bug cropping up again in
> the future in unexpected places; and it includes less dependency on
> htree internals, as Andreas said.

All true.

Since the htree stuff is implemented in the same file and the get_parent
operation is rather basic I didn't see much point. The advantages of my
patch are that it looks at a single directory block and that it doesn't
affect the i_dir_start_lookup state kept for non dx-directories.

> Cheers,
>  Stephen

--
Henrik Grubbström					grubba@roxen.com
Roxen Internet Software AB