| Message ID | 4ce88bdb84d0c2b5cf1db551ff9a90cc23f294ae.1497620860.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-06-16 at 09:50 -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 | 6 +++++- > fs/nfs/unlink.c | 11 +++++++++++ > include/linux/nfs_xdr.h | 1 + > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index 2ac00bf4ecf1..7108d272bc87 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -2035,7 +2035,11 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry, > } > > error = rpc_wait_for_completion_task(task); > - if (error == 0) > + if (error != 0) { > + ((struct nfs_renamedata *)task->tk_calldata)->cancelled = 1; > + /* Paired with the atomic_dec_and_test() barrier in rpc_do_put_task() */ > + smp_wmb(); > + } else > error = task->tk_status; > rpc_put_task(task); > nfs_mark_for_revalidate(old_inode); > diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c > index 191aa577dd1f..28628dde38b9 100644 > --- a/fs/nfs/unlink.c > +++ b/fs/nfs/unlink.c > @@ -288,6 +288,17 @@ 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) { > + spin_lock(&data->old_dir->i_lock); > + nfs_force_lookup_revalidate(data->old_dir); > + spin_unlock(&data->old_dir->i_lock); > + spin_lock(&data->new_dir->i_lock); > + nfs_force_lookup_revalidate(data->new_dir); > + spin_unlock(&data->new_dir->i_lock); > + } > + One more minor nit: It's quite possible that old_dir == new_dir. If that's the case you'll be doing the same operation twice here. Maybe add a check for that and only mess with new_dir here if old_dir != 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..1aca3d7c1810 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; > + bool cancelled; > }; > > struct nfs_access_entry; This looks good to me though. You can add this to both patches. Reviewed-by: Jeff Layton <jlayton@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 Jun 2017, at 10:54, Jeff Layton wrote: > On Fri, 2017-06-16 at 09:50 -0400, Benjamin Coddington wrote: >> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c >> index 191aa577dd1f..28628dde38b9 100644 >> --- a/fs/nfs/unlink.c >> +++ b/fs/nfs/unlink.c >> @@ -288,6 +288,17 @@ 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) { >> + spin_lock(&data->old_dir->i_lock); >> + nfs_force_lookup_revalidate(data->old_dir); >> + spin_unlock(&data->old_dir->i_lock); >> + spin_lock(&data->new_dir->i_lock); >> + nfs_force_lookup_revalidate(data->new_dir); >> + spin_unlock(&data->new_dir->i_lock); >> + } >> + > > One more minor nit: > > It's quite possible that old_dir == new_dir. If that's the case you'll > be doing the same operation twice here. Maybe add a check for that and > only mess with new_dir here if old_dir != new_dir? Hmm.. yes that would be better, OK! I'll send it again. >> 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..1aca3d7c1810 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; >> + bool cancelled; >> }; >> >> struct nfs_access_entry; > > This looks good to me though. You can add this to both patches. > > Reviewed-by: Jeff Layton <jlayton@redhat.com> Thanks for all the suggestions and review! Ben -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 2ac00bf4ecf1..7108d272bc87 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -2035,7 +2035,11 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry, } error = rpc_wait_for_completion_task(task); - if (error == 0) + if (error != 0) { + ((struct nfs_renamedata *)task->tk_calldata)->cancelled = 1; + /* Paired with the atomic_dec_and_test() barrier in rpc_do_put_task() */ + smp_wmb(); + } else error = task->tk_status; rpc_put_task(task); nfs_mark_for_revalidate(old_inode); diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c index 191aa577dd1f..28628dde38b9 100644 --- a/fs/nfs/unlink.c +++ b/fs/nfs/unlink.c @@ -288,6 +288,17 @@ 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) { + spin_lock(&data->old_dir->i_lock); + nfs_force_lookup_revalidate(data->old_dir); + spin_unlock(&data->old_dir->i_lock); + spin_lock(&data->new_dir->i_lock); + nfs_force_lookup_revalidate(data->new_dir); + spin_unlock(&data->new_dir->i_lock); + } + 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..1aca3d7c1810 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; + bool cancelled; }; struct nfs_access_entry;
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 | 6 +++++- fs/nfs/unlink.c | 11 +++++++++++ include/linux/nfs_xdr.h | 1 + 3 files changed, 17 insertions(+), 1 deletion(-)