NFS: nfs_rename() handle -ERESTARTSYS dentry left behind
diff mbox

Message ID 11625082826baa811b3ad5b87e1e6e712c6e582d.1481813137.git.bcodding@redhat.com
State New
Headers show

Commit Message

Benjamin Coddington Dec. 15, 2016, 2:48 p.m. UTC
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@redhat.com>
---
 fs/nfs/dir.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Jeff Layton Dec. 15, 2016, 3:02 p.m. UTC | #1
On Thu, 2016-12-15 at 09:48 -0500, 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.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  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);
> +	}
>  
>  	/* new dentry created? */
>  	if (dentry)

Looks reasonable to me. 

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
Trond Myklebust Dec. 15, 2016, 10:38 p.m. UTC | #2
> On Dec 15, 2016, at 09:48, Benjamin Coddington <bcodding@redhat.com> 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@redhat.com>

> ---

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

Cheers
  Trond
Benjamin Coddington Dec. 16, 2016, 11:09 a.m. UTC | #3
On 15 Dec 2016, at 17:38, Trond Myklebust wrote:

>> On Dec 15, 2016, at 09:48, Benjamin Coddington <bcodding@redhat.com> 
>> 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@redhat.com>
>> ---
>> 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?

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
Trond Myklebust Dec. 16, 2016, 2:23 p.m. UTC | #4
DQo+IE9uIERlYyAxNiwgMjAxNiwgYXQgMDY6MDksIEJlbmphbWluIENvZGRpbmd0b24gPGJjb2Rk
aW5nQHJlZGhhdC5jb20+IHdyb3RlOg0KPiANCj4gT24gMTUgRGVjIDIwMTYsIGF0IDE3OjM4LCBU
cm9uZCBNeWtsZWJ1c3Qgd3JvdGU6DQo+IA0KPj4+IE9uIERlYyAxNSwgMjAxNiwgYXQgMDk6NDgs
IEJlbmphbWluIENvZGRpbmd0b24gPGJjb2RkaW5nQHJlZGhhdC5jb20+IHdyb3RlOg0KPj4+IA0K
Pj4+IEFuIGludGVycnVwdGVkIHJlbmFtZSB3aWxsIGxlYXZlIHRoZSBvbGQgZGVudHJ5IGJlaGlu
ZCBpZiB0aGUgcmVuYW1lDQo+Pj4gc3VjY2VlZHMuICBGaXggdGhpcyBieSBmb3JjaW5nIGEgbG9v
a3VwIHRoZSBuZXh0IHRpbWUgdGhyb3VnaA0KPj4+IC0+ZF9yZXZhbGlkYXRlLg0KPj4+IA0KPj4+
IFNpZ25lZC1vZmYtYnk6IEJlbmphbWluIENvZGRpbmd0b24gPGJjb2RkaW5nQHJlZGhhdC5jb20+
DQo+Pj4gLS0tDQo+Pj4gZnMvbmZzL2Rpci5jIHwgMTQgKysrKysrKysrKysrLS0NCj4+PiAxIGZp
bGUgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4+PiANCj4+PiBk
aWZmIC0tZ2l0IGEvZnMvbmZzL2Rpci5jIGIvZnMvbmZzL2Rpci5jDQo+Pj4gaW5kZXggNWYxYWY0
Y2QxYTMzLi41ZDQwOTYxNmY3N2UgMTAwNjQ0DQo+Pj4gLS0tIGEvZnMvbmZzL2Rpci5jDQo+Pj4g
KysrIGIvZnMvbmZzL2Rpci5jDQo+Pj4gQEAgLTIxMDAsMTQgKzIxMDAsMjQgQEAgaW50IG5mc19y
ZW5hbWUoc3RydWN0IGlub2RlICpvbGRfZGlyLCBzdHJ1Y3QgZGVudHJ5ICpvbGRfZGVudHJ5LA0K
Pj4+IAkJZF9yZWhhc2gocmVoYXNoKTsNCj4+PiAJdHJhY2VfbmZzX3JlbmFtZV9leGl0KG9sZF9k
aXIsIG9sZF9kZW50cnksDQo+Pj4gCQkJbmV3X2RpciwgbmV3X2RlbnRyeSwgZXJyb3IpOw0KPj4+
IC0JaWYgKCFlcnJvcikgew0KPj4+ICsNCj4+PiArCXN3aXRjaCAoZXJyb3IpIHsNCj4+PiArCWNh
c2UgMDoNCj4+PiAJCWlmIChuZXdfaW5vZGUgIT0gTlVMTCkNCj4+PiAJCQluZnNfZHJvcF9ubGlu
ayhuZXdfaW5vZGUpOw0KPj4+IAkJZF9tb3ZlKG9sZF9kZW50cnksIG5ld19kZW50cnkpOw0KPj4+
IAkJbmZzX3NldF92ZXJpZmllcihuZXdfZGVudHJ5LA0KPj4+IAkJCQkJbmZzX3NhdmVfY2hhbmdl
X2F0dHJpYnV0ZShuZXdfZGlyKSk7DQo+Pj4gLQl9IGVsc2UgaWYgKGVycm9yID09IC1FTk9FTlQp
DQo+Pj4gKwkJYnJlYWs7DQo+Pj4gKwljYXNlIC1FTk9FTlQ6DQo+Pj4gCQluZnNfZGVudHJ5X2hh
bmRsZV9lbm9lbnQob2xkX2RlbnRyeSk7DQo+Pj4gKwkJYnJlYWs7DQo+Pj4gKwljYXNlIC1FUkVT
VEFSVFNZUzoNCj4+PiArCQkvKiBUaGUgcmVzdWx0IG9mIHRoZSByZW5hbWUgaXMgdW5rbm93bi4g
UGxheSBpdCBzYWZlIGJ5DQo+Pj4gKwkJICogZm9yY2luZyBhIG5ldyBsb29rdXAgKi8NCj4+PiAr
CQluZnNfZm9yY2VfbG9va3VwX3JldmFsaWRhdGUob2xkX2Rpcik7DQo+Pj4gKwkJbmZzX2ZvcmNl
X2xvb2t1cF9yZXZhbGlkYXRlKG5ld19kaXIpOw0KPj4+ICsJfQ0KPj4gDQo+PiBEb2VzbuKAmXQg
dGhpcyBlcnJvciBoYW5kbGluZyBiZWxvbmcgaW4gbmZzX2FzeW5jX3JlbmFtZV9kb25lKCksIG9y
IHBvc3NpYmx5IGluIGl0cyDigJxkYXRhLT5jb21wbGV0ZSgp4oCdIGNhbGxiYWNrPyBUaGVyZSBp
c27igJl0IG11Y2ggcG9pbnQgaW4gZm9yY2luZyBhIG5ldyBsb29rdXAgdW50aWwgd2Uga25vdyB0
aGUgUlBDIGNhbGwgaGFzIHJ1biBpdHMgY291cnNlLg0KPiANCj4gVGhhdCB3b3VsZCBiZSBtb3Jl
IGNvcnJlY3QsIGhvd2V2ZXIgaWYgbW92ZWQgdGhlcmUsIHdlJ2QgYmUgZm9yY2luZyBhIGxvb2t1
cCBhZnRlciBldmVyeSByZW5hbWUsIG5vdCBqdXN0IGEgcmVuYW1lIHRoYXQgd2FzIHNpZ25hbGVk
LiAgSXMgaXQgd29ydGggdHJ5aW5nIHRvIGZpbmQgYSB3YXkgdG8gaW5mb3JtIHRob3NlIGZ1bmN0
aW9ucyB0aGF0IHRoZSB3YWl0IHdhcyBpbnRlcnJ1cHRlZD8NCj4gDQoNClRoZXJlIGFyZSBhbHJl
YWR5IHByZWNlZGVudHMgZm9yIHRoaXMuIExvb2ssIGZvciBpbnN0YW5jZSwgYXQgaG93IHRoZSBk
YXRhLT5jYW5jZWxsZWQgZmxhZyBpbnRlcm9wZXJhdGVzIGJldHdlZW4gbmZzNF9ydW5fb3Blbl90
YXNrKCkgYW5kIA0KbmZzNF9vcGVuX3JlbGVhc2UoKSB0byB0cmlnZ2VyIHN0YXRlIHJlY292ZXJ5
IChieSBpc3N1aW5nIGEgY2xvc2UpIGlmIHRoZSBSUEMgY2FsbCB3YXMgY29tcGxldGVkLCBidXQg
dGhlIHVzZXIgaW50ZXJydXB0ZWQgdGhlIG9wZXJhdGlvbi4NCg0KQ2hlZXJzDQogIFRyb25k

--
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
Jeff Layton Dec. 16, 2016, 2:35 p.m. UTC | #5
On Fri, 2016-12-16 at 14:23 +0000, Trond Myklebust wrote:
> > 
> > On Dec 16, 2016, at 06:09, Benjamin Coddington <bcodding@redhat.com> wrote:
> > 
> > On 15 Dec 2016, at 17:38, Trond Myklebust wrote:
> > 
> > > 
> > > > 
> > > > On Dec 15, 2016, at 09:48, Benjamin Coddington <bcodding@redhat.com> 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@redhat.com>
> > > > ---
> > > > 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?
Trond Myklebust Dec. 16, 2016, 2:44 p.m. UTC | #6
> On Dec 16, 2016, at 09:35, Jeff Layton <jlayton@poochiereds.net> wrote:

> 

> On Fri, 2016-12-16 at 14:23 +0000, Trond Myklebust wrote:

>>> 

>>> On Dec 16, 2016, at 06:09, Benjamin Coddington <bcodding@redhat.com> wrote:

>>> 

>>> On 15 Dec 2016, at 17:38, Trond Myklebust wrote:

>>> 

>>>> 

>>>>> 

>>>>> On Dec 15, 2016, at 09:48, Benjamin Coddington <bcodding@redhat.com> 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@redhat.com>

>>>>> ---

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

Patch
diff mbox

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);
+	}
 
 	/* new dentry created? */
 	if (dentry)