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-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
