On Thu, 2017-06-15 at 14:18 -0400, Jeff Layton wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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?
>
As Ben pointed out on IRC, that flag is checked in rpc_release, so as
long as we ensure that it's set while we hold a task reference we should
be fine here without any locking.
That said, do we need a barrier here? We do need to ensure that
cancelled is set before the rpc_put_task occurs.
> > 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@xxxxxxxxxxxxxxx>
--
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
