Implement 'lookup count' tracking for `Inode` #221

dannycjones · 2023-04-25T17:08:50Z

Early draft of tracking lookup_count, so that we know when we can drop from the superblock.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

dannycjones · 2023-04-26T10:16:16Z

Some open questions for me:

Are there ways to better test this? (I don't think we can force the Kernel to perform forget(ino, nlookup)).
Should we be doing saturating operations, particularly on subtraction, to avoid underflow errors?

dannycjones · 2023-04-26T16:51:30Z

I think the lookup_count is not accurate. It is not capturing file handles correctly, or if it is - it needs more clear explanation.

jamesbornholt

I think it would be helpful to document our understanding of the rules for the lookup count, maybe as a section in the module comment at the top of inode.rs or fs.rs.

Here's my understanding of it:

anything that in libfuse would call fuse_reply_entry or fuse_reply_create should increment the lookup count
- fuse_reply_entry is valid for link, lookup, mkdir, mknod, and symlink
- fuse_reply_create is valid only for create
for us, we don't implement link, symlink, or create (or mkdir until Implement mkdir #202), so it's just lookup and mknod for now
every entry returned by readdirplus, except . and .., also has its lookup count incremented (which this PR needs to do)
I don't think it's explicitly stated, but lookup counts start at zero -- the kernel can't request an inode until it knows about its existence, and it can't know about its existence until it finds out from one of the operations above, which all increment the count

mountpoint-s3/src/fs.rs

mountpoint-s3/src/inode.rs

jamesbornholt · 2023-04-26T16:53:16Z

mountpoint-s3/src/inode.rs

+                    // Safe to remove, kernel no longer has a reference to it.
+                    trace!(ino, "removing inode from superblock");
+                    inodes.remove(&ino);


Inode is an Arc<InodeInner>, and so the Inode itself might live on even though we removed it from the inode map. Maybe we should add a forgotten flag to InodeInner or something that we can sanity check when returning inodes.

Was there somewhere you had in mind for making this check? I'm not against such a check, but I'm also not clear where we'd do it.

Aside - I think removing the Ino -> Inode mapping as I've written at this point is wrong. There's no reason I have right now to believe other parts of our filesystem implementation won't rely on the Inode being present in this map.

Those implementations would be wrong, which is kinda what I mean about having another way to check. I was thinking something like we'd assert(!inode.forgotten()) any time we're about to return an inode out of inode.rs.

mountpoint-s3/src/inode.rs

jamesbornholt · 2023-04-26T17:06:40Z

I think the lookup_count is not accurate. It is not capturing file handles correctly, or if it is - it needs more clear explanation.

There shouldn't be any direct interaction between file (or directory) handles and lookup count. The trick is that the only way to open a file (directory) handle is to call open (opendir), and the only way to call those is to already know the inode number, which the kernel must have discovered from an earlier call to lookup or readdirplus or mknod.

Another way to say this is that the lookup count is a mechanism for remembering which inodes the kernel knows about. Opening a file/directory handle doesn't affect which inodes the kernel knows about, since it already knew about the one it opened.

Signed-off-by: Daniel Carl Jones <[email protected]>

jamesbornholt · 2023-04-28T21:52:26Z

mountpoint-s3/src/fs.rs

@@ -1,3 +1,31 @@
+//! File system types and operations, not relying on our FUSE library.


general nit on this comment: we've been roughly trying to wrap at at 120 chars wherever we can (which is what our rustfmt config is set to, but it doesn't affect comments).

jamesbornholt · 2023-04-28T21:52:52Z

mountpoint-s3/src/fs.rs

+//! File system types and operations, not relying on our FUSE library.
+//!
+//! **Note:** this abstraction is not intended to abstract FUSE itself,
+//! only allow us to run tests without FUSE in the loop.


Suggested change

//! only allow us to run tests without FUSE in the loop.

//! only allow us to run tests without a real FUSE device in the loop.

jamesbornholt · 2023-04-28T21:53:26Z

mountpoint-s3/src/fs.rs

+//! #### Counting references to [Inode]
+//!
+//! One of the responsibilities of [S3Filesystem] is to track the _lookup count_.
+//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the FUSE driver,


Suggested change

//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the FUSE driver,

//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the kernel,

jamesbornholt · 2023-04-28T21:53:47Z

mountpoint-s3/src/fs.rs

+//!
+//! One of the responsibilities of [S3Filesystem] is to track the _lookup count_.
+//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the FUSE driver,
+//! ensuring that we never delete an [Inode] that may be references in the future by the FUSE driver.


Suggested change

//! ensuring that we never delete an [Inode] that may be references in the future by the FUSE driver.

//! ensuring that we never forget about an [InodeNo] that may be referenced in the future by the kernel.

jamesbornholt · 2023-04-28T21:53:59Z

mountpoint-s3/src/fs.rs

+//! One of the responsibilities of [S3Filesystem] is to track the _lookup count_.
+//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the FUSE driver,
+//! ensuring that we never delete an [Inode] that may be references in the future by the FUSE driver.
+//! Note - this should not be confused with any counting of references to an [Inode] from its parent or a [FileHandle].


Suggested change

//! Note - this should not be confused with any counting of references to an [Inode] from its parent or a [FileHandle].

//! This should not be confused with any counting of references to an [Inode] from its parent or a [FileHandle].

jamesbornholt · 2023-04-28T21:55:09Z

mountpoint-s3/src/fs.rs

+//! Some operations will not return a new [InodeNo] but rather one the FUSE driver already knows about.
+//! `open` is one such case since FUSE already provides the [InodeNo] as an argument when it calls open.


This isn't quite the intent -- any operation that takes an InodeNo as input doesn't need to change the lookup count of that InodeNo.

jamesbornholt · 2023-04-28T21:55:56Z

mountpoint-s3/src/fs.rs

    pub async fn readdir<R: DirectoryReplier>(
        &self,
        parent: InodeNo,
        fh: u64,
        offset: i64,
+        plus: bool,


Suggested change

plus: bool,

inc_lookup_count: bool,

jamesbornholt · 2023-04-28T21:56:31Z

mountpoint-s3/src/fuse.rs

+//! `fuser`-specific bindings, mostly wrapping the generic filesystem code in [crate::fs].
+//!
+//! Note that the purpose of this module is not necessarily to abstract away FUSE from a FileSystem,
+//! but instead to provide a thin wrapper between the `fuser` library and our filesystem handlers.


for testing purposes

jamesbornholt · 2023-04-28T21:56:53Z

mountpoint-s3/src/inode.rs

@@ -39,6 +39,8 @@ use crate::sync::{Arc, Mutex, RwLock};
 pub type InodeNo = u64;

 pub const ROOT_INODE_NO: InodeNo = 1;
+/// Starts at zero, as it tracks references Kernel has to the [Inode]. It's zero until returned in a FUSE reply.
+const DEFAULT_LOOKUP_COUNT: u64 = 0;


Suggested change

const DEFAULT_LOOKUP_COUNT: u64 = 0;

const INITIAL_LOOKUP_COUNT: u64 = 0;

jamesbornholt · 2023-04-28T21:57:16Z

mountpoint-s3/src/inode.rs

+            }
+            Some(inode) => {
+                let lookup_count = inode.dec_lookup_count(n);
+                if lookup_count < 1 {


it's unsigned, so

Suggested change

if lookup_count < 1 {

if lookup_count == 0 {

sauraank · 2023-05-31T14:01:05Z

mountpoint-s3/src/inode.rs

@@ -97,6 +100,30 @@ impl Superblock {
        Self { inner: Arc::new(inner) }
    }

+    /// The kernel tells us when it removes a reference to an [InodeNo] from its internal caches via a forget call.


Why were we not implementing forget earlier? Is it due to kernel caching?

It wasn't a priority for us - during alpha, it did not feel essential to cleanup the superblock list of Inodes.

Caching doesn't come into why we're implementing forget I don't think, but it may (IDK) influence when the kernel is telling us its forgotten about a particular Inode.

passaro · 2023-07-06T13:45:11Z

Superseded by #359

dannycjones temporarily deployed to PR integration tests April 25, 2023 17:08 — with GitHub Actions Inactive

dannycjones mentioned this pull request Apr 25, 2023

Implement unlink operation #214

Closed

dannycjones changed the title ~~Implement 'lookup_count` tracking for Inode~~ Implement 'lookup count' tracking for Inode Apr 25, 2023

dannycjones force-pushed the lookup-counting branch from 75873b8 to f26992d Compare April 26, 2023 10:11

dannycjones temporarily deployed to PR integration tests April 26, 2023 10:11 — with GitHub Actions Inactive

dannycjones marked this pull request as ready for review April 26, 2023 10:11

dannycjones requested review from jamesbornholt and passaro April 26, 2023 10:11

dannycjones marked this pull request as draft April 26, 2023 16:51

dannycjones removed the request for review from jamesbornholt April 26, 2023 16:51

jamesbornholt reviewed Apr 26, 2023

View reviewed changes

dannycjones had a problem deploying to PR integration tests April 28, 2023 10:31 — with GitHub Actions Failure

dannycjones added 3 commits April 28, 2023 11:44

Add lookup_count to Inode

2bf46e7

Signed-off-by: Daniel Carl Jones <[email protected]>

Update based on feedback

6333823

Signed-off-by: Daniel Carl Jones <[email protected]>

Plumb in lookup counting for readdirplus in an ugly way

f9407bb

Signed-off-by: Daniel Carl Jones <[email protected]>

dannycjones force-pushed the lookup-counting branch from fe62ac3 to 6321a53 Compare April 28, 2023 12:34

dannycjones had a problem deploying to PR integration tests April 28, 2023 12:34 — with GitHub Actions Failure

Add module comments for mountpoint-s3::fs and mountpoint-s3::fuse

82a12ae

Signed-off-by: Daniel Carl Jones <[email protected]>

dannycjones force-pushed the lookup-counting branch from 6321a53 to 82a12ae Compare April 28, 2023 12:43

dannycjones had a problem deploying to PR integration tests April 28, 2023 12:43 — with GitHub Actions Failure

Appease rustfmt and clippy

e2827fc

Signed-off-by: Daniel Carl Jones <[email protected]>

dannycjones temporarily deployed to PR integration tests April 28, 2023 12:50 — with GitHub Actions Inactive

jamesbornholt reviewed Apr 28, 2023

View reviewed changes

sauraank reviewed May 31, 2023

View reviewed changes

dannycjones self-assigned this May 31, 2023

dannycjones mentioned this pull request Jun 12, 2023

Implement full lifecycle of inodes (forget) #277

Closed

passaro closed this Jul 6, 2023

dannycjones deleted the lookup-counting branch July 19, 2023 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement 'lookup count' tracking for `Inode` #221

Implement 'lookup count' tracking for `Inode` #221

dannycjones commented Apr 25, 2023

dannycjones commented Apr 26, 2023

dannycjones commented Apr 26, 2023

jamesbornholt left a comment

jamesbornholt Apr 26, 2023

dannycjones Apr 28, 2023 •

edited

Loading

dannycjones Apr 28, 2023 •

edited

Loading

jamesbornholt Apr 28, 2023

jamesbornholt commented Apr 26, 2023 •

edited

Loading

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

jamesbornholt Apr 28, 2023

sauraank May 31, 2023

dannycjones May 31, 2023

passaro commented Jul 6, 2023

		@@ -1,3 +1,31 @@
		//! File system types and operations, not relying on our FUSE library.

	//! only allow us to run tests without FUSE in the loop.
	//! only allow us to run tests without a real FUSE device in the loop.

	//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the FUSE driver,
	//! Lookup count is a FUSE concept and tracks the number of times we provide a reference to an [InodeNo] to the kernel,

	//! ensuring that we never delete an [Inode] that may be references in the future by the FUSE driver.
	//! ensuring that we never forget about an [InodeNo] that may be referenced in the future by the kernel.

	//! Note - this should not be confused with any counting of references to an [Inode] from its parent or a [FileHandle].
	//! This should not be confused with any counting of references to an [Inode] from its parent or a [FileHandle].

		//! Some operations will not return a new [InodeNo] but rather one the FUSE driver already knows about.
		//! `open` is one such case since FUSE already provides the [InodeNo] as an argument when it calls open.

	const DEFAULT_LOOKUP_COUNT: u64 = 0;
	const INITIAL_LOOKUP_COUNT: u64 = 0;

Implement 'lookup count' tracking for Inode #221

Implement 'lookup count' tracking for Inode #221

Conversation

dannycjones commented Apr 25, 2023

dannycjones commented Apr 26, 2023

dannycjones commented Apr 26, 2023

jamesbornholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannycjones Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

dannycjones Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbornholt commented Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

passaro commented Jul 6, 2023

Implement 'lookup count' tracking for `Inode` #221

Implement 'lookup count' tracking for `Inode` #221

dannycjones Apr 28, 2023 •

edited

Loading

dannycjones Apr 28, 2023 •

edited

Loading

jamesbornholt commented Apr 26, 2023 •

edited

Loading