Return-Path: Message-ID: <1497558869.4607.15.camel@redhat.com> Subject: Re: [PATCH 2/2] NFS: nfs_rename() - revalidate directories on -ERESTARTSYS From: Jeff Layton To: Benjamin Coddington Cc: Trond Myklebust , Anna Schumaker , mszeredi@redhat.com, bfields@redhat.com, linux-nfs@vger.kernel.org Date: Thu, 15 Jun 2017 16:34:29 -0400 In-Reply-To: <4D7B208E-553B-4170-8081-12DE3881A04C@redhat.com> References: <32ef5d3ded4fe75bb6fc6e1a1aebdd0297257d9e.1497541002.git.bcodding@redhat.com> <1497550694.4607.10.camel@redhat.com> <1497553607.4607.13.camel@poochiereds.net> <4D7B208E-553B-4170-8081-12DE3881A04C@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: 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 > > > > --- > > > > 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