On Fri, Jul 22, 2016 at 11:08:17AM +1000, NeilBrown wrote: > On Fri, Jun 10 2016, fdmanana@xxxxxxxxxx wrote: > > > From: Filipe Manana <fdmanana@xxxxxxxx> > > > > When we attempt to read an inode from disk, we end up always returning an > > -ESTALE error to the caller regardless of the actual failure reason, which > > can be an out of memory problem (when allocating a path), some error found > > when reading from the fs/subvolume btree (like a genuine IO error) or the > > inode does not exists. So lets start returning the real error code to the > > callers so that they don't treat all -ESTALE errors as meaning that the > > inode does not exists (such as during orphan cleanup). This will also be > > needed for a subsequent patch in the same series dealing with a special > > fsync case. > > > > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx> > > SNIP > > > @@ -5594,7 +5602,8 @@ struct inode *btrfs_iget(struct super_block *s, struct btrfs_key *location, > > } else { > > unlock_new_inode(inode); > > iput(inode); > > - inode = ERR_PTR(-ESTALE); > > + ASSERT(ret < 0); > > + inode = ERR_PTR(ret < 0 ? ret : -ESTALE); > > } > > Just a heads-up. This change breaks NFS :-( > > The change in error code percolates up the call chain: > > nfs4_pufh->fh_verify->nfsd_set_fh_dentry->exportfs_decode_fh > ->btrfs_fh_to_dentry->ntrfs_get_dentry->btrfs_iget > > and nfsd returns NFS4ERR_NOENT to the client instead of NFS4ERR_STALE, > and the client doesn't handle that quite the same way. > > This doesn't mean that the change is wrong, but it could mean we need to > fix something else in the path to sanitize the error code. > > nfsd_set_fh_dentry already has > > error = nfserr_stale; > if (PTR_ERR(exp) == -ENOENT) > return error; > > if (IS_ERR(exp)) > return nfserrno(PTR_ERR(exp)); > > for a different error case, so duplicating that would work, but I doubt > it is best. At the very least we should check for valid errors, not > specific invalid ones. > > Bruce: do you have an opinion where we should make sure that PUTFH (and > various other requests) returns a valid error code? Uh, I guess not. Maybe exportfs_decode_fh? Though my kneejerk reaction is to be cranky and wonder why btrfs suddenly needs a different convention for decode_fh(). --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html