Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: fix lost hardlink #413

Merged
merged 1 commit into from
May 6, 2022

Conversation

imeoer
Copy link
Collaborator

@imeoer imeoer commented Apr 29, 2022

The merge operation in builder (nydus-image) use RafsSuper::walk_inodes
method to walkthrough the whole filesystem inodes by DFS order, but it
does not call the callback for the hardlink pair, which causes some hardlink
files to be lost in the final bootstrap after merge.

The root cause is that the RafsSuper::get_inode method always gets only
the RafsInode of a particular file name in the hardlink pair because the ino
number of the hardlink pair are the same.

Fix by passing RafsInode trait object in RafsSuper::walk_inodes directly.

Signed-off-by: Yan Song imeoer@linux.alibaba.com

Copy link
Collaborator

@liubogithub liubogithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be an important fix, any test case to keep it from happening in the future?

Copy link
Collaborator

@liubogithub liubogithub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I am wrong.

walk_inodes is only used to getting chunk info from an inode, but hardlink inode pair should share the same chunk infos, so how are lost hardlink files related to walk_inodes?

@imeoer
Copy link
Collaborator Author

imeoer commented Apr 29, 2022

It seems to be an important fix, any test case to keep it from happening in the future?

@liubogithub Will add a test case to the converter package since walk_inodes is currently used mainly for tar build.

@imeoer
Copy link
Collaborator Author

imeoer commented Apr 29, 2022

walk_inodes is only used to getting chunk info from an inode, but hardlink inode pair should share the same chunk infos, so how are lost hardlink files related to walk_inodes?

@liubogithub Not true, walk_inodes is used to get all inodes in src/bin/nydus-image/merge.rs:130.

For example, if there is a hardlink pair file-1, file-2 and file-3, the previous walk_inodes implementation will callback file-1 three times, which causes the merge operation to lose file-2, file-3.

rafs/src/metadata/mod.rs Outdated Show resolved Hide resolved
@imeoer imeoer force-pushed the builder-fix-lost-hardlink branch from a7d8545 to 109fc72 Compare May 6, 2022 06:23
The merge operation in builder (nydus-image) use `RafsSuper::walk_inodes`
method to walkthrough the whole filesystem inodes by DFS order, but it
does not call the callback for the hardlink pair, which causes some hardlink
files to be lost in the final bootstrap after merge.

The root cause is that the `RafsSuper::get_inode` method always gets only
the RafsInode of a particular file name in the hardlink pair because the ino
numbers of the hardlink pair are the same.

Fix by passing `RafsInode` trait object in `RafsSuper::walk_inodes` directly.

Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
@imeoer imeoer force-pushed the builder-fix-lost-hardlink branch from 109fc72 to ad778db Compare May 6, 2022 06:26
@bergwolf bergwolf merged commit ae79d5a into dragonflyoss:master May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants