> On Dec 16, 2016, at 09:35, Jeff Layton <jlayton@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, 2016-12-16 at 14:23 +0000, Trond Myklebust wrote:
>>>
>>> On Dec 16, 2016, at 06:09, Benjamin Coddington <bcodding@xxxxxxxxxx> wrote:
>>>
>>> On 15 Dec 2016, at 17:38, Trond Myklebust wrote:
>>>
>>>>
>>>>>
>>>>> On Dec 15, 2016, at 09:48, Benjamin Coddington <bcodding@xxxxxxxxxx> 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.
>>>>>
>>>>> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
>>>>> ---
>>>>> fs/nfs/dir.c | 14 ++++++++++++--
>>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
>>>>> index 5f1af4cd1a33..5d409616f77e 100644
>>>>> --- a/fs/nfs/dir.c
>>>>> +++ b/fs/nfs/dir.c
>>>>> @@ -2100,14 +2100,24 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>>>>> d_rehash(rehash);
>>>>> trace_nfs_rename_exit(old_dir, old_dentry,
>>>>> new_dir, new_dentry, error);
>>>>> - if (!error) {
>>>>> +
>>>>> + switch (error) {
>>>>> + case 0:
>>>>> if (new_inode != NULL)
>>>>> nfs_drop_nlink(new_inode);
>>>>> d_move(old_dentry, new_dentry);
>>>>> nfs_set_verifier(new_dentry,
>>>>> nfs_save_change_attribute(new_dir));
>>>>> - } else if (error == -ENOENT)
>>>>> + break;
>>>>> + case -ENOENT:
>>>>> nfs_dentry_handle_enoent(old_dentry);
>>>>> + break;
>>>>> + case -ERESTARTSYS:
>>>>> + /* The result of the rename is unknown. Play it safe by
>>>>> + * forcing a new lookup */
>>>>> + nfs_force_lookup_revalidate(old_dir);
>>>>> + nfs_force_lookup_revalidate(new_dir);
>>>>> + }
>>>>
>>>> Doesn’t this error handling belong in nfs_async_rename_done(), or possibly in its “data->complete()” callback? There isn’t much point in forcing a new lookup until we know the RPC call has run its course.
>>>
>>> That would be more correct, however if moved there, we'd be forcing a lookup after every rename, not just a rename that was signaled. Is it worth trying to find a way to inform those functions that the wait was interrupted?
>>>
>>
>> There are already precedents for this. Look, for instance, at how the data->cancelled flag interoperates between nfs4_run_open_task() and
>> nfs4_open_release() to trigger state recovery (by issuing a close) if the RPC call was completed, but the user interrupted the operation.
>>
>> Cheers
>> Trond
>
> There is the timing to consider here as well. Once you return from this
> function the vfs is going to unlock everything without doing the d_move.
>
> Is it better to mark the directories for revalidation at that point, or
> when the RENAME reply comes in? I would think that marking it for reval
> immediately would be best. Is there an argument for waiting?
>
See above. It is pointless to revalidate before the rename() has completed.
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
