Re: [PATCH 2/2] NFS: nfs_rename() - revalidate directories on -ERESTARTSYS

[Jeff Layton <jlayton@redhat.com>]

On Thu, 2017-06-15 at 12:13 -0400, Benjamin Coddington wrote:
> An interrupted rename will leave the old dentry behind if the rename
> succeeds.  Fix this by forcing a lookup the next time through
> ->d_revalidate.
> 
> A previous attempt at solving this problem took the approach to complete
> the work of the rename asynchronously, however that approach was wrong
> since it would allow the d_move() to occur after the directory's i_mutex
> had been dropped by the original process.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/dir.c            | 2 ++
>  fs/nfs/unlink.c         | 7 +++++++
>  include/linux/nfs_xdr.h | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 1faf337b316f..bb697e5c3f6c 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2037,6 +2037,8 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	error = rpc_wait_for_completion_task(task);
>  	if (error == 0)
>  		error = task->tk_status;
> +	else
> +		((struct nfs_renamedata *)task->tk_calldata)->cancelled = 1;

This looks a bit racy. You could end up setting cancelled just after the
reply comes in and the completion callback checks it. I think that you
probably either want to do this with an atomic operation or under a lock
of some sort.

You could probably do it with an xchg() operation?

>  	rpc_put_task(task);
>  	nfs_mark_for_revalidate(old_inode);
>  out:
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index 191aa577dd1f..b47158a34879 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -288,6 +288,13 @@ static void nfs_async_rename_release(void *calldata)
>  	if (d_really_is_positive(data->old_dentry))
>  		nfs_mark_for_revalidate(d_inode(data->old_dentry));
>  
> +	/* The result of the rename is unknown. Play it safe by
> +	 * forcing a new lookup */
> +	if (data->cancelled) {
> +		nfs_force_lookup_revalidate(data->old_dir);
> +		nfs_force_lookup_revalidate(data->new_dir);
> +	}
> +
>  	dput(data->old_dentry);
>  	dput(data->new_dentry);
>  	iput(data->old_dir);
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index b28c83475ee8..2a8406b4b353 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1533,6 +1533,7 @@ struct nfs_renamedata {
>  	struct nfs_fattr	new_fattr;
>  	void (*complete)(struct rpc_task *, struct nfs_renamedata *);
>  	long timeout;
> +	int cancelled;

No need for a whole int for a flag and these do get allocated. Make it a
bool?

>  };
>  
>  struct nfs_access_entry;

-- 
Jeff Layton <jlayton@redhat.com>