-
Notifications
You must be signed in to change notification settings - Fork 204
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
storage: update BlobPrefetchRequest fields type from u32 to u64 #349
Conversation
MercyMM
commented
Mar 24, 2022
if size > 0 { | ||
return Err(einval!(format!( | ||
"load_chunks_on_demand: blob_id {:?}, size: {:?} is less than 0", | ||
blob_id, size |
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 supposed to be if size < 0
?
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.
It is a mistakes. I will fix it
blobfs/src/sync_io.rs
Outdated
(size - offset as i64) | ||
)) | ||
})?; | ||
if size < offset { |
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.
what about if size == offset
?
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 BlobPrefetchRequest.len will be 0, storage mod should handle it correctly, let me check to make sure.
blobfs/src/sync_io.rs
Outdated
})?; | ||
if size < offset { | ||
return Err(einval!(format!( | ||
"blobfs: blob {:?} offset {:?} is larger than size {:?}", |
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.
Can you please add "load_chunks_on_demand" in the error message to make it easier to locate where goes wrong?
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.
ok
std::cmp::min(size as u32, u32::MAX - offset as u32), | ||
); | ||
} else { | ||
let _ = cache.reader().prefetch_blob_data_range(offset, size); |
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.
By design the prefetch table is supposed to have (offset, size) < u32::MAX.
Can we leave a warn!
or info!
here about (offset, size) is > u32::MAX?
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.
After this patch, u64 is now by design. This does not affect default behavior. If it does not cause potential bugs, I feel that warn! is useless.
f1abc4a
to
7cf52c0
Compare
LGTM! |
Besides there're clippy warnings that should be fixed. |
In BlobMetaState.get_chunk_index_nocheck, if addr == self.chunks [last].compressed_offset, it will return error. This patch add error log for it Signed-off-by: gexuyang <gexuyang@linux.alibaba.com>
Blobfs relies on the BlobPrefetchRequest structure that field of "offset" and "len" are both u32, the maximum support is 4G. But the maximum blob file will exceed 4G, so change the define of "offset" and "len" to u64. Signed-off-by: gexuyang <gexuyang@linux.alibaba.com>
delete unused debug_assert, and delete unused BlobFs.cfg. Signed-off-by: gexuyang <gexuyang@linux.alibaba.com>