Re: [PATCH 1/2] Btrfs: be more precise on errors when getting an inode from disk

[NeilBrown <neilb@suse.com>]

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

On Fri, Jun 10 2016, fdmanana@kernel.org wrote:

> From: Filipe Manana <fdmanana@suse.com>
>
> 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@suse.com>

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?

Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]