enhanced ESTALE error handling

PATCH In review Peter Staubach 6 participants 11 messages Jan 18, 2008 – Mar 11, 2008

Subsystems (3)

VFS NFS client NFS server

lore

Share & tools

lore

Peter Staubach

author

2008-01-18 15:36 UTC #0 lore raw

PATCH 1/3 enhanced ESTALE error handling

Hi.

This is a patch to enhance ESTALE error handling during the lookup process. The error, ESTALE, can occur when out of data dentries, stored in the dcache, is used to translate a pathname component to a dentry. When this occurs, the dentry which contains the pointer to the inode which refers to the non-existent file is dropped from the dcache and then the lookup process started again. Care is taken to ensure that forward process is always being made. If forward process is not detected, then the lookup process is terminated and the error, ENOENT, is returned to the caller.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/namei.c renamed from fs/namei.c.org +40 ~~-10~~

@@ -741,7 +741,7 @@ static __always_inline void follow_dotdo
 {
 	struct fs_struct *fs = current->fs;
 
-	while(1) {
+	while (1) {
 		struct vfsmount *parent;
 		struct dentry *old = nd->dentry;

@@ -840,7 +840,7 @@ static fastcall int __link_path_walk(con
 		lookup_flags = LOOKUP_FOLLOW | (nd->flags & LOOKUP_CONTINUE);
 
 	/* At this point we know we have a real path component. */
-	for(;;) {
+	for (;;) {
 		unsigned long hash;
 		struct qstr this;
 		unsigned int c;

@@ -992,7 +992,7 @@ return_reval:
 		 */
 		if (nd->dentry && nd->dentry->d_sb &&
 		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
-			err = -ESTALE;
+			err = -ENOENT;
 			/* Note: we do not d_invalidate() */
 			if (!nd->dentry->d_op->d_revalidate(nd->dentry, nd))
 				break;

@@ -1003,6 +1003,8 @@ out_dput:
 		dput_path(&next, nd);
 		break;
 	}
+	if (err == -ESTALE)
+		d_drop(nd->dentry);
 	path_release(nd);
 return_err:
 	return err;

@@ -1025,12 +1027,27 @@ static int fastcall link_path_walk(const
 	mntget(save.mnt);
 
 	result = __link_path_walk(name, nd);
-	if (result == -ESTALE) {
+	while (result == -ESTALE) {
+		/*
+		 * If no progress was made looking up the pathname,
+		 * then stop and return ENOENT instead of ESTALE.
+		 */
+		if (nd->dentry == save.dentry) {
+			result = -ENOENT;
+			break;
+		}
 		*nd = save;
 		dget(nd->dentry);
 		mntget(nd->mnt);
 		nd->flags |= LOOKUP_REVAL;
 		result = __link_path_walk(name, nd);
+		/*
+		 * If no progress was made this time, then return
+		 * ENOENT instead of ESTALE because no recovery
+		 * is possible to recover the stale file handle.
+		 */
+		if (result == -ESTALE && nd->dentry == save.dentry)
+			result = -ENOENT;
 	}
 
 	dput(save.dentry);

@@ -1268,8 +1285,8 @@ int path_lookup_open(int dfd, const char
  * @create_mode: create intent flags
  */
 static int path_lookup_create(int dfd, const char *name,
-			      unsigned int lookup_flags, struct nameidata *nd,
-			      int open_flags, int create_mode)
+		unsigned int lookup_flags, struct nameidata *nd,
+		int open_flags, int create_mode)
 {
 	return __path_lookup_intent_open(dfd, name, lookup_flags|LOOKUP_CREATE,
 			nd, open_flags, create_mode);

@@ -1712,7 +1729,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
 
 	acc_mode = ACC_MODE(flag);

@@ -1739,7 +1759,8 @@ int open_namei(int dfd, const char *path
 	/*
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;

@@ -1812,10 +1833,17 @@ ok:
 	return 0;
 
 exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}
 	path_release(nd);
 	return error;

@@ -1825,7 +1853,7 @@ do_link:
 		goto exit_dput;
 	/*
 	 * This is subtle. Instead of calling do_follow_link() we do the
-	 * thing by hands. The reason is that this way we have zero link_count
+	 * thing by hand. The reason is that this way we have zero link_count
 	 * and path_walk() (called from ->follow_link) honoring LOOKUP_PARENT.
 	 * After that we have the parent and last component, i.e.
 	 * we are in the same situation as after the first path_walk().

@@ -1844,6 +1872,8 @@ do_link:
 		 * with "intent.open".
 		 */
 		release_open_intent(nd);
+		if (error == ESTALE)
+			goto top;
 		return error;
 	}
 	nd->flags &= ~LOOKUP_PARENT;

@@ -1857,7 +1887,7 @@ do_link:
 		goto exit;
 	}
 	error = -ELOOP;
-	if (count++==32) {
+	if (count++ == 32) {
 		__putname(nd->last.name);
 		goto exit;
 	}

Matthew Wilcox

2008-01-18 16:14 UTC #1 lore raw

Quoted 19 lines · Peter Staubach

fs/namei.c renamed from fs/namei.c.org +9 -1

@@ -1025,12 +1027,27 @@ static int fastcall link_path_walk(const
 	mntget(save.mnt);
 
 	result = __link_path_walk(name, nd);
-	if (result == -ESTALE) {
+	while (result == -ESTALE) {
+		/*
+		 * If no progress was made looking up the pathname,
+		 * then stop and return ENOENT instead of ESTALE.
+		 */
+		if (nd->dentry == save.dentry) {
+			result = -ENOENT;
+			break;
+		}
 		*nd = save;
 		dget(nd->dentry);
 		mntget(nd->mnt);
 		nd->flags |= LOOKUP_REVAL;
 		result = __link_path_walk(name, nd);

fs/namei.c static int fastcall link_path_walk(const · line 1053

Quoted 10 lines

fs/namei.c renamed from fs/namei.c.org +7 -0

+		/*
+		 * If no progress was made this time, then return
+		 * ENOENT instead of ESTALE because no recovery
+		 * is possible to recover the stale file handle.
+		 */
+		if (result == -ESTALE && nd->dentry == save.dentry)
+			result = -ENOENT;
 	}
 
 	dput(save.dentry);

Why do you need both of these tests? The first one should be enough, surely?

fs/namei.c int path_lookup_open(int dfd, const char · line 1289

Quoted 8 lines

fs/namei.c renamed from fs/namei.c.org +2 -2

@@ -1268,8 +1285,8 @@ int path_lookup_open(int dfd, const char
  * @create_mode: create intent flags
  */
 static int path_lookup_create(int dfd, const char *name,
-			      unsigned int lookup_flags, struct nameidata *nd,
-			      int open_flags, int create_mode)
+		unsigned int lookup_flags, struct nameidata *nd,
+		int open_flags, int create_mode)

Gratuitous reformatting?

Quoted 27 lines

fs/namei.c renamed from fs/namei.c.org +7 -2

@@ -1712,7 +1729,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
 
 	acc_mode = ACC_MODE(flag);

@@ -1739,7 +1759,8 @@ int open_namei(int dfd, const char *path
 	/*
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;

@@ -1812,10 +1833,17 @@ ok:
 	return 0;
 
 exit_dput:
+	if (error == -ESTALE)

fs/namei.c ok: · line 1846

Quoted 10 lines

fs/namei.c renamed from fs/namei.c.org +6 -0

+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}

I wonder if a tail-call might not work better here.

Signature · 4 lines

Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Peter Staubach

author

2008-01-18 16:45 UTC #2 lore raw

Quoted 30 lines · Matthew Wilcox

Quoted 28 lines · Peter Staubach

fs/namei.c renamed from fs/namei.c.org +16 -1

@@ -1025,12 +1027,27 @@ static int fastcall link_path_walk(const
 	mntget(save.mnt);
 
 	result = __link_path_walk(name, nd);
-	if (result == -ESTALE) {
+	while (result == -ESTALE) {
+		/*
+		 * If no progress was made looking up the pathname,
+		 * then stop and return ENOENT instead of ESTALE.
+		 */
+		if (nd->dentry == save.dentry) {
+			result = -ENOENT;
+			break;
+		}
 		*nd = save;
 		dget(nd->dentry);
 		mntget(nd->mnt);
 		nd->flags |= LOOKUP_REVAL;
 		result = __link_path_walk(name, nd);
+		/*
+		 * If no progress was made this time, then return
+		 * ENOENT instead of ESTALE because no recovery
+		 * is possible to recover the stale file handle.
+		 */
+		if (result == -ESTALE && nd->dentry == save.dentry)
+			result = -ENOENT;
 	}

Quoted 7 lines

fs/namei.c static int fastcall link_path_walk(const · line 1053
Quoted 2 lines
fs/namei.c renamed from fs/namei.c.org +0 -0
 	dput(save.dentry);
Why do you need both of these tests? The first one should be enough, surely?

Yes, good point.

Quoted 13 lines

fs/namei.c int path_lookup_open(int dfd, const char · line 1290

Quoted 9 lines

fs/namei.c renamed from fs/namei.c.org +2 -2

@@ -1268,8 +1285,8 @@ int path_lookup_open(int dfd, const char
  * @create_mode: create intent flags
  */
 static int path_lookup_create(int dfd, const char *name,
-			      unsigned int lookup_flags, struct nameidata *nd,
-			      int open_flags, int create_mode)
+		unsigned int lookup_flags, struct nameidata *nd,
+		int open_flags, int create_mode)

Gratuitous reformatting?

Elimination of an overly long line?

Quoted 25 lines

fs/namei.c renamed from fs/namei.c.org +6 -2

@@ -1712,7 +1729,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
 
 	acc_mode = ACC_MODE(flag);

@@ -1739,7 +1759,8 @@ int open_namei(int dfd, const char *path
 	/*
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;

@@ -1812,10 +1833,17 @@ ok:
 	return 0;

Quoted 15 lines

fs/namei.c ok: · line 1849

Quoted 13 lines

fs/namei.c renamed from fs/namei.c.org +7 -0

 exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}

I wonder if a tail-call might not work better here.

"Tail-call"?

Thanx...

ps
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

J. Bruce Fields

2008-01-18 17:18 UTC #3 lore raw

Quoted 13 lines · Peter Staubach

Quoted 11 lines · Matthew Wilcox
fs/namei.c int path_lookup_open(int dfd, const char · line 1289
Quoted 6 lines · Peter Staubach
static int path_lookup_create(int dfd, const char *name,
fs/namei.c renamed from fs/namei.c.org +2 -2
-			      unsigned int lookup_flags, struct nameidata *nd,
-			      int open_flags, int create_mode)
+		unsigned int lookup_flags, struct nameidata *nd,
+		int open_flags, int create_mode)
Gratuitous reformatting?

Quoted 1 line

Elimination of an overly long line?

I usually try to gather any coding style, comment grammar, etc., fixes into a single patch or two at the beginning of a series. That keeps the substantive patches (the hardest to understand) shorter.

--b.

Quoted 47 lines

Quoted 29 lines

fs/namei.c renamed from fs/namei.c.org +4 -1

@@ -1712,7 +1729,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
  	acc_mode = ACC_MODE(flag);
 @@ -1739,7 +1759,8 @@ int open_namei(int dfd, const char *path
 	/*

(quoted diff) +4 -1

 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;
 @@ -1812,10 +1833,17 @@ ok:
 	return 0;
  exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);

Quoted 8 lines

fs/namei.c ok: · line 1846
Quoted 6 lines
fs/namei.c renamed from fs/namei.c.org +5 -0
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}
I wonder if a tail-call might not work better here.

"Tail-call"?

   Thanx...

      ps

-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Staubach

author

2008-01-18 17:32 UTC #4 lore raw

Quoted 17 lines · J. Bruce Fields

Quoted 15 lines · Peter Staubach
Quoted 13 lines · Matthew Wilcox
fs/namei.c int path_lookup_open(int dfd, const char · line 1289
Quoted 7 lines · Peter Staubach
static int path_lookup_create(int dfd, const char *name,
fs/namei.c renamed from fs/namei.c.org +2 -2
-			      unsigned int lookup_flags, struct nameidata *nd,
-			      int open_flags, int create_mode)
+		unsigned int lookup_flags, struct nameidata *nd,
+		int open_flags, int create_mode)
Gratuitous reformatting?

Quoted 8 lines

Quoted 2 lines
Elimination of an overly long line?
I usually try to gather any coding style, comment grammar, etc., fixes into a single patch or two at the beginning of a series. That keeps the substantive patches (the hardest to understand) shorter.

That's probably great advice. I can easily enough undo the change since it does not affect the functionality of the patch. It was made while I was doing the analysis for the patch and to make the style better match the style used in other surrounding routines.

Thanx...

Quoted 50 lines

--b.
Quoted 47 lines
Quoted 28 lines
Quoted 28 lines
fs/namei.c renamed from fs/namei.c.org +4 -1
@@ -1712,7 +1729,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
  	acc_mode = ACC_MODE(flag);
 @@ -1739,7 +1759,8 @@ int open_namei(int dfd, const char *path
 	/*
(quoted diff) +4 -1
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;
 @@ -1812,10 +1833,17 @@ ok:
 	return 0;
  exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
Quoted 9 lines
fs/namei.c ok: · line 1846
Quoted 7 lines
fs/namei.c renamed from fs/namei.c.org +5 -0
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}
I wonder if a tail-call might not work better here.
"Tail-call"?
   Thanx...

      ps
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Peter Staubach

author

2008-02-01 20:57 UTC #5 lore raw

PATCH 1/3 enhanced lookup ESTALE error handling (v2)

In reply to Peter Staubach

Hi.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/namei.c renamed from fs/namei.c.org +34 -8

@@ -741,7 +741,7 @@ static __always_inline void follow_dotdo
 {
 	struct fs_struct *fs = current->fs;
 
-	while(1) {
+	while (1) {
 		struct vfsmount *parent;
 		struct dentry *old = nd->dentry;

@@ -840,7 +840,7 @@ static fastcall int __link_path_walk(con
 		lookup_flags = LOOKUP_FOLLOW | (nd->flags & LOOKUP_CONTINUE);
 
 	/* At this point we know we have a real path component. */
-	for(;;) {
+	for (;;) {
 		unsigned long hash;
 		struct qstr this;
 		unsigned int c;

@@ -992,7 +992,7 @@ return_reval:
 		 */
 		if (nd->dentry && nd->dentry->d_sb &&
 		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
-			err = -ESTALE;
+			err = -ENOENT;
 			/* Note: we do not d_invalidate() */
 			if (!nd->dentry->d_op->d_revalidate(nd->dentry, nd))
 				break;

@@ -1003,6 +1003,8 @@ out_dput:
 		dput_path(&next, nd);
 		break;
 	}
+	if (err == -ESTALE)
+		d_drop(nd->dentry);
 	path_release(nd);
 return_err:
 	return err;

@@ -1019,13 +1021,24 @@ static int fastcall link_path_walk(const
 {
 	struct nameidata save = *nd;
 	int result;
+	struct dentry *svd;
 
 	/* make sure the stuff we saved doesn't go away */
 	dget(save.dentry);
 	mntget(save.mnt);
 
+	svd = nd->dentry;
 	result = __link_path_walk(name, nd);
-	if (result == -ESTALE) {
+	while (result == -ESTALE) {
+		/*
+		 * If no progress was made looking up the pathname,
+		 * then stop and return ENOENT instead of ESTALE.
+		 */
+		if (nd->dentry == svd) {
+			result = -ENOENT;
+			break;
+		}
+		svd = nd->dentry;
 		*nd = save;
 		dget(nd->dentry);
 		mntget(nd->mnt);

@@ -1712,7 +1725,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
 
 	acc_mode = ACC_MODE(flag);

@@ -1739,7 +1755,8 @@ int open_namei(int dfd, const char *path
 	/*
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;

@@ -1812,10 +1829,17 @@ ok:
 	return 0;
 
 exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
+	if (error == -ESTALE) {
+		d_drop(nd->dentry);
+		path_release(nd);
+		goto top;
+	}
 	path_release(nd);
 	return error;

@@ -1825,7 +1849,7 @@ do_link:
 		goto exit_dput;
 	/*
 	 * This is subtle. Instead of calling do_follow_link() we do the
-	 * thing by hands. The reason is that this way we have zero link_count
+	 * thing by hand. The reason is that this way we have zero link_count
 	 * and path_walk() (called from ->follow_link) honoring LOOKUP_PARENT.
 	 * After that we have the parent and last component, i.e.
 	 * we are in the same situation as after the first path_walk().

@@ -1844,6 +1868,8 @@ do_link:
 		 * with "intent.open".
 		 */
 		release_open_intent(nd);
+		if (error == ESTALE)
+			goto top;
 		return error;
 	}
 	nd->flags &= ~LOOKUP_PARENT;

@@ -1857,7 +1883,7 @@ do_link:
 		goto exit;
 	}
 	error = -ELOOP;
-	if (count++==32) {
+	if (count++ == 32) {
 		__putname(nd->last.name);
 		goto exit;
 	}

Peter Staubach

author

2008-03-10 20:23 UTC #6 lore raw

PATCH 1/3 enhanced lookup ESTALE error handling (v3)

Hi.

Thanx...

Signed-off-by Peter Staubach <[email protected]>

fs/namei.c renamed from fs/namei.c.org +34 -8

@@ -750,7 +750,7 @@ static __always_inline void follow_dotdo
 {
 	struct fs_struct *fs = current->fs;
 
-	while(1) {
+	while (1) {
 		struct vfsmount *parent;
 		struct dentry *old = nd->path.dentry;

@@ -849,7 +849,7 @@ static int __link_path_walk(const char *
 		lookup_flags = LOOKUP_FOLLOW | (nd->flags & LOOKUP_CONTINUE);
 
 	/* At this point we know we have a real path component. */
-	for(;;) {
+	for (;;) {
 		unsigned long hash;
 		struct qstr this;
 		unsigned int c;

@@ -1003,7 +1003,7 @@ return_reval:
 		 */
 		if (nd->path.dentry && nd->path.dentry->d_sb &&
 		    (nd->path.dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
-			err = -ESTALE;
+			err = -ENOENT;
 			/* Note: we do not d_invalidate() */
 			if (!nd->path.dentry->d_op->d_revalidate(
 					nd->path.dentry, nd))

@@ -1015,6 +1015,8 @@ out_dput:
 		path_put_conditional(&next, nd);
 		break;
 	}
+	if (err == -ESTALE)
+		d_drop(nd->path.dentry);
 	path_put(&nd->path);
 return_err:
 	return err;

@@ -1031,13 +1033,24 @@ static int link_path_walk(const char *na
 {
 	struct nameidata save = *nd;
 	int result;
+	struct dentry *svd;
 
 	/* make sure the stuff we saved doesn't go away */
 	dget(save.path.dentry);
 	mntget(save.path.mnt);
 
+	svd = nd->path.dentry;
 	result = __link_path_walk(name, nd);
-	if (result == -ESTALE) {
+	while (result == -ESTALE) {
+		/*
+		 * If no progress was made looking up the pathname,
+		 * then stop and return ENOENT instead of ESTALE.
+		 */
+		if (nd->path.dentry == svd) {
+			result = -ENOENT;
+			break;
+		}
+		svd = nd->path.dentry;
 		*nd = save;
 		dget(nd->path.dentry);
 		mntget(nd->path.mnt);

@@ -1714,7 +1727,10 @@ int open_namei(int dfd, const char *path
 	int acc_mode, error;
 	struct path path;
 	struct dentry *dir;
-	int count = 0;
+	int count;
+
+top:
+	count = 0;
 
 	acc_mode = ACC_MODE(flag);

@@ -1741,7 +1757,8 @@ int open_namei(int dfd, const char *path
 	/*
 	 * Create - we need to know the parent.
 	 */
-	error = path_lookup_create(dfd,pathname,LOOKUP_PARENT,nd,flag,mode);
+	error = path_lookup_create(dfd, pathname, LOOKUP_PARENT, nd,
+				flag, mode);
 	if (error)
 		return error;

@@ -1814,10 +1831,17 @@ ok:
 	return 0;
 
 exit_dput:
+	if (error == -ESTALE)
+		d_drop(path.dentry);
 	path_put_conditional(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
 		release_open_intent(nd);
+	if (error == -ESTALE) {
+		d_drop(nd->path.dentry);
+		path_put(&nd->path);
+		goto top;
+	}
 	path_put(&nd->path);
 	return error;

@@ -1827,7 +1851,7 @@ do_link:
 		goto exit_dput;
 	/*
 	 * This is subtle. Instead of calling do_follow_link() we do the
-	 * thing by hands. The reason is that this way we have zero link_count
+	 * thing by hand. The reason is that this way we have zero link_count
 	 * and path_walk() (called from ->follow_link) honoring LOOKUP_PARENT.
 	 * After that we have the parent and last component, i.e.
 	 * we are in the same situation as after the first path_walk().

@@ -1846,6 +1870,8 @@ do_link:
 		 * with "intent.open".
 		 */
 		release_open_intent(nd);
+		if (error == ESTALE)
+			goto top;
 		return error;
 	}
 	nd->flags &= ~LOOKUP_PARENT;

@@ -1859,7 +1885,7 @@ do_link:
 		goto exit;
 	}
 	error = -ELOOP;
-	if (count++==32) {
+	if (count++ == 32) {
 		__putname(nd->last.name);
 		goto exit;
 	}

Andrew Morton

2008-03-10 21:38 UTC #7 lore raw

Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)

Quoted 10 lines · On Mon, 10 Mar 2008 16:23:33 -0400 Peter Staubach…

This is a patch to enhance ESTALE error handling during the lookup process. The error, ESTALE, can occur when out of data dentries, stored in the dcache, is used to translate a pathname component to a dentry. When this occurs, the dentry which contains the pointer to the inode which refers to the non-existent file is dropped from the dcache and then the lookup process started again. Care is taken to ensure that forward process is always being made. If forward process is not detected, then the lookup process is terminated and the error, ENOENT, is returned to the caller.

This collides in non-trivial ways with the always-coming-never-arrives r-o-bind-mounts patches.

I have an old version of those patches in -mm and I believe that Al is/was/has set up some git tree with these patches and perhaps other stuff.

So some coordination is required please. I'd suggest that if Al indeed does have such a tree, he hand over the URL so I can get it into -mm and that you then redo these patches on top of that.

Please also Cc Al and Christoph on these patches, as they are hitting files which they maintain and develop.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Miklos Szeredi

2008-03-10 22:03 UTC #8 lore raw

Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)

Quoted 17 lines

Quoted 10 lines
This is a patch to enhance ESTALE error handling during the lookup process. The error, ESTALE, can occur when out of data dentries, stored in the dcache, is used to translate a pathname component to a dentry. When this occurs, the dentry which contains the pointer to the inode which refers to the non-existent file is dropped from the dcache and then the lookup process started again. Care is taken to ensure that forward process is always being made. If forward process is not detected, then the lookup process is terminated and the error, ENOENT, is returned to the caller.
This collides in non-trivial ways with the always-coming-never-arrives r-o-bind-mounts patches.
I have an old version of those patches in -mm and I believe that Al is/was/has set up some git tree with these patches and perhaps other stuff.

Quoted 6 lines

So some coordination is required please. I'd suggest that if Al indeed does have such a tree, he hand over the URL so I can get it into -mm and that you then redo these patches on top of that.
Please also Cc Al and Christoph on these patches, as they are hitting files which they maintain and develop.

Also my objection about breaking backward compatibility for fuse filesystems is still seemingly unaddressed.

I know it would be more convenient to push this problem into the fuse filesystems, but they are unfortunately on the other side of the stable kernel ABI, so this is not an option.

I've already presented a solution (which wasn't greeted with big enthusiasm), and I'm open to discussion about other ways to solve this.

Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dave Hansen

2008-03-10 22:19 UTC #9 lore raw

Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)

Quoted 11 lines · Miklos Szeredi

Quoted 11 lines
Quoted 10 lines
This is a patch to enhance ESTALE error handling during the lookup process. The error, ESTALE, can occur when out of data dentries, stored in the dcache, is used to translate a pathname component to a dentry. When this occurs, the dentry which contains the pointer to the inode which refers to the non-existent file is dropped from the dcache and then the lookup process started again. Care is taken to ensure that forward process is always being made. If forward process is not detected, then the lookup process is terminated and the error, ENOENT, is returned to the caller.

Quoted 5 lines

Quoted 5 lines
This collides in non-trivial ways with the always-coming-never-arrives r-o-bind-mounts patches.
I have an old version of those patches in -mm and I believe that Al is/was/has set up some git tree with these patches and perhaps other stuff.

Al's tree is here:

http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/viro/vfs-2.6.git;a=summary

It's probably best to try and base your patches on top of there.

-- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Andrew Morton

2008-03-11 04:12 UTC #10 lore raw

Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)

Quoted 15 lines · On Mon, 10 Mar 2008 15:19:44 -0700 Dave Hansen…

Quoted 14 lines · Miklos Szeredi
Quoted 14 lines
Quoted 10 lines
This is a patch to enhance ESTALE error handling during the lookup process. The error, ESTALE, can occur when out of data dentries, stored in the dcache, is used to translate a pathname component to a dentry. When this occurs, the dentry which contains the pointer to the inode which refers to the non-existent file is dropped from the dcache and then the lookup process started again. Care is taken to ensure that forward process is always being made. If forward process is not detected, then the lookup process is terminated and the error, ENOENT, is returned to the caller.
This collides in non-trivial ways with the always-coming-never-arrives r-o-bind-mounts patches.

Quoted 9 lines

Quoted 2 lines
Quoted 2 lines
I have an old version of those patches in -mm and I believe that Al is/was/has set up some git tree with these patches and perhaps other stuff.
Al's tree is here:
http://git.kernel.org/gitweb.cgi?p=linux/kernel/git/viro/vfs-2.6.git;a=summary
It's probably best to try and base your patches on top of there.

Al has a unique versioning scheme which I am not smart enough to understand, so I'd be reluctant to pick this up without coordinating with him.

afacit the "b1" branch there is the latest, but it doesn't appear to have been touched for a couple of weeks.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html