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.
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.
Ben
--
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
