storage/drivers: optimize chowner logic - #1083
Conversation
|
cc @giuseppe |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
What about directories and their st_nlink precisely?
At least s/are known to be hard links/could possibly be/ because a directory (if it represents .. physically) will have more links than subdirectories, so every directory with a subdirectory matches the current condition and is added to inodes.
At the other extreme, such a system with physical .. will not have hard links to directories, because the physical .. entry would be ambiguous. So we should never expect the os.Link to succeed, and we should never expect the Walk to give us the same directory inode twice. So we might assume that directories are not links, and not add them to inodes at all.
But then again, there is macOS, which does physically implement hard links to directories (at least used within backups) so it is not an impossible situation.
… and then there is a somewhat-widespread practice of setting directory st_nlink to 1 (e.g. fuse-overlayfs -o static_nlink, reportedly some network filesystems) when unknown and the like.
We clearly want this optimization for regular files. For directories … meh; I think it’s most important to be clear about what the model / assumptions are.
I hope “directories with link count exactly 2 are clearly not hard links; if it is 1 or > 2, we are not entirely sure” should be quite safe.
I am open to an argument that directory hard links should never exist and we don’t need to check them at all; I’m unsure, but at least the macOS code is using a different implementation anyway.
| // inodes to path map, so the chowner can reconstructed hard links | ||
| // To safe memory only elements that are known to be a hard links are added. |
There was a problem hiding this comment.
| // inodes to path map, so the chowner can reconstructed hard links | |
| // To safe memory only elements that are known to be a hard links are added. | |
| // inodes to path map, so the chowner can reconstruct hard links | |
| // To save memory only elements that are known to be hard links are added. |
| var normalLinkCount uint64 = 1 | ||
| if info.IsDir() { | ||
| // a regular empty directory has at least two links | ||
| normalLinkCount = 2 |
There was a problem hiding this comment.
The code below uses st.Nlink > 1. That shouldn’t be normalLinkCount? See elsewhere.
I think that is reasonable and the easiest thing to do. Our main targets are linux and freebsd for the storage code here and they both do not support directory hardlinks https://man.freebsd.org/cgi/man.cgi?query=ln
|
|
Packit jobs failed. @containers/packit-build please check. |
Currently the storage-chown-by-maps program is used to chown (possibly large) directory trees of images. In order to preserve hardlinks it stores a map inode -> path which is one entry per inode currently thus could consume a lot of memory. Since we only care about hard links we should skip storing paths that are known not to have any links, nlink == 1 for files or any directory. This reduces the map size from number of all inodes to number of inodes that have hard links. In practice that should be a lot less files then. I included some basic benchmarks as well to prove that this helps significantly. To the reduced allocations we also improve the runtime. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
I could have sworn I did, but clearly I did not. Fixed. |
|
Packit jobs failed. @podman-container-tools/packit-jobs please check. |
3 participants
Currently the storage-chown-by-maps program is used to chown (possibly large) directory trees of images. In order to preserve hardlinks it stores a map inode -> path which is one entry per inode currently thus could consume a lot of memory.
Since we only care about hard links we should skip storing paths that are known not to have any links, nlink == 1 for files or any directory. This reduces the map size from number of all inodes to number of inodes that have hard links. In practice that should be a lot less files then.
I included some basic benchmarks as well to prove that this helps significantly. To the reduced allocations we also improve the runtime.
Old:
New: