> On Mar 15, 2017, at 10:00 AM, Benjamin Coddington <bcodding@xxxxxxxxxx> wrote:
>
> On 14 Mar 2017, at 18:25, Benjamin Coddington wrote:
>
>> Hi Chuck,
>>
>> On 14 Mar 2017, at 11:07, Chuck Lever wrote:
>>
>>> Hi Ben!
>>>
>>> After addressing some IB stack issues that arose in v4.11-rc1,
>>> I was finally able to run my standard suite of NFS tests with
>>> v4.11-rc2. I found that part of the git regression suite fails
>>> with this kernel.
>>> ...
>>> A wire capture shows that some GETATTR operations receive
>>> an NFS3ERR_STALE response from the server. I looked at one
>>> of these, and it's against a renamed file "index.lock".
>>>
>>> So I reverted:
>>>
>>> commit 920b4530fb80430ff30ef83efe21ba1fa5623731
>>> Author: Benjamin Coddington <bcodding@xxxxxxxxxx>
>>> Date: Wed Feb 1 00:00:07 2017 -0500
>>>
>>> NFS: nfs_rename() handle -ERESTARTSYS dentry left behind
>>>
>>> And t9200 now passes:
>>
>> Yep, something seems to be wrong with calling
>> nfs_mark_for_revalidate(old_inode) in rpc_call_done instead of after
>> rpc_release. Thanks for catching this. I'm out of time today, and will
>> look at this in the morning.
>
> I found the test was doing something like:
>
> rename("/mnt/localhost/attic_gremlin,", "/mnt/localhost/attic_gremlin,v") = 0
> open("attic_gremlin", O_RDONLY) = 7
> open("attic_gremlin,v", O_RDONLY) = 8
>
> So, I think the problem is that patch moved the d_move() before
> d_rehash(new_inode).. so it's actually rehasing the old dentry's name. I'm testing
> the following fix:
I applied this to my client, and was not able to reproduce the problem.
Tested-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> From 425f85c59142fe0821d30b11ba0df7704ebf6de6 Mon Sep 17 00:00:00 2001
> Message-Id: <425f85c59142fe0821d30b11ba0df7704ebf6de6.1489586013.git.bcodding@xxxxxxxxxx>
> From: Benjamin Coddington <bcodding@xxxxxxxxxx>
> Date: Wed, 15 Mar 2017 08:06:38 -0400
> Subject: [PATCH] NFS: Fix old dentry rehash after move
>
> Now that nfs_rename()'s d_move has moved within the RPC task's rpc_call_done
> callback, rehashing new_dentry will actually rehash the old dentry's name
> in nfs_rename(). d_move() is going to rehash the new dentry for us anyway,
> so doing it again here is unnecessary.
>
> Reported-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> Fixes: 920b4530fb80 ("NFS: nfs_rename() handle -ERESTARTSYS dentry left behind")
> ---
> fs/nfs/dir.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index fb499a3f21b5..f92ba8d6c556 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2055,7 +2055,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> {
> struct inode *old_inode = d_inode(old_dentry);
> struct inode *new_inode = d_inode(new_dentry);
> - struct dentry *dentry = NULL, *rehash = NULL;
> + struct dentry *dentry = NULL;
> struct rpc_task *task;
> int error = -EBUSY;
>
> @@ -2078,10 +2078,8 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> * To prevent any new references to the target during the
> * rename, we unhash the dentry in advance.
> */
> - if (!d_unhashed(new_dentry)) {
> + if (!d_unhashed(new_dentry))
> d_drop(new_dentry);
> - rehash = new_dentry;
> - }
>
> if (d_count(new_dentry) > 2) {
> int err;
> @@ -2098,7 +2096,6 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> goto out;
>
> new_dentry = dentry;
> - rehash = NULL;
> new_inode = NULL;
> }
> }
> @@ -2119,8 +2116,6 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
> error = task->tk_status;
> rpc_put_task(task);
> out:
> - if (rehash)
> - d_rehash(rehash);
> trace_nfs_rename_exit(old_dir, old_dentry,
> new_dir, new_dentry, error);
> /* new dentry created? */
> --
> 2.9.3
--
Chuck Lever
--
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
