| Message ID | 11625082826baa811b3ad5b87e1e6e712c6e582d.1481813137.git.bcodding@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> 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
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
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
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?
> 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.
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)
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(-)