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

implement stable unique inode for passthroughfs #131

Merged
merged 1 commit into from
May 12, 2023

Conversation

zyfjeff
Copy link
Contributor

@zyfjeff zyfjeff commented May 9, 2023

In the current passthroughfs implementation, temporary inodes are allocated and then stored in memory, But a FORGET request causes the inode to be removed from memory, new inodes are reassigned, resulting in the instability of the inodes of a file. this PR implements a stable inode, Limited by the current implementation limitations of VFS, Host inode + mnt + dev needs to be controlled in 56bit, and it is not supported beyond the range.

@zyfjeff zyfjeff changed the title implement stable unique inode for passthroughfs Wip: implement stable unique inode for passthroughfs May 9, 2023
@zyfjeff zyfjeff force-pushed the fix-unique-inode branch 4 times, most recently from 1d6ee78 to 8b6c7b2 Compare May 9, 2023 15:18
@zyfjeff zyfjeff changed the title Wip: implement stable unique inode for passthroughfs implement stable unique inode for passthroughfs May 9, 2023
src/passthrough/mod.rs Outdated Show resolved Hide resolved
};

if unique_inode.ino > MAX_HOST_INO {
if unique_inode.ino > (MAX_HOST_INO * 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In case that unique_inode.ino is bigger than MAX_HOST_INO, we need another allocator for ino, just like UniqueInodeGenerator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@zyfjeff
Copy link
Contributor Author

zyfjeff commented May 10, 2023

Forget does not delete the internal inode state, in this way can also ensure a stable inode, but it requires some memory overhead, 100w different files occupy 60M

// safe to unwrap, inode must be in data map if found by ids, otherwise unwrap on
// corruption.
self.inode_by_ids(ids).map(|inode| self.get(inode).unwrap())
match self.inode_by_ids(ids) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be simplified as

let inode = self.inode_by_ids(ids)?;
self.get(inode)

Copy link
Contributor

Choose a reason for hiding this comment

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

using map?

self.inode_by_ids(ids).map(|id| self.get(id))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.inode_by_ids(ids).map(|id| self.get(id)) don't work, self.get(id) return a another Option, not a value

// safe to unwrap, inode must be in data map if found by ids, otherwise unwrap on
// corruption.
self.inode_by_ids(ids).map(|inode| self.get(inode).unwrap())
match self.inode_by_ids(ids) {
Copy link
Contributor

Choose a reason for hiding this comment

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

using map?

self.inode_by_ids(ids).map(|id| self.get(id))

/// it should be noted that the returned host inode will be stored in the lower 47bits,
/// and for inodes over 47bits, virtual inode will be used instead.
/// The default value for this option is `false`.
pub use_host_ino: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to control this behavior when it has known issues to return the host inode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default implementation is the same as before, allocating a virtual inode at a time, but now when forgetting, it is not removed from memory to ensure consistency. The second way is to use host inode to encode, mainly because the internal hot upgrade will be more complicated under the two inode encoding methods, so the default implementation is retained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use_host_ino and libfuse's use_ino options are similar
https://manpages.debian.org/testing/fuse/mount.fuse.8.en.html#use_ino

In the current passthroughfs implementation, temporary inodes are allocated and then stored in memory,
But a FORGET request causes the inode to be removed from memory, new anodes are reassigned,
resulting in the instability of the inodes of a file. this PR implements a stable inode,
Limited by the current implementation limitations of VFS, Host inode + mnt + dev needs to be controlled in 56bit,
and it is not supported beyond the range.

Signed-off-by: zyfjeff <[email protected]>
Copy link
Contributor

@bergwolf bergwolf left a comment

Choose a reason for hiding this comment

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

/lgtm

@bergwolf bergwolf merged commit 70dc263 into cloud-hypervisor:master May 12, 2023
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