-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
df: switch from u64 to u128 to handle fs with large inodes nr #6071
df: switch from u64 to u128 to handle fs with large inodes nr #6071
Conversation
eaab79c
to
68679ea
Compare
GNU testsuite comparison:
|
68679ea
to
29844af
Compare
GNU testsuite comparison:
|
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.
The u128
change is fine I think. I guess it's not compatible, but it feels correct. Having to deal with overflow here seems a bit like overflow though. I'd say that's an issue that we postpone for when someone actually runs into a real use case where u128
overflows. But if you have some use case for it we can discuss that.
src/uu/df/src/table.rs
Outdated
type InodesIntT = u128; | ||
type MaybeInodesT = Option<InodesIntT>; |
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 appreciate the idea here, but I think it obfuscates the exact data type too much. Id rather just see Option<u128>
everywhere than MaybeInodesT
because then I actually know what I'm dealing with.
bd87899
to
018758c
Compare
GNU testsuite comparison:
|
667f5c3
to
eed7a53
Compare
eed7a53
to
6fa2c58
Compare
GNU testsuite comparison:
|
addresses #6070
changes:
u128
for accumulating inodesuseOption<>
additionally to handle overflows correctlyI know that u128 is slower than u64 as its probably not natively supported, but we are not accumulating millions of numbers such that it should not be significant.
ADDITION: