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

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

[fdmanana]

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>
---
 fs/btrfs/inode.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e5558d9..1ae0fc8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3419,10 +3419,10 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 		found_key.offset = 0;
 		inode = btrfs_iget(root->fs_info->sb, &found_key, root, NULL);
 		ret = PTR_ERR_OR_ZERO(inode);
-		if (ret && ret != -ESTALE)
+		if (ret && ret != -ENOENT)
 			goto out;
 
-		if (ret == -ESTALE && root == root->fs_info->tree_root) {
+		if (ret == -ENOENT && root == root->fs_info->tree_root) {
 			struct btrfs_root *dead_root;
 			struct btrfs_fs_info *fs_info = root->fs_info;
 			int is_dead_root = 0;
@@ -3458,7 +3458,7 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
 		 * Inode is already gone but the orphan item is still there,
 		 * kill the orphan item.
 		 */
-		if (ret == -ESTALE) {
+		if (ret == -ENOENT) {
 			trans = btrfs_start_transaction(root, 1);
 			if (IS_ERR(trans)) {
 				ret = PTR_ERR(trans);
@@ -3617,7 +3617,7 @@ static noinline int acls_after_inode_item(struct extent_buffer *leaf,
 /*
  * read an inode from the btree into the in-memory inode
  */
-static void btrfs_read_locked_inode(struct inode *inode)
+static int btrfs_read_locked_inode(struct inode *inode)
 {
 	struct btrfs_path *path;
 	struct extent_buffer *leaf;
@@ -3636,14 +3636,19 @@ static void btrfs_read_locked_inode(struct inode *inode)
 		filled = true;
 
 	path = btrfs_alloc_path();
-	if (!path)
+	if (!path) {
+		ret = -ENOMEM;
 		goto make_bad;
+	}
 
 	memcpy(&location, &BTRFS_I(inode)->location, sizeof(location));
 
 	ret = btrfs_lookup_inode(NULL, root, path, &location, 0);
-	if (ret)
+	if (ret) {
+		if (ret > 0)
+			ret = -ENOENT;
 		goto make_bad;
+	}
 
 	leaf = path->nodes[0];
 
@@ -3796,11 +3801,12 @@ cache_acl:
 	}
 
 	btrfs_update_iflags(inode);
-	return;
+	return 0;
 
 make_bad:
 	btrfs_free_path(path);
 	make_bad_inode(inode);
+	return ret;
 }
 
 /*
@@ -5585,7 +5591,9 @@ struct inode *btrfs_iget(struct super_block *s, struct btrfs_key *location,
 		return ERR_PTR(-ENOMEM);
 
 	if (inode->i_state & I_NEW) {
-		btrfs_read_locked_inode(inode);
+		int ret;
+
+		ret = btrfs_read_locked_inode(inode);
 		if (!is_bad_inode(inode)) {
 			inode_tree_add(inode);
 			unlock_new_inode(inode);
@@ -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);
 		}
 	}
 
-- 
2.7.0.rc3

[PATCH 2/2] Btrfs: improve performance on fsync against new inode after rename/unlink

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

With commit 56f23fdbb600 ("Btrfs: fix file/data loss caused by fsync after
rename and new inode") we got simple fix for a functional issue when the
following sequence of actions is done:

  at transaction N
  create file A at directory D
  at transaction N + M (where M >= 1)
  move/rename existing file A from directory D to directory E
  create a new file named A at directory D
  fsync the new file
  power fail

The solution was to simply detect such scenario and fallback to a full
transaction commit when we detect it. However this turned out to had a
significant impact on throughput (and a bit on latency too) for benchmarks
using the dbench tool, which simulates real workloads from smbd (Samba)
servers. For example on a test vm (with a debug kernel):

Unpatched:
Throughput 19.1572 MB/sec  32 clients  32 procs  max_latency=1005.229 ms

Patched:
Throughput 23.7015 MB/sec  32 clients  32 procs  max_latency=809.206 ms

The patched results (this patch is applied) are similar to the results of
a kernel with the commit 56f23fdbb600 ("Btrfs: fix file/data loss caused
by fsync after rename and new inode") reverted.

This change avoids the fallback to a transaction commit and instead makes
sure all the names of the conflicting inode (the one that had a name in a
past transaction that matches the name of the new file in the same parent
directory) are logged so that at log replay time we don't lose neither the
new file nor the old file, and the old file gets the name it was renamed
to.

This also ends up avoiding a full transaction commit for a similar case
that involves an unlink instead of a rename of the old file:

  at transaction N
  create file A at directory D
  at transaction N + M (where M >= 1)
  remove file A
  create a new file named A at directory D
  fsync the new file
  power fail

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/inode.c    | 19 +++++++++++-
 fs/btrfs/tree-log.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1ae0fc8..6dcd38b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4194,6 +4194,7 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 	int err = 0;
 	struct btrfs_root *root = BTRFS_I(dir)->root;
 	struct btrfs_trans_handle *trans;
+	u64 last_unlink_trans;
 
 	if (inode->i_size > BTRFS_EMPTY_DIR_SIZE)
 		return -ENOTEMPTY;
@@ -4216,11 +4217,27 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 	if (err)
 		goto out;
 
+	last_unlink_trans = BTRFS_I(inode)->last_unlink_trans;
+
 	/* now the directory is empty */
 	err = btrfs_unlink_inode(trans, root, dir, d_inode(dentry),
 				 dentry->d_name.name, dentry->d_name.len);
-	if (!err)
+	if (!err) {
 		btrfs_i_size_write(inode, 0);
+		/*
+		 * Propagate the last_unlink_trans value of the deleted dir to
+		 * its parent directory. This is to prevent an unrecoverable
+		 * log tree in the case we do something like this:
+		 * 1) create dir foo
+		 * 2) create snapshot under dir foo
+		 * 3) delete the snapshot
+		 * 4) rmdir foo
+		 * 5) mkdir foo
+		 * 6) fsync foo or some file inside foo
+		 */
+		if (last_unlink_trans >= trans->transid)
+			BTRFS_I(dir)->last_unlink_trans = last_unlink_trans;
+	}
 out:
 	btrfs_end_transaction(trans, root);
 	btrfs_btree_balance_dirty(root);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index aa8fed1..0c39afb 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -4469,7 +4469,8 @@ static int btrfs_log_trailing_hole(struct btrfs_trans_handle *trans,
 static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 					 const int slot,
 					 const struct btrfs_key *key,
-					 struct inode *inode)
+					 struct inode *inode,
+					 u64 *other_ino)
 {
 	int ret;
 	struct btrfs_path *search_path;
@@ -4528,7 +4529,16 @@ static int btrfs_check_ref_name_override(struct extent_buffer *eb,
 					   search_path, parent,
 					   name, this_name_len, 0);
 		if (di && !IS_ERR(di)) {
-			ret = 1;
+			struct btrfs_key di_key;
+
+			btrfs_dir_item_key_to_cpu(search_path->nodes[0],
+						  di, &di_key);
+			if (di_key.type == BTRFS_INODE_ITEM_KEY) {
+				ret = 1;
+				*other_ino = di_key.objectid;
+			} else {
+				ret = -EAGAIN;
+			}
 			goto out;
 		} else if (IS_ERR(di)) {
 			ret = PTR_ERR(di);
@@ -4718,16 +4728,71 @@ again:
 		if ((min_key.type == BTRFS_INODE_REF_KEY ||
 		     min_key.type == BTRFS_INODE_EXTREF_KEY) &&
 		    BTRFS_I(inode)->generation == trans->transid) {
+			u64 other_ino = 0;
+
 			ret = btrfs_check_ref_name_override(path->nodes[0],
 							    path->slots[0],
-							    &min_key, inode);
+							    &min_key, inode,
+							    &other_ino);
 			if (ret < 0) {
 				err = ret;
 				goto out_unlock;
 			} else if (ret > 0) {
-				err = 1;
-				btrfs_set_log_full_commit(root->fs_info, trans);
-				goto out_unlock;
+				struct btrfs_key inode_key;
+				struct inode *other_inode;
+
+				if (ins_nr > 0) {
+					ins_nr++;
+				} else {
+					ins_nr = 1;
+					ins_start_slot = path->slots[0];
+				}
+				ret = copy_items(trans, inode, dst_path, path,
+						 &last_extent, ins_start_slot,
+						 ins_nr, inode_only,
+						 logged_isize);
+				if (ret < 0) {
+					err = ret;
+					goto out_unlock;
+				}
+				ins_nr = 0;
+				btrfs_release_path(path);
+				inode_key.objectid = other_ino;
+				inode_key.type = BTRFS_INODE_ITEM_KEY;
+				inode_key.offset = 0;
+				other_inode = btrfs_iget(root->fs_info->sb,
+							 &inode_key, root,
+							 NULL);
+				/*
+				 * If the other inode that had a conflicting dir
+				 * entry was deleted in the current transaction,
+				 * we don't need to do more work nor fallback to
+				 * a transaction commit.
+				 */
+				if (IS_ERR(other_inode) &&
+				    PTR_ERR(other_inode) == -ENOENT) {
+					goto next_key;
+				} else if (IS_ERR(other_inode)) {
+					err = PTR_ERR(other_inode);
+					goto out_unlock;
+				}
+				/*
+				 * We are safe logging the other inode without
+				 * acquiring its i_mutex as long as we log with
+				 * the LOG_INODE_EXISTS mode. We're safe against
+				 * concurrent renames of the other inode as well
+				 * because during a rename we pin the log and
+				 * update the log with the new name before we
+				 * unpin it.
+				 */
+				err = btrfs_log_inode(trans, root, other_inode,
+						      LOG_INODE_EXISTS,
+						      0, LLONG_MAX, ctx);
+				iput(other_inode);
+				if (err)
+					goto out_unlock;
+				else
+					goto next_key;
 			}
 		}
 
@@ -4795,7 +4860,7 @@ next_slot:
 			ins_nr = 0;
 		}
 		btrfs_release_path(path);
-
+next_key:
 		if (min_key.offset < (u64)-1) {
 			min_key.offset++;
 		} else if (min_key.type < max_key.type) {
@@ -4989,8 +5054,12 @@ static noinline int check_parent_dirs_for_sync(struct btrfs_trans_handle *trans,
 		if (!parent || d_really_is_negative(parent) || sb != d_inode(parent)->i_sb)
 			break;
 
-		if (IS_ROOT(parent))
+		if (IS_ROOT(parent)) {
+			inode = d_inode(parent);
+			if (btrfs_must_commit_transaction(trans, inode))
+				ret = 1;
 			break;
+		}
 
 		parent = dget_parent(parent);
 		dput(old_parent);
-- 
2.7.0.rc3

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

[NeilBrown]

[-- 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 --]

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

[J. Bruce Fields]

On Fri, Jul 22, 2016 at 11:08:17AM +1000, NeilBrown wrote:
> 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?

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.

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

[NeilBrown]

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

On Fri, Jul 22 2016, J. Bruce Fields wrote:

> On Fri, Jul 22, 2016 at 11:08:17AM +1000, NeilBrown wrote:
>> 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?
>
> 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().
>

I can certainly agree with that perspective, though it would be
appropriate in that case to make sure we document the requirements for
fh_to_dentry (the current spelling for 'decode_fh').  So I went looking
for documentation and found:

 * fh_to_dentry:
 *    @fh_to_dentry is given a &struct super_block (@sb) and a file handle
 *    fragment (@fh, @fh_len). It should return a &struct dentry which refers
 *    to the same file that the file handle fragment refers to.  If it cannot,
 *    it should return a %NULL pointer if the file was found but no acceptable
 *    &dentries were available, or an %ERR_PTR error code indicating why it
 *    couldn't be found (e.g. %ENOENT or %ENOMEM).  Any suitable dentry can be
 *    returned including, if necessary, a new dentry created with d_alloc_root.
 *    The caller can then find any other extant dentries by following the
 *    d_alias links.
 *

So the new btrfs code is actually conformant!!
That documentation dates back to 2002 when I wrote it....
And it looks like ENOENT wasn't handled correctly then :-(

I suspect anything that isn't ENOMEM should be converted to ESTALE.
ENOMEM causes the client to be asked to retry the request later.

Does this look reasonable to you?
(Adding Christof as he as contributed a lot to exportfs)

If there is agreement I'll test and post a proper patch.

Thanks,
NeilBrown


diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 207ba8d627ca..3527b58cd5bc 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 	if (!nop || !nop->fh_to_dentry)
 		return ERR_PTR(-ESTALE);
 	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
-	if (!result)
-		result = ERR_PTR(-ESTALE);
-	if (IS_ERR(result))
-		return result;
+	if (PTR_ERR(result) == -ENOMEM)
+		return ERR_CAST(result)
+	if (IS_ERR_OR_NULL(result))
+		return ERR_PTR(-ESTALE);
 
 	if (d_is_dir(result)) {
 		/*
@@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 
  err_result:
 	dput(result);
+	if (err != -ENOMEM)
+		err = -ESTALE;
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(exportfs_decode_fh);

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

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

[J. Bruce Fields]

On Fri, Jul 22, 2016 at 12:40:26PM +1000, NeilBrown wrote:
> On Fri, Jul 22 2016, J. Bruce Fields wrote:
> 
> > On Fri, Jul 22, 2016 at 11:08:17AM +1000, NeilBrown wrote:
> >> 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?
> >
> > 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().
> >
> 
> I can certainly agree with that perspective, though it would be
> appropriate in that case to make sure we document the requirements for
> fh_to_dentry (the current spelling for 'decode_fh').  So I went looking
> for documentation and found:
> 
>  * fh_to_dentry:
>  *    @fh_to_dentry is given a &struct super_block (@sb) and a file handle
>  *    fragment (@fh, @fh_len). It should return a &struct dentry which refers
>  *    to the same file that the file handle fragment refers to.  If it cannot,
>  *    it should return a %NULL pointer if the file was found but no acceptable
>  *    &dentries were available, or an %ERR_PTR error code indicating why it
>  *    couldn't be found (e.g. %ENOENT or %ENOMEM).  Any suitable dentry can be
>  *    returned including, if necessary, a new dentry created with d_alloc_root.
>  *    The caller can then find any other extant dentries by following the
>  *    d_alias links.
>  *
> 
> So the new btrfs code is actually conformant!!
> That documentation dates back to 2002 when I wrote it....
> And it looks like ENOENT wasn't handled correctly then :-(
> 
> I suspect anything that isn't ENOMEM should be converted to ESTALE.
> ENOMEM causes the client to be asked to retry the request later.
> 
> Does this look reasonable to you?
> (Adding Christof as he as contributed a lot to exportfs)
> 
> If there is agreement I'll test and post a proper patch.

I can live with it.  It bothers me that we're losing potentially useful
information about what went wrong in the filesystem.  Maybe this is a
place a dprintk could be handy?

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 207ba8d627ca..3527b58cd5bc 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  	if (!nop || !nop->fh_to_dentry)
>  		return ERR_PTR(-ESTALE);
>  	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
> -	if (!result)
> -		result = ERR_PTR(-ESTALE);
> -	if (IS_ERR(result))
> -		return result;
> +	if (PTR_ERR(result) == -ENOMEM)
> +		return ERR_CAST(result)
> +	if (IS_ERR_OR_NULL(result))
> +		return ERR_PTR(-ESTALE);
>  
>  	if (d_is_dir(result)) {
>  		/*
> @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  
>   err_result:
>  	dput(result);
> +	if (err != -ENOMEM)
> +		err = -ESTALE;
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL_GPL(exportfs_decode_fh);

[PATCH] exportfs: be careful to only return expected errors.

[NeilBrown]

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



When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
In particular it can be tempting to return ENOENT, but this is not
handled well by nfsd.

Rather than requiring strict adherence to error code code filesystems,
treat all unexpected error codes the same as ESTALE.  This is safest.

Signed-off-by: NeilBrown <neilb@suse.com>
---

I didn't add a dprintk for unexpected error messages, partly
because dprintk isn't usable in exportfs.  I could have used pr_debug()
but I really didn't see much value.

This has been tested together with the btrfs change, and it restores
correct functionality.

Thanks,
NeilBrown

 fs/exportfs/expfs.c      | 10 ++++++----
 include/linux/exportfs.h | 13 +++++++------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 207ba8d627ca..a4b531be9168 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 	if (!nop || !nop->fh_to_dentry)
 		return ERR_PTR(-ESTALE);
 	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
-	if (!result)
-		result = ERR_PTR(-ESTALE);
-	if (IS_ERR(result))
-		return result;
+	if (PTR_ERR(result) == -ENOMEM)
+		return ERR_CAST(result);
+	if (IS_ERR_OR_NULL(result))
+		return ERR_PTR(-ESTALE);
 
 	if (d_is_dir(result)) {
 		/*
@@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
 
  err_result:
 	dput(result);
+	if (err != -ENOMEM)
+		err = -ESTALE;
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(exportfs_decode_fh);
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index b03c0625fa6e..5ab958cdc50b 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -157,12 +157,13 @@ struct fid {
  *    @fh_to_dentry is given a &struct super_block (@sb) and a file handle
  *    fragment (@fh, @fh_len). It should return a &struct dentry which refers
  *    to the same file that the file handle fragment refers to.  If it cannot,
- *    it should return a %NULL pointer if the file was found but no acceptable
- *    &dentries were available, or an %ERR_PTR error code indicating why it
- *    couldn't be found (e.g. %ENOENT or %ENOMEM).  Any suitable dentry can be
- *    returned including, if necessary, a new dentry created with d_alloc_root.
- *    The caller can then find any other extant dentries by following the
- *    d_alias links.
+ *    it should return a %NULL pointer if the file cannot be found, or an
+ *    %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred.
+ *    Any other error code is treated like %NULL, and will cause an %ESTALE error
+ *    for callers of exportfs_decode_fh().
+ *    Any suitable dentry can be returned including, if necessary, a new dentry
+ *    created with d_alloc_root.  The caller can then find any other extant
+ *    dentries by following the d_alias links.
  *
  * fh_to_parent:
  *    Same as @fh_to_dentry, except that it returns a pointer to the parent
-- 
2.9.2


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

Re: [PATCH] exportfs: be careful to only return expected errors.

[Christoph Hellwig]

On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote:
> 
> 
> When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
> In particular it can be tempting to return ENOENT, but this is not
> handled well by nfsd.
> 
> Rather than requiring strict adherence to error code code filesystems,
> treat all unexpected error codes the same as ESTALE.  This is safest.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> 
> I didn't add a dprintk for unexpected error messages, partly
> because dprintk isn't usable in exportfs.  I could have used pr_debug()
> but I really didn't see much value.
> 
> This has been tested together with the btrfs change, and it restores
> correct functionality.

I don't really like all this magic which is partially historic.  I think
we should instead allow the fs to return any error from the export
operations, and forbid returning NULL entirely.  Then the actualy caller
(nfsd) can sort out which errors it wants to send over the wire.

Re: [PATCH] exportfs: be careful to only return expected errors.

[J. Bruce Fields]

On Thu, Aug 04, 2016 at 05:47:19AM -0700, Christoph Hellwig wrote:
> On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote:
> > 
> > 
> > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
> > In particular it can be tempting to return ENOENT, but this is not
> > handled well by nfsd.
> > 
> > Rather than requiring strict adherence to error code code filesystems,
> > treat all unexpected error codes the same as ESTALE.  This is safest.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.com>
> > ---
> > 
> > I didn't add a dprintk for unexpected error messages, partly
> > because dprintk isn't usable in exportfs.  I could have used pr_debug()
> > but I really didn't see much value.
> > 
> > This has been tested together with the btrfs change, and it restores
> > correct functionality.
> 
> I don't really like all this magic which is partially historic.  I think
> we should instead allow the fs to return any error from the export
> operations,

What errors other than ENOENT and ENOMEM do you think are reasonable?

ENOENT is going to screw up both nfsd and open_by_fhandle_at, which are
the only callers.

> and forbid returning NULL entirely.  Then the actualy caller
> (nfsd) can sort out which errors it wants to send over the wire.

The needs of those two callers don't look very different to me, and I
can't recall seeing a correct use of an error other than ESTALE or
ENOMEM, so I've been thinking of it more of a question of how to best
handle a misbehaving filesystem.

--b.

Re: [PATCH] exportfs: be careful to only return expected errors.

[NeilBrown]

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

On Thu, Aug 04 2016, Christoph Hellwig wrote:

> On Thu, Aug 04, 2016 at 10:19:06AM +1000, NeilBrown wrote:
>> 
>> 
>> When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
>> In particular it can be tempting to return ENOENT, but this is not
>> handled well by nfsd.
>> 
>> Rather than requiring strict adherence to error code code filesystems,
>> treat all unexpected error codes the same as ESTALE.  This is safest.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> 
>> I didn't add a dprintk for unexpected error messages, partly
>> because dprintk isn't usable in exportfs.  I could have used pr_debug()
>> but I really didn't see much value.
>> 
>> This has been tested together with the btrfs change, and it restores
>> correct functionality.
>
> I don't really like all this magic which is partially historic.  I think
> we should instead allow the fs to return any error from the export
> operations, and forbid returning NULL entirely.  Then the actualy caller
> (nfsd) can sort out which errors it wants to send over the wire.

I'm certainly open to that possibility.
But is the "actual caller":
  nfsd_set_fh_dentry(), or
  fh_verify() or
  the various callers of fh_verify() which might have different rules
  about which error codess are acceptable?

I could probably make an argument for having fh_verify() be careful
about error codes, but as exportfs_decode_fh() is a more public
interface, I think it is more important that it have well defined error
options.

Are there *any* errors that could sensibly be returned from
exportfs_decode_fh() other than
  -ESTALE (there is no such file), or
  -ENOMEM (there probably is a file, but I cannot allocate a dentry for
           it) or
  -EACCES (there is such a file, but it isn't "acceptable")

???

If there aren't, why should we let them through?

NeilBrown

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

Re: [PATCH] exportfs: be careful to only return expected errors.

[NeilBrown]

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

On Thu, Aug 04 2016, NeilBrown wrote:

>
>
> When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
> In particular it can be tempting to return ENOENT, but this is not
> handled well by nfsd.
>
> Rather than requiring strict adherence to error code code filesystems,
> treat all unexpected error codes the same as ESTALE.  This is safest.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>
> I didn't add a dprintk for unexpected error messages, partly
> because dprintk isn't usable in exportfs.  I could have used pr_debug()
> but I really didn't see much value.
>
> This has been tested together with the btrfs change, and it restores
> correct functionality.
>
> Thanks,
> NeilBrown


Hi Bruce,
 I notice that this patch isn't in -next, but the btrfs change which
 motivated it is.
 That means, unless there is some other work around somewhere, btrfs
 might start having problems with nfs export.

 Can we get this patch into 4.9-rc??

 Or has someone fixed it a different way?

Thanks,
NeilBrown


>
>  fs/exportfs/expfs.c      | 10 ++++++----
>  include/linux/exportfs.h | 13 +++++++------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 207ba8d627ca..a4b531be9168 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  	if (!nop || !nop->fh_to_dentry)
>  		return ERR_PTR(-ESTALE);
>  	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
> -	if (!result)
> -		result = ERR_PTR(-ESTALE);
> -	if (IS_ERR(result))
> -		return result;
> +	if (PTR_ERR(result) == -ENOMEM)
> +		return ERR_CAST(result);
> +	if (IS_ERR_OR_NULL(result))
> +		return ERR_PTR(-ESTALE);
>  
>  	if (d_is_dir(result)) {
>  		/*
> @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>  
>   err_result:
>  	dput(result);
> +	if (err != -ENOMEM)
> +		err = -ESTALE;
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL_GPL(exportfs_decode_fh);
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index b03c0625fa6e..5ab958cdc50b 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -157,12 +157,13 @@ struct fid {
>   *    @fh_to_dentry is given a &struct super_block (@sb) and a file handle
>   *    fragment (@fh, @fh_len). It should return a &struct dentry which refers
>   *    to the same file that the file handle fragment refers to.  If it cannot,
> - *    it should return a %NULL pointer if the file was found but no acceptable
> - *    &dentries were available, or an %ERR_PTR error code indicating why it
> - *    couldn't be found (e.g. %ENOENT or %ENOMEM).  Any suitable dentry can be
> - *    returned including, if necessary, a new dentry created with d_alloc_root.
> - *    The caller can then find any other extant dentries by following the
> - *    d_alias links.
> + *    it should return a %NULL pointer if the file cannot be found, or an
> + *    %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred.
> + *    Any other error code is treated like %NULL, and will cause an %ESTALE error
> + *    for callers of exportfs_decode_fh().
> + *    Any suitable dentry can be returned including, if necessary, a new dentry
> + *    created with d_alloc_root.  The caller can then find any other extant
> + *    dentries by following the d_alias links.
>   *
>   * fh_to_parent:
>   *    Same as @fh_to_dentry, except that it returns a pointer to the parent
> -- 
> 2.9.2

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

Re: [PATCH] exportfs: be careful to only return expected errors.

[J. Bruce Fields]

On Thu, Oct 06, 2016 at 05:39:24PM +1100, NeilBrown wrote:
> On Thu, Aug 04 2016, NeilBrown wrote:
> 
> >
> >
> > When nfsd calls fh_to_dentry, it expect ESTALE or ENOMEM as errors.
> > In particular it can be tempting to return ENOENT, but this is not
> > handled well by nfsd.
> >
> > Rather than requiring strict adherence to error code code filesystems,
> > treat all unexpected error codes the same as ESTALE.  This is safest.
> >
> > Signed-off-by: NeilBrown <neilb@suse.com>
> > ---
> >
> > I didn't add a dprintk for unexpected error messages, partly
> > because dprintk isn't usable in exportfs.  I could have used pr_debug()
> > but I really didn't see much value.
> >
> > This has been tested together with the btrfs change, and it restores
> > correct functionality.
> >
> > Thanks,
> > NeilBrown
> 
> 
> Hi Bruce,
>  I notice that this patch isn't in -next, but the btrfs change which
>  motivated it is.
>  That means, unless there is some other work around somewhere, btrfs
>  might start having problems with nfs export.

Whoops, I wonder what happened.  Looking back....  Oh, I think I set it
aside pending a response from Christoph, but I don't think that's really
necessary.

>  Can we get this patch into 4.9-rc??
> 
>  Or has someone fixed it a different way?

No, I'll just apply.

I need to send a pull request soon, I just have to make up my mind on
COPY first.

--b.

> Thanks,
> NeilBrown
> 
> 
> >
> >  fs/exportfs/expfs.c      | 10 ++++++----
> >  include/linux/exportfs.h | 13 +++++++------
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 207ba8d627ca..a4b531be9168 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -428,10 +428,10 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> >  	if (!nop || !nop->fh_to_dentry)
> >  		return ERR_PTR(-ESTALE);
> >  	result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
> > -	if (!result)
> > -		result = ERR_PTR(-ESTALE);
> > -	if (IS_ERR(result))
> > -		return result;
> > +	if (PTR_ERR(result) == -ENOMEM)
> > +		return ERR_CAST(result);
> > +	if (IS_ERR_OR_NULL(result))
> > +		return ERR_PTR(-ESTALE);
> >  
> >  	if (d_is_dir(result)) {
> >  		/*
> > @@ -541,6 +541,8 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> >  
> >   err_result:
> >  	dput(result);
> > +	if (err != -ENOMEM)
> > +		err = -ESTALE;
> >  	return ERR_PTR(err);
> >  }
> >  EXPORT_SYMBOL_GPL(exportfs_decode_fh);
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index b03c0625fa6e..5ab958cdc50b 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -157,12 +157,13 @@ struct fid {
> >   *    @fh_to_dentry is given a &struct super_block (@sb) and a file handle
> >   *    fragment (@fh, @fh_len). It should return a &struct dentry which refers
> >   *    to the same file that the file handle fragment refers to.  If it cannot,
> > - *    it should return a %NULL pointer if the file was found but no acceptable
> > - *    &dentries were available, or an %ERR_PTR error code indicating why it
> > - *    couldn't be found (e.g. %ENOENT or %ENOMEM).  Any suitable dentry can be
> > - *    returned including, if necessary, a new dentry created with d_alloc_root.
> > - *    The caller can then find any other extant dentries by following the
> > - *    d_alias links.
> > + *    it should return a %NULL pointer if the file cannot be found, or an
> > + *    %ERR_PTR error code of %ENOMEM if a memory allocation failure occurred.
> > + *    Any other error code is treated like %NULL, and will cause an %ESTALE error
> > + *    for callers of exportfs_decode_fh().
> > + *    Any suitable dentry can be returned including, if necessary, a new dentry
> > + *    created with d_alloc_root.  The caller can then find any other extant
> > + *    dentries by following the d_alias links.
> >   *
> >   * fh_to_parent:
> >   *    Same as @fh_to_dentry, except that it returns a pointer to the parent
> > -- 
> > 2.9.2