[PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
on attempts to use a filehandle of a deleted file (spotted with pynfs
test PUTFH3).  The ENOENT was coming from xfs_iget.

(It's tempting to wonder whether we should just map all xfs_iget errors
to ESTALE, but I don't believe so--xfs_iget can also return ENOMEM at
least, which we wouldn't want mapped to ESTALE.)

While we're at it, the other return of ENOENT in xfs_nfs_get_inode()
also looks wrong.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/xfs/linux-2.6/xfs_export.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_export.c b/fs/xfs/linux-2.6/xfs_export.c
index f4f878f..75e5d32 100644
--- a/fs/xfs/linux-2.6/xfs_export.c
+++ b/fs/xfs/linux-2.6/xfs_export.c
@@ -151,14 +151,14 @@ xfs_nfs_get_inode(
 		 * We don't use ESTALE directly down the chain to not
 		 * confuse applications using bulkstat that expect EINVAL.
 		 */
-		if (error == EINVAL)
+		if (error == EINVAL || error == ENOENT)
 			error = ESTALE;
 		return ERR_PTR(-error);
 	}
 
 	if (ip->i_d.di_gen != generation) {
 		IRELE(ip);
-		return ERR_PTR(-ENOENT);
+		return ERR_PTR(-ESTALE);
 	}
 
 	return VFS_I(ip);
-- 
1.7.4.1

Re: [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[Alex Elder]

On Thu, 2011-07-14 at 16:50 -0400, J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@redhat.com>
> 
> On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
> on attempts to use a filehandle of a deleted file (spotted with pynfs
> test PUTFH3).  The ENOENT was coming from xfs_iget.
> 
> (It's tempting to wonder whether we should just map all xfs_iget errors
> to ESTALE, but I don't believe so--xfs_iget can also return ENOMEM at
> least, which we wouldn't want mapped to ESTALE.)
> 
> While we're at it, the other return of ENOENT in xfs_nfs_get_inode()
> also looks wrong.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Looks reasonable to me.

I'll give others a chance to weigh in, but otherwise I'll
get this in for Linux 3.1.

Reviewed-by: Alex Elder <aelder@sgi.com>

Re: [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[Christoph Hellwig]

On Thu, Jul 14, 2011 at 04:50:36PM -0400, J. Bruce Fields wrote:
> From: J. Bruce Fields <bfields@redhat.com>
> 
> On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
> on attempts to use a filehandle of a deleted file (spotted with pynfs
> test PUTFH3).  The ENOENT was coming from xfs_iget.

With that you mean the ip->i_d.di_mode checks?  Given that we should
only be bale to get these from NFS or the handle ioctls I suspect just
turning them into ESTALE should be fine.

Re: [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[J. Bruce Fields]

On Thu, Jul 14, 2011 at 06:31:26PM -0400, Christoph Hellwig wrote:
> On Thu, Jul 14, 2011 at 04:50:36PM -0400, J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@redhat.com>
> > 
> > On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
> > on attempts to use a filehandle of a deleted file (spotted with pynfs
> > test PUTFH3).  The ENOENT was coming from xfs_iget.
> 
> With that you mean the ip->i_d.di_mode checks?  Given that we should
> only be bale to get these from NFS or the handle ioctls I suspect just
> turning them into ESTALE should be fine.
> 

Like the following?  That passes my test.  I wouldn't have thought of
doing it that way because I wouldn't know how to check that

	- the change will only affect nfsd and the handle ioctls, and
	- those are the only two places under xfs_iget that will
	  generate an ENOENT (which will never be the right error return
	  on failure to find a filehandle).

If those are true, great.

--b.

commit 94578e7d1c3e00ad29608c70fae314f85a465840
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Thu Jul 14 15:39:49 2011 -0400

    xfs: failure mapping nfs fh to inode should return ESTALE
    
    On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
    on attempts to use a filehandle of a deleted file (spotted with pynfs
    test PUTFH3).  The ENOENT was coming from xfs_iget.
    
    While we're at it, the other return of ENOENT in xfs_nfs_get_inode()
    also looks wrong.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/xfs/linux-2.6/xfs_export.c b/fs/xfs/linux-2.6/xfs_export.c
index f4f878f..9e94d57 100644
--- a/fs/xfs/linux-2.6/xfs_export.c
+++ b/fs/xfs/linux-2.6/xfs_export.c
@@ -158,7 +158,7 @@ xfs_nfs_get_inode(
 
 	if (ip->i_d.di_gen != generation) {
 		IRELE(ip);
-		return ERR_PTR(-ENOENT);
+		return ERR_PTR(-ESTALE);
 	}
 
 	return VFS_I(ip);
diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index 3631783..19752451 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -222,7 +222,7 @@ xfs_iget_cache_hit(
 	 * If lookup is racing with unlink return an error immediately.
 	 */
 	if (ip->i_d.di_mode == 0 && !(flags & XFS_IGET_CREATE)) {
-		error = ENOENT;
+		error = ESTALE;
 		goto out_error;
 	}
 
@@ -333,7 +333,7 @@ xfs_iget_cache_miss(
 	trace_xfs_iget_miss(ip);
 
 	if ((ip->i_d.di_mode == 0) && !(flags & XFS_IGET_CREATE)) {
-		error = ENOENT;
+		error = ESTALE;
 		goto out_destroy;
 	}
 

Re: [xfs-masters] [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[Dave Chinner]

On Thu, Jul 14, 2011 at 06:31:26PM -0400, Christoph Hellwig wrote:
> On Thu, Jul 14, 2011 at 04:50:36PM -0400, J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@redhat.com>
> > 
> > On xfs exports, nfsd is incorrectly returning ENOENT instead of ESTALE
> > on attempts to use a filehandle of a deleted file (spotted with pynfs
> > test PUTFH3).  The ENOENT was coming from xfs_iget.
> 
> With that you mean the ip->i_d.di_mode checks?  Given that we should
> only be bale to get these from NFS or the handle ioctls I suspect just
> turning them into ESTALE should be fine.

Bulkstat also checks for ENOENT returns from xfs_iget() as the
lookup it does is inherently racy. That is, if the inode is
allocated when it is read from the IBT, but then has been unlinked
before the actual xfs_iget() call, it expects to get back an ENOENT
to tell it the inode has been unlinked and shouldn't be included in
the bulkstat output. Otherwise, it returns the error to userspace.

So converting the mode checks to return ESTALE definitely has the
chance of breaking stuff that uses bulkstat (e.g. xfsdump) because
it will return errors now where it previously just skipped the
(unlinked) inode.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [xfs-masters] [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[Christoph Hellwig]

On Sat, Jul 16, 2011 at 11:50:14AM +1000, Dave Chinner wrote:
> Bulkstat also checks for ENOENT returns from xfs_iget() as the
> lookup it does is inherently racy. That is, if the inode is
> allocated when it is read from the IBT, but then has been unlinked
> before the actual xfs_iget() call, it expects to get back an ENOENT
> to tell it the inode has been unlinked and shouldn't be included in
> the bulkstat output. Otherwise, it returns the error to userspace.
> 
> So converting the mode checks to return ESTALE definitely has the
> chance of breaking stuff that uses bulkstat (e.g. xfsdump) because
> it will return errors now where it previously just skipped the
> (unlinked) inode.

Sounds like we should simply go with the original version of the patch
then.

Re: [xfs-masters] [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[J. Bruce Fields]

On Fri, Jul 15, 2011 at 09:53:46PM -0400, Christoph Hellwig wrote:
> On Sat, Jul 16, 2011 at 11:50:14AM +1000, Dave Chinner wrote:
> > Bulkstat also checks for ENOENT returns from xfs_iget() as the
> > lookup it does is inherently racy. That is, if the inode is
> > allocated when it is read from the IBT, but then has been unlinked
> > before the actual xfs_iget() call, it expects to get back an ENOENT
> > to tell it the inode has been unlinked and shouldn't be included in
> > the bulkstat output. Otherwise, it returns the error to userspace.
> > 
> > So converting the mode checks to return ESTALE definitely has the
> > chance of breaking stuff that uses bulkstat (e.g. xfsdump) because
> > it will return errors now where it previously just skipped the
> > (unlinked) inode.
> 
> Sounds like we should simply go with the original version of the patch
> then.

OK.  Let me know if someone needs that resent....

--b.

Re: [xfs-masters] [PATCH] xfs: failure mapping nfs fh to inode should return ESTALE

[Alex Elder]

On Mon, 2011-07-18 at 09:37 -0400, J. Bruce Fields wrote:
> On Fri, Jul 15, 2011 at 09:53:46PM -0400, Christoph Hellwig wrote:
> > On Sat, Jul 16, 2011 at 11:50:14AM +1000, Dave Chinner wrote:
> > > Bulkstat also checks for ENOENT returns from xfs_iget() as the
> > > lookup it does is inherently racy. That is, if the inode is
> > > allocated when it is read from the IBT, but then has been unlinked
> > > before the actual xfs_iget() call, it expects to get back an ENOENT
> > > to tell it the inode has been unlinked and shouldn't be included in
> > > the bulkstat output. Otherwise, it returns the error to userspace.
> > > 
> > > So converting the mode checks to return ESTALE definitely has the
> > > chance of breaking stuff that uses bulkstat (e.g. xfsdump) because
> > > it will return errors now where it previously just skipped the
> > > (unlinked) inode.
> > 
> > Sounds like we should simply go with the original version of the patch
> > then.
> 
> OK.  Let me know if someone needs that resent....
> 
> --b.

No need.  I'll use the original one.  Thanks.	-Alex