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

common/interval_set: return int64_t for size() #12898

Merged
merged 1 commit into from Apr 21, 2017

Conversation

Projects
None yet
2 participants
@XinzeChi
Member

XinzeChi commented Jan 12, 2017

Signed-off-by: Xinze Chi xinze@xsky.com

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 12, 2017

@XinzeChi what problem does this change address? or it's just a cosmetic change?

@tchaikov tchaikov added the common label Jan 12, 2017

@XinzeChi

This comment has been minimized.

Member

XinzeChi commented Jan 12, 2017

@tchaikov, _size is declared as int64_t

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jan 12, 2017

@XinzeChi i see, but the same question still holds. so it's just for the sake of consistency? i wanted to know if this really fixes a problem, if yes, we need to evaluate it to see the impact of the bug.

@XinzeChi

This comment has been minimized.

Member

XinzeChi commented Jan 12, 2017

if the interval_set size is bigger than 2G, the size() would return wrong result. Such as if ceph stores some file which size of bigger than 2G.

common/interval_set: return int64_t for size()
Signed-off-by: Xinze Chi <xinze@xsky.com>
@@ -239,7 +239,7 @@ class btree_interval_set {
return _size == other._size && m == other.m;
}
int size() const {
int64_t size() const {

This comment has been minimized.

@tchaikov

tchaikov Jan 16, 2017

Contributor

@XinzeChi could you name some callers of size() which are impacted by this issue?

This comment has been minimized.

@XinzeChi

XinzeChi Jan 16, 2017

Member

Because intervet_set is very basic class, some one may hit this bug. Maybe there is no callers in osd which could be impacted by this issue. I am not familiar with mds.
I grep the source code of mds. There are some callers in mds, such as inos.size() .

@tchaikov tchaikov self-assigned this Feb 6, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 10, 2017

i tried to find the place where the type of size() matters. but failed. so i don't think it's a bug fix.

@tchaikov tchaikov added the needs-qa label Apr 10, 2017

@XinzeChi

This comment has been minimized.

Member

XinzeChi commented Apr 10, 2017

Hmm, it may be a potential bug.

@tchaikov

This comment has been minimized.

@tchaikov tchaikov merged commit c67bb9c into ceph:master Apr 21, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment