On Thu, 2017-06-15 at 16:19 -0400, Benjamin Coddington wrote:
> On 15 Jun 2017, at 15:06, Jeff Layton wrote:
>
> > 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.
>
> Yes, I think we probably do.
>
Yeah, I think a smp_wmb() there, paired with the implied barrier in the
atomic_dec_and_test in rpc_put_task?
>
> > > > 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);
>
> Jeff's pointed out on IRC that we should hold the i_lock to call
> nfs_force_lookup_revalidate(), so I'll add that.
>
>
> > > > + }
> > > > +
> > > > 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?
>
> or
>
> unsigned int : 1
>
> which seems to be often used -- see nfs4_opendata. The cancelled flag
> could
> be changed there as well I suppose.
I'd prefer a bool, but it's really up to Trond and Anna, I suppose.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
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
