Drop inode after rename
Subsystems (2)
PATCH 1/1 Drop inode after rename
With recent kernels, we were seeing frequent corruption of files on NFSv2 immediately after creation, often containing pieces of of frequently written files such as .viminfo and the like.
The problem turns out to be caused by rename. The applications encountering this problem were all rewriting files by first creating a temporary copy, and finally renaming it, replacing the original file.
When doing a rename on a NFSv2 mount on top of an existing file, the inode of the overwritten file would remain, with all caches intact. If a new file created subsequently ended up with the same inode number, it would pick up the stale cache.
The fix is to decrement i_nlink of the overwritten inode, like we do for unlink, rmdir etc already.
fs/nfs/dir.c +19 -1
@@ -1429,8 +1429,15 @@ go_ahead:shrink_dcache_parent (old_dentry); } -if (new_inode) +/* d_delete will currently not do a dentry_iput +* because new_dentry->d_count is at least 2 when +* we get here. As a matter of caution +* let's grab a reference to the inode anyway. +*/ +if (new_inode &&igrab (new_inode)) { +nfs_begin_data_update (new_inode);d_delete (new_dentry); + }nfs_begin_data_update (old_dir);nfs_begin_data_update (new_dir);
@@ -1440,6 +1447,17 @@ go_ahead:nfs_end_data_update (old_inode);nfs_end_data_update (new_dir);nfs_end_data_update (old_dir); + +if (new_inode) { +/* If this was the last reference to the inode make +* sure the VFS zaps it and all associated caches. +*/ +if (error ==0 ) + new_inode->i_nlink--; +nfs_end_data_update (new_inode); +iput (new_inode); + } + out:if (rehash)d_rehash (rehash);
Signature · 12 lines
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play [email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax ------------------------------------------------------- This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar _______________________________________________ NFS maillist - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
Sorry, I sent an older version of this patch. Here's the one for 2.6.12.
Olaf
Signature · 2 lines
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play [email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax
fs/nfs/dir.c +19 -1
@@ -1445,8 +1445,15 @@ go_ahead:shrink_dcache_parent (old_dentry); } -if (new_inode) +/* d_delete will currently not do a dentry_iput +* because new_dentry->d_count is at least 2 when +* we get here. As a matter of caution +* let's grab a reference to the inode anyway. +*/ +if (new_inode &&igrab (new_inode)) { +nfs_begin_data_update (new_inode);d_delete (new_dentry); + }nfs_begin_data_update (old_dir);nfs_begin_data_update (new_dir);
@@ -1456,6 +1463,17 @@ go_ahead:nfs_end_data_update (old_inode);nfs_end_data_update (new_dir);nfs_end_data_update (old_dir); + +if (new_inode) { +/* If this was the last reference to the inode make +* sure the VFS zaps it and all associated caches. +*/ +if (error ==0 ) + new_inode->i_nlink--; +nfs_end_data_update (new_inode); +iput (new_inode); + } + out:if (rehash)d_rehash (rehash);
m=C3=A5 den 11.07.2005 Klokka 12:46 (+0200) skreiv Olaf Kirch:
Quoted 4 lines
With recent kernels, we were seeing frequent corruption of files on NFSv2 immediately after creation, often containing pieces of of frequently written files such as .viminfo and the like.
=20
Quoted 8 lines
The problem turns out to be caused by rename. The applications encountering this problem were all rewriting files by first creating a temporary copy, and finally renaming it, replacing the original file. =20
When doing a rename on a NFSv2 mount on top of an existing file, the inode of the overwritten file would remain, with all caches intact. If a new file created subsequently ended up with the same inode number, it would pick up the stale cache.
This should never happen. The inodes are not indexed on the inode number, they are indexed on the _filehandle_.
Cheers,
Trond
-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 7 lines · Trond Myklebust
Quoted 4 lines
When doing a rename on a NFSv2 mount on top of an existing file, the inode of the overwritten file would remain, with all caches intact. If a new file created subsequently ended up with the same inode number, it would pick up the stale cache.
This should never happen. The inodes are not indexed on the inode number, they are indexed on the _filehandle_.
But it does happen. The new file on the NFS server has the exact same file handle.
Olaf
Signature · 12 lines
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play [email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax ------------------------------------------------------- This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar _______________________________________________ NFS maillist - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
m=C3=A5 den 11.07.2005 Klokka 13:07 (+0200) skreiv Olaf Kirch:
Quoted 3 lines · Trond Myklebust
Quoted 2 lines
Quoted 2 lines
When doing a rename on a NFSv2 mount on top of an existing file, the inode of the overwritten file would remain, with all caches intact. I=
f
Quoted 1 line
Quoted 1 line
Quoted 1 line
a new file created subsequently ended up with the same inode number, =
it
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
would pick up the stale cache.
=20
This should never happen. The inodes are not indexed on the inode number, they are indexed on the _filehandle_.=20
But it does happen. The new file on the NFS server has the exact same file handle.
If people care enough about their data to worry about this sort of problem, then they should care enough to use a server that doesn't violate the NFS protocol.
Cheers,
Trond
-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 3 lines · Trond Myklebust
If people care enough about their data to worry about this sort of problem, then they should care enough to use a server that doesn't violate the NFS protocol.
unfsd definitely does it this way, unfortunately. I agree that it's not a very good idea to continue to use unfsd, but...
I would still vote for merging this patch, as it just does the same nlink accounting that other functions already do, eg nfs_rmdir, nfs_safe_remove, etc. The latter even has a comment preceding it that says
* We invalidate the attribute cache and free the inode prior to the operation
* to avoid possible races if the server reuses the inode.
So I think my patch isn't that absurd, is it? :)
Cheers,
Olaf
Signature · 12 lines
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play [email protected] | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax ------------------------------------------------------- This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar _______________________________________________ NFS maillist - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
m=C3=A5 den 11.07.2005 Klokka 13:24 (+0200) skreiv Olaf Kirch:
Quoted 2 lines
unfsd definitely does it this way, unfortunately. I agree that it's not a very good idea to continue to use unfsd, but...
I still don't have a good idea about what kind of cache consistency guarantees a client should attempt to offer against a unfsd server.
Quoted 6 lines
I would still vote for merging this patch, as it just does
the same nlink accounting that other functions already do,
egnfs_rmdir,nfs_safe_remove, etc. The latter even has a comment preceding it that says
=20
* We invalidate the attribute cache and free the inode prior to the opera=
tion
Quoted 3 lines
* to avoid possible races if the server reuses the inode. =20
So I think my patch isn't that absurd, is it? :)
...but it should be unnecessary.
d_delete() is supposed to cause the inode to be dropped: if it cannot immediately iput the inode, it will drop the dentry. So unless there exists a hard link or an aliased dentry, the inode should get dropped and freed pretty rapidly.
The only case I can see that might cause a problem is if someone holds onto the dropped dentry for long enough to have a lookup() pick up the inode again. I cannot see how your patch would change matters there.
I also have some trouble seeing why basic checks of mtime and size upon lookup/open would fail to catch most of the races. AFAICS you should almost always get a pagecache invalidation due to that.
Cheers,
Trond
-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Quoted 6 lines · Trond Myklebust
...but it should be unnecessary.
d_delete()is supposed to cause the inode to be dropped: if it cannot immediately iput the inode, it will drop the dentry. So unless there exists a hard link or an aliased dentry, the inode should get dropped and freed pretty rapidly.
AFAIR the problem is that i_nlink doesn't get decremented and therefore the inode doesn't get deleted.
Cheers,
Michael.
Signature · 12 lines
Michael Schroeder [email protected] main(_){while(_=~getchar())putchar(~_-1/(~(_|32)/13*2-11)*13);} ------------------------------------------------------- This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar _______________________________________________ NFS maillist - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs
m=C3=A5 den 11.07.2005 Klokka 14:28 (+0200) skreiv Michael Schroeder:
Quoted 2 lines
AFAIR the problem is that
i_nlinkdoesn't get decremented and therefore the inode doesn't get deleted.
OK. So how about just a one-liner _before_ the call to d_delete()? We don't really care whether or not the rename was successful.
Cheers,
Trond
-------------------------------------------------------
This SF.Net email is sponsored by the 'Do More With Dual!' webinar happening July 14 at 8am PDT/11am EDT. We invite you to explore the latest in dual core and dual graphics technology at this free one hour event hosted by HP, AMD, and NVIDIA. To register visit http://www.hp.com/go/dualwebinar
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs