utime： fix __32u sec time overflow #21113
Conversation
|
@liewegas need a review please |
src/osd/OSD.cc
Outdated
| @@ -6911,10 +6911,18 @@ OSDService::ScrubJob::ScrubJob(CephContext* cct, | |||
| double r = rand() / (double)RAND_MAX; | |||
| sched_time += | |||
| scrub_min_interval * cct->_conf->osd_scrub_interval_randomize_ratio * r; | |||
| if (sched_time < timestamp) { | |||
There was a problem hiding this comment.
can we update the implementation of utime_t& operator+=(utime_t& l, double f) instead? so if the result will overflow, we can just assign std::limits<uint32_t>::max() to it?
There was a problem hiding this comment.
right, file changed, help review,please
src/include/utime.h
Outdated
| @@ -47,6 +47,11 @@ class utime_t { | |||
| bool is_zero() const { | |||
| return (tv.tv_sec == 0) && (tv.tv_nsec == 0); | |||
| } | |||
|
|
|||
| void set_max() { | |||
| tv.tv_sec = UINT32_MAX; | |||
There was a problem hiding this comment.
might want to use std::limits<decltype(tv.tv_sec)>::max() instead.
kungf
force-pushed
the
scrub_interval_overflow
branch
from
7e07114
to
36eb583
Compare
kungf
changed the title
|
Jenkins retest this please |
|
@tchaikov need a review, please |
src/include/utime.h
Outdated
| (l.nsec()+r.nsec())%1000000000L ); | ||
| __u64 sec = (__u64)l.sec() + r.sec() + (l.nsec()+r.nsec())/1000000000ul; | ||
| __u64 nsec = (l.nsec()+r.nsec())%1000000000ul; | ||
| return utime_t( ut_safe_convert(sec), ut_safe_convert(nsec)); |
There was a problem hiding this comment.
is the second ut_safe_convert() necessary?
remove the space after (.
why are you using different ways to normalize the result in operator+ and operator+=? could you be more consistent ?
src/include/utime.h
Outdated
| @@ -32,6 +32,10 @@ | |||
| // -------- | |||
| // utime_t | |||
|
|
|||
| //from __u64 to __u32 | |||
| inline __u32 ut_safe_convert(__u64 t) { | |||
| return t < std::numeric_limits<uint32_t>::max() ? t : std::numeric_limits<uint32_t>::max(); | |||
There was a problem hiding this comment.
i'd suggest rename ut_safe_convert() to cap_to_u32_max(). also might want to use std::min() here.
kungf
force-pushed
the
scrub_interval_overflow
branch
from
36eb583
to
4216841
Compare
|
6307200000 is around 200 years - nobody sane is going to set it so high intentionally. While I agree on the fix, I think it's the osd_scrub_min/max_interval that should be capped to reasonable boundaries. |
|
@branch-predictor some one who want close scrub may set it, he may not care how may years he set it, but only want set a very large num, then close the scrub through this way. what do you mean? only cap osd_scrub_min/max_interval in osd.cc? |
|
@kungf to disable scrub, you can use noscrub/nodeepscrub cluster flags.
No, add appriopriate |
src/include/utime.h
Outdated
| @@ -32,6 +32,10 @@ | |||
| // -------- | |||
| // utime_t | |||
|
|
|||
| //from __u64 to __u32 | |||
There was a problem hiding this comment.
@kungf could you drop this comment. it's unnecessary and redundant here.
Signed-off-by: kungf <yang.wang@easystack.cn>
kungf
force-pushed
the
scrub_interval_overflow
branch
from
4216841
to
dd2ee8d
Compare
|
@branch-predictor sorry, i missed your comment. yeah, a min-max bound would be better. |
when i set osd_scrub_min(max)_interval to a very large num, just like 6307200000(20036524*3600), osd still do scrub. finally i find that the utime_t::nsec only __u32, it's max num is 4294967295, and <
6307200000, so scrub sched_time will overflow, and it will be a random time.
related: #20965
Signed-off-by: kungf yang.wang@easystack.cn