| Message ID | 20110714205036.GA19457@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> -- 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
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. -- 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
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.
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. -- 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
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. -- 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
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 -- 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
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);