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

vcache #1153

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

vcache #1153

wants to merge 19 commits into from

Conversation

mohrcore
Copy link
Collaborator

We need basic vcache functionality to support any external filesystem.

@mohrcore mohrcore added the WiP not ready for code review label Jun 11, 2021
@mohrcore mohrcore added review please review this PR and removed WiP not ready for code review labels Aug 8, 2021
Copy link
Collaborator

@pj1031999 pj1031999 left a comment

Choose a reason for hiding this comment

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

Please resolve conflicts.

@mohrcore mohrcore added WiP not ready for code review and removed review please review this PR labels Jan 4, 2022
@mohrcore
Copy link
Collaborator Author

mohrcore commented Jan 4, 2022

No idea on why the vmem test fails. It strikes me as very bizarre. Is the pool allocator broken, or am I just doing something wrong?

@pj1031999
Copy link
Collaborator

@mohrcore value of VF_CACHED in your code is 0. The only place where you are using this flag calls pool allocator (and is always called). When you change VF_CACHED to 1 then your code will be working better.

Splitted the vnode cleanup into two VOPs (VOP_INACTIVE, VOP_RECLAIM). VOP_RECLAIM should be used only for vcached filesystems.
Renamed some vcache procedures.
vfs_vcache_return (earlier vfs_vcache_put) distinguishes now between detached an attached vnodes.
@mohrcore
Copy link
Collaborator Author

@pj1031999 thank you for you comment. That was certainly causing some issues. I've fixed that now.

@mohrcore
Copy link
Collaborator Author

Allow me to explain my reasoning behind adding a new VOP:

A vnode use count as I get it represents the number of entities that need direct access to data held by a vnode. A use count greater than zero, therefore implies that the vnode is not recycleable at the moment (ie. can't be on vcache freelist). When the use count drops to zero, the vnode should be moved to a vcache freelist and thus become recycleable. This will be done through a vfs_vcache_return procedure called when a vnode with VF_CACHED flag set gets its use counter dropped to zero. Right before that happens it's be crucial for a filesytem code to take action if a vnode is no longer valid, ie. it's corresponding inode is to be removed from a filesystem (eg. user has deleted a file). The filesystem code should detach any of its data from a vnode to prevent vcache from returning an outdated vnode on request that uses a new inode with the same number as the old one. This action can be done using vfs_vcache_detach procedure, which will mark the node "detached" and will prevent it from being put into any bucket within vcache in vfs_vcache_return. Such action could've been performed inside VOP_RECLAIM that used to be called when vnode usecount dropped to zero.

The problem is that it was the last point at which a filesystem code could do anything in regards to a vnode, before it gets recycled. That meant that any inodes that were allocated by a filesystem needed to be freed in that moment, which in regards to filesystems present on block devices makes the vcache impractical. (I guess alternatively they could be cached internally by a filesystem) A filesystem would need to flush an inode to a block device every time the use count drops to zero and load it back the moment the vnode goes back into use. Thus VOP_RECLAIM is now called only when the vcache system needs to recycle the vnode for use with another filesystem, or when the filesystem chooses to detach its data from a vnode. A vnode is expected to have its usecnt equal to 0 at this point. In VOP_RECLAIM a filesystem is required to ensure that all the resources it has associated to the vnode will be in correct state once the vnode is gone, so they could be attached later to another vnode if needed. For tmpfs it just means a little cleanup work (removing an inode if a file is expected to be removed). For something like ext2 it could mean flushing the inode to its block device (and given that block buffering will most likely require a vnode in order to identify a block, this could also mean flushing and de-buffering the entire data associated with that vnode).

Because it's still necessary to take action the moment vnode use count drops to zero, as described eariler (detaching filesystem data from invalid vnode), the old VOP_RECLAIMbecame VOP_INACTIVE.

According to NetBSD manual:

"VOP_INACTIVE() is called when the kernel is no longer using the vnode."
"VOP_RECLAIM() is called when a vnode is being reused for a different file system."

@cahirwpz what are your thoughts on this?

@mohrcore
Copy link
Collaborator Author

vmem test still fails, but I'm unable to reproduce this failure locally. It seems to be some rare heizenbug. I'm unfamiliar with vmem at this point, so I have no clue what is a potential cause (other than a race condidtion I might be overlooking).

@mohrcore
Copy link
Collaborator Author

mohrcore commented Jan 25, 2022

Changing the pool_alloc to kmalloc seems to solve the issue. I re-ran the entire testing pipeline three times with no failures after that small change.

My take is that the pool allocator is broken, or there's some limitation to its use that I'm not aware of. Or the vmem test is broken (I don't get what it does tho). Feel free to call it bs, but I don't see another explanation atm.

@mohrcore mohrcore added review please review this PR and removed WiP not ready for code review labels Jan 26, 2022
@mohrcore mohrcore requested a review from pj1031999 January 26, 2022 18:07
Comment on lines 54 to 56
typedef int vnode_ioctl_t(vnode_t *v, u_long cmd, void *data, file_t *fp);
typedef int vnode_inactive(vnode_t *v);
typedef int vnode_reclaim_t(vnode_t *v);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inconsistent naming schema.

Comment on lines +636 to 640
.v_inactive = tmpfs_vop_inactive,
.v_reclaim = tmpfs_vop_reclaim,
.v_readlink = tmpfs_vop_readlink,
.v_symlink = tmpfs_vop_symlink,
.v_link = tmpfs_vop_link};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add , after last struct member - then shitty clang-format will be happy and it will produce human readable output :)

Comment on lines -1005 to +1008
mtx_lock(&v->tfn_timelock);
mtx_lock(&node->tfn_timelock);
if (atime->tv_sec != VNOVAL)
v->tfn_atime = *atime;
node->tfn_atime = *atime;
if (mtime->tv_sec != VNOVAL)
v->tfn_mtime = *mtime;
mtx_unlock(&v->tfn_timelock);
node->tfn_mtime = *mtime;
mtx_unlock(&node->tfn_timelock);
Copy link
Collaborator

Choose a reason for hiding this comment

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

WITH_MTX_LOCK

Comment on lines +30 to +32
static vcache_t vcache_hash(mount_t *mp, ino_t ino) {
return (((vcache_t)mp->mnt_vnodecovered >> 3) + ino) % VCACHE_BUCKET_CNT;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to convince me that it's a good hash function

Comment on lines +107 to +111
vnode_t *vfs_vcache_reborrow(mount_t *mp, ino_t ino) {
SCOPED_MTX_LOCK(&vcache_giant_lock);

return vcache_hashget(mp, ino);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's the only place that uses vcache_hashget in code. Why they are separated functions?

Comment on lines +135 to +136
error = vfs_vcache_detach(vn);
assert(error == 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a purpose of this assertion? Why you don't propagate error?

@cahirwpz cahirwpz removed the review please review this PR label Jun 20, 2022
@cahirwpz cahirwpz added the orphaned needs new owner label Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
orphaned needs new owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants