LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] enhanced ESTALE error handling
@ 2008-01-18 15:36 Peter Staubach
  2008-01-18 16:14 ` Matthew Wilcox
  2008-02-01 20:57 ` [PATCH 1/3] enhanced lookup ESTALE error handling (v2) Peter Staubach
  0 siblings, 2 replies; 11+ messages in thread
From: Peter Staubach @ 2008-01-18 15:36 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: linux-nfs, Andrew Morton, Trond Myklebust, linux-fsdevel

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

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...

       ps

Signed-off-by: Peter Staubach <staubach@redhat.com>

[-- Attachment #2: estale.lookup --]
[-- Type: text/plain, Size: 3883 bytes --]

--- linux-2.6.23.i686/fs/namei.c.org
+++ linux-2.6.23.i686/fs/namei.c
@@ -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;
 	}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced ESTALE error handling
  2008-01-18 15:36 [PATCH 1/3] enhanced ESTALE error handling Peter Staubach
@ 2008-01-18 16:14 ` Matthew Wilcox
  2008-01-18 16:45   ` Peter Staubach
  2008-02-01 20:57 ` [PATCH 1/3] enhanced lookup ESTALE error handling (v2) Peter Staubach
  1 sibling, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2008-01-18 16:14 UTC (permalink / raw)
  To: Peter Staubach
  Cc: Linux Kernel Mailing List, linux-nfs, Andrew Morton,
	Trond Myklebust, linux-fsdevel

On Fri, Jan 18, 2008 at 10:36:01AM -0500, Peter Staubach wrote:
> @@ -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);

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

> @@ -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?

> @@ -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;
> +	}

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

-- 
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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced ESTALE error handling
  2008-01-18 16:14 ` Matthew Wilcox
@ 2008-01-18 16:45   ` Peter Staubach
  2008-01-18 17:18     ` J. Bruce Fields
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Staubach @ 2008-01-18 16:45 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Linux Kernel Mailing List, linux-nfs, Andrew Morton,
	Trond Myklebust, linux-fsdevel

Matthew Wilcox wrote:
> On Fri, Jan 18, 2008 at 10:36:01AM -0500, Peter Staubach wrote:
>   
>> @@ -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);
>>     
>
> Why do you need both of these tests?  The first one should be enough,
> surely?
>
>   

Yes, good point.

>> @@ -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?

>> @@ -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;
>> +	}
>>     
>
> I wonder if a tail-call might not work better here.

"Tail-call"?

    Thanx...

       ps

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced ESTALE error handling
  2008-01-18 16:45   ` Peter Staubach
@ 2008-01-18 17:18     ` J. Bruce Fields
  2008-01-18 17:32       ` Peter Staubach
  0 siblings, 1 reply; 11+ messages in thread
From: J. Bruce Fields @ 2008-01-18 17:18 UTC (permalink / raw)
  To: Peter Staubach
  Cc: Matthew Wilcox, Linux Kernel Mailing List, linux-nfs,
	Andrew Morton, Trond Myklebust, linux-fsdevel

On Fri, Jan 18, 2008 at 11:45:52AM -0500, Peter Staubach wrote:
> Matthew Wilcox wrote:
>> On Fri, Jan 18, 2008 at 10:36:01AM -0500, Peter Staubach wrote:
>>>  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?

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.

>
>>> @@ -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;
>>> +	}
>>>     
>>
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced ESTALE error handling
  2008-01-18 17:18     ` J. Bruce Fields
@ 2008-01-18 17:32       ` Peter Staubach
  0 siblings, 0 replies; 11+ messages in thread
From: Peter Staubach @ 2008-01-18 17:32 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Matthew Wilcox, Linux Kernel Mailing List, linux-nfs,
	Andrew Morton, Trond Myklebust, linux-fsdevel

J. Bruce Fields wrote:
> On Fri, Jan 18, 2008 at 11:45:52AM -0500, Peter Staubach wrote:
>   
>> Matthew Wilcox wrote:
>>     
>>> On Fri, Jan 18, 2008 at 10:36:01AM -0500, Peter Staubach wrote:
>>>       
>>>>  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?
>>     
>
> 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...

       ps

> --b.
>
>   
>>>> @@ -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;
>>>> +	}
>>>>     
>>>>         
>>> 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 majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>     


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] enhanced lookup ESTALE error handling (v2)
  2008-01-18 15:36 [PATCH 1/3] enhanced ESTALE error handling Peter Staubach
  2008-01-18 16:14 ` Matthew Wilcox
@ 2008-02-01 20:57 ` Peter Staubach
  2008-03-10 20:23   ` [PATCH 1/3] enhanced lookup ESTALE error handling (v3) Peter Staubach
  1 sibling, 1 reply; 11+ messages in thread
From: Peter Staubach @ 2008-02-01 20:57 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: linux-nfs, Andrew Morton, Trond Myklebust, linux-fsdevel

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

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...

      ps

Signed-off-by: Peter Staubach <staubach@redhat.com>

[-- Attachment #2: estale.lookup.2 --]
[-- Type: text/plain, Size: 3254 bytes --]

--- linux-2.6.24.i686/fs/namei.c.org
+++ linux-2.6.24.i686/fs/namei.c
@@ -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;
 	}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/3] enhanced lookup ESTALE error handling (v3)
  2008-02-01 20:57 ` [PATCH 1/3] enhanced lookup ESTALE error handling (v2) Peter Staubach
@ 2008-03-10 20:23   ` Peter Staubach
  2008-03-10 21:38     ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Peter Staubach @ 2008-03-10 20:23 UTC (permalink / raw)
  To: Linux Kernel Mailing List
  Cc: linux-nfs, Andrew Morton, Trond Myklebust, linux-fsdevel

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

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...

        ps

Signed-off-by: Peter Staubach <staubach@redhat.com>

[-- Attachment #2: estale.lookup.3 --]
[-- Type: text/plain, Size: 3357 bytes --]

--- linux-2.6.24.i686/fs/namei.c.org
+++ linux-2.6.24.i686/fs/namei.c
@@ -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;
 	}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)
  2008-03-10 20:23   ` [PATCH 1/3] enhanced lookup ESTALE error handling (v3) Peter Staubach
@ 2008-03-10 21:38     ` Andrew Morton
  2008-03-10 22:03       ` Miklos Szeredi
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2008-03-10 21:38 UTC (permalink / raw)
  To: Peter Staubach
  Cc: linux-kernel, linux-nfs, trond.myklebust, linux-fsdevel, Al Viro,
	Christoph Hellwig, Dave Hansen

On Mon, 10 Mar 2008 16:23:33 -0400
Peter Staubach <staubach@redhat.com> wrote:

> 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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)
  2008-03-10 21:38     ` Andrew Morton
@ 2008-03-10 22:03       ` Miklos Szeredi
  2008-03-10 22:19         ` Dave Hansen
  0 siblings, 1 reply; 11+ messages in thread
From: Miklos Szeredi @ 2008-03-10 22:03 UTC (permalink / raw)
  To: akpm
  Cc: staubach, linux-kernel, linux-nfs, trond.myklebust,
	linux-fsdevel, viro, hch, haveblue

> > 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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)
  2008-03-10 22:03       ` Miklos Szeredi
@ 2008-03-10 22:19         ` Dave Hansen
  2008-03-11  4:12           ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Hansen @ 2008-03-10 22:19 UTC (permalink / raw)
  To: Miklos Szeredi
  Cc: akpm, staubach, linux-kernel, linux-nfs, trond.myklebust,
	linux-fsdevel, viro, hch

On Mon, 2008-03-10 at 23:03 +0100, Miklos Szeredi wrote:
> > > 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.

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] enhanced lookup ESTALE error handling (v3)
  2008-03-10 22:19         ` Dave Hansen
@ 2008-03-11  4:12           ` Andrew Morton
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2008-03-11  4:12 UTC (permalink / raw)
  To: Dave Hansen
  Cc: Miklos Szeredi, staubach, linux-kernel, linux-nfs,
	trond.myklebust, linux-fsdevel, viro, hch

On Mon, 10 Mar 2008 15:19:44 -0700 Dave Hansen <haveblue@us.ibm.com> wrote:

> On Mon, 2008-03-10 at 23:03 +0100, Miklos Szeredi wrote:
> > > > 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.
> 
> 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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2008-03-11  4:14 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-01-18 15:36 [PATCH 1/3] enhanced ESTALE error handling Peter Staubach
2008-01-18 16:14 ` Matthew Wilcox
2008-01-18 16:45   ` Peter Staubach
2008-01-18 17:18     ` J. Bruce Fields
2008-01-18 17:32       ` Peter Staubach
2008-02-01 20:57 ` [PATCH 1/3] enhanced lookup ESTALE error handling (v2) Peter Staubach
2008-03-10 20:23   ` [PATCH 1/3] enhanced lookup ESTALE error handling (v3) Peter Staubach
2008-03-10 21:38     ` Andrew Morton
2008-03-10 22:03       ` Miklos Szeredi
2008-03-10 22:19         ` Dave Hansen
2008-03-11  4:12           ` Andrew Morton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).