-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix Lfs to pass usertests #612
Conversation
EDIT: Now, when tested with rustc 2022-03-09 and on release mode, only the following usertests fail and all others seem to always success.
Also, note #613 |
kernel-rs/src/fs/lfs/inode.rs
Outdated
.set(self.inum, disk_block_no, &mut segment, ctx)); | ||
let mut imap = tx.fs.imap(ctx); | ||
assert!(imap.set(self.inum, disk_block_no, &mut segment, ctx)); | ||
imap.free(ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid to free map before committing the segment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will cause any problems, but I changed it anyway just in case and to make things consistent with Itable::alloc_inode
. Note that,
- if you were talking about deadlocks, I don't think it'll cause any problems since when we want to lock both
Segment
andImap
, we always do it in the order ofSegment
->Imap
. - if its about race conditions, then I also think its fine since we already wrote to the imap and
segment.commit()
doesn't modify its content (just reads the buf and writes it to the disk). If we want to modify the imap's content, we need the segment guard anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no problem, isn’t freeing the imap after committing the segment detrimental to the performance in a multi-threaded execution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right and actually, there's a few more places where I decided to be careful (even though it may be unnecessary) rather than aim for better performance. I think we should remove such waste of performance later after we confirmed we removed all deadlocks in lfs.
/// | ||
/// # Note | ||
/// | ||
/// You should make sure the segment has an empty block before calling this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be better to check this inside this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since InodeGuard::{writable_data_block_inner, writable_indirect_block}
is only used inside the InodeGuard::writable_data_block
, and since those two methods must be called always after checking that we have at least two blocks on the segment, I think its better to just panic instead of runtime check inside those functions.
|
|
10c06ca
to
78e7081
Compare
bors r+ without actually reviewing this PR... |
Build succeeded: |
InodeGuard::bmap_internal
. 1acf937SegSumEntry::IndirectMap
. de1c0d3Now, only the following usertests seem to always fail.
We need to implement the segment cleaner to fix this.