| Message ID | 1466382443-11063-22-git-send-email-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 20-6-2016 02:27, Deepa Dinamani wrote: > CURRENT_TIME is not y2038 safe. > The macro will be deleted and all the references to it > will be replaced by ktime_get_* apis. > > struct timespec is also not y2038 safe. > Retain timespec for timestamp representation here as ceph > uses it internally everywhere. > These references will be changed to use struct timespec64 > in a separate patch. > > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> > Cc: "Yan, Zheng" <zyan@redhat.com> > Cc: Sage Weil <sage@redhat.com> > Cc: Ilya Dryomov <idryomov@gmail.com> > Cc: ceph-devel@vger.kernel.org > --- > net/ceph/messenger.c | 6 ++++-- > net/ceph/osd_client.c | 4 ++-- > 2 files changed, 6 insertions(+), 4 deletions(-) Hi Deepa, Although I applaud 64-bit timestamps, the ktime_get_* api seems to be a Linux only API. So IF ever things need to be the least bit portable, I suggest wrapping these routines into a class of itself. So that porting confines itself to only that class. (plus/minus hairy semantic details) For what is now committed the change is not really that big. On the other hand, I 'll have to start figuring out if FreeBSD really does do 64bit timestamps... So I would welcome a more elaborate description of the problem (it seems that Linux is going 64bit in its inodes??) and what is being fixed and how. So that in the porting case not only the code will be the source to figure out what is going on. Thanx, --WjW -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Adding Arnd, as he is the overall y2038 project lead. >> CURRENT_TIME is not y2038 safe. >> The macro will be deleted and all the references to it >> will be replaced by ktime_get_* apis. >> >> struct timespec is also not y2038 safe. >> Retain timespec for timestamp representation here as ceph >> uses it internally everywhere. >> These references will be changed to use struct timespec64 >> in a separate patch. > Hi Deepa, > > Although I applaud 64-bit timestamps, > the ktime_get_* api seems to be a Linux only API. > So IF ever things need to be the least bit portable, I suggest wrapping > these routines into a class of itself. So that porting confines itself > to only that class. (plus/minus hairy semantic details) ktime_get_* apis just return 64 bit timestamps. They are the equivalent of CURRENT_TIME. Did you have a wrapper for CURRENT_TIME? Are these concerns about all of ceph kernel code or just that the libceph is expected to be portable? > For what is now committed the change is not really that big. > > On the other hand, I 'll have to start figuring out if FreeBSD really > does do 64bit timestamps... From what I can see here: http://fxr.watson.org/fxr/ident?im=10;i=__time_t, FreeBSD time_t is already 64 bit long. So it supports 64 bit timestamps already. > So I would welcome a more elaborate description of the problem (it seems > that Linux is going 64bit in its inodes??) and what is being fixed and > how. So that in the porting case not only the code will be the source to > figure out what is going on. Yes, all inode timestamps are going to be switched over to use timespec64 i.e., 64 bit timestamps. We will switch over just the vfs layer: struct inode, struct attr and struct kstat. End filesystems such as ceph will still use timespec at this time. So whenever they access vfs timestamps, they will go through conversion apis: timespec64_to_timespec() / timespec_to_timespec64(). My reference tree is https://github.com/deepa-hub/vfs/tree/y2038 . This gives you an overall idea of the changes. Only a few names have to be updated according to review discussions. But, you can see what the end filesystems will look like after the vfs switch-over. The problem with ceph is really that the timestamps sent over wire use the same encode/ decode function calls whether the timestamp is related to files or not. I'm not sure what the plan for wire protocol is here. As Sage suggested on the thread: http://www.spinics.net/lists/ceph-devel/msg28960.html , you could interpret time values as u32. After this, porting to BSD should be straight forward. Then, you can substitute nanotime() for ktime_get_real_ts64()?: http://fxr.watson.org/fxr/source/kern/kern_tc.c?im=3#L387,938 -Deepa -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22-6-2016 02:56, Deepa Dinamani wrote: > Adding Arnd, as he is the overall y2038 project lead. > >>> CURRENT_TIME is not y2038 safe. >>> The macro will be deleted and all the references to it >>> will be replaced by ktime_get_* apis. >>> >>> struct timespec is also not y2038 safe. >>> Retain timespec for timestamp representation here as ceph >>> uses it internally everywhere. >>> These references will be changed to use struct timespec64 >>> in a separate patch. >> Hi Deepa, >> >> Although I applaud 64-bit timestamps, >> the ktime_get_* api seems to be a Linux only API. >> So IF ever things need to be the least bit portable, I suggest wrapping >> these routines into a class of itself. So that porting confines itself >> to only that class. (plus/minus hairy semantic details) > > ktime_get_* apis just return 64 bit timestamps. > They are the equivalent of CURRENT_TIME. > Did you have a wrapper for CURRENT_TIME? > > Are these concerns about all of ceph kernel code or just that the libceph > is expected to be portable? > >> For what is now committed the change is not really that big. >> >> On the other hand, I 'll have to start figuring out if FreeBSD really >> does do 64bit timestamps... > > From what I can see here: > http://fxr.watson.org/fxr/ident?im=10;i=__time_t, > > FreeBSD time_t is already 64 bit long. > So it supports 64 bit timestamps already. Yup, I look a bit further into it. After your mail I started diging into my memory and realised that 64bit times are quite some time on FreeBSD. And wrote even a small test to be sure... #include <stdio.h> #include <time.h> #include <sys/stat.h> #include <sys/timespec.h> struct stat s; struct itimerspec t; int main () { printf( "Size of __time_t: %lu bytes\n", sizeof(__time_t)); printf( "Size of st_atim: %lu bytes\n", sizeof(s.st_atim)); printf( "Size of timespec: %lu bytes\n", sizeof(t.it_interval)); } [~/Src/c-code] wjw > print_time_t_size Size of __time_t: 8 bytes Size of st_atim: 16 bytes Size of timespec: 16 bytes So FreeBSD is good. > >> So I would welcome a more elaborate description of the problem (it seems >> that Linux is going 64bit in its inodes??) and what is being fixed and >> how. So that in the porting case not only the code will be the source to >> figure out what is going on. > > Yes, all inode timestamps are going to be switched over to use timespec64 i.e., > 64 bit timestamps. > We will switch over just the vfs layer: struct inode, struct attr and > struct kstat. > End filesystems such as ceph will still use timespec at this time. > So whenever they access vfs timestamps, they will go through conversion apis: > timespec64_to_timespec() / timespec_to_timespec64(). > > My reference tree is https://github.com/deepa-hub/vfs/tree/y2038 . > > This gives you an overall idea of the changes. > Only a few names have to be updated according to review discussions. > But, you can see what the end filesystems will look like after the vfs > switch-over. > > The problem with ceph is really that the timestamps sent over wire use the same > encode/ decode function calls whether the timestamp is related to files or not. > I'm not sure what the plan for wire protocol is here. As Sage > suggested on the thread: > http://www.spinics.net/lists/ceph-devel/msg28960.html , you could > interpret time values as u32. I've seen the discussion here but for the time being chose to ignore that, since whatever would come from it > > After this, porting to BSD should be straight forward. > Then, you can substitute nanotime() for ktime_get_real_ts64()?: > http://fxr.watson.org/fxr/source/kern/kern_tc.c?im=3#L387,938 So that is more or less why I suggest to make into a (small) class. Otherwise there are going to be wraped into ifdef's. You could even put it just in a *.h file, and than a good compiler would do the optimisation, and clean-out one call set, and inline. --WjW -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Adding Arnd, as he is the overall y2038 project lead. >> >>>> CURRENT_TIME is not y2038 safe. >>>> The macro will be deleted and all the references to it >>>> will be replaced by ktime_get_* apis. >>>> >>>> struct timespec is also not y2038 safe. >>>> Retain timespec for timestamp representation here as ceph >>>> uses it internally everywhere. >>>> These references will be changed to use struct timespec64 >>>> in a separate patch. >>> Hi Deepa, >>> >>> Although I applaud 64-bit timestamps, >>> the ktime_get_* api seems to be a Linux only API. >>> So IF ever things need to be the least bit portable, I suggest wrapping >>> these routines into a class of itself. So that porting confines itself >>> to only that class. (plus/minus hairy semantic details) >> >> ktime_get_* apis just return 64 bit timestamps. >> They are the equivalent of CURRENT_TIME. >> Did you have a wrapper for CURRENT_TIME? >> >> Are these concerns about all of ceph kernel code or just that the libceph >> is expected to be portable? >> >>> For what is now committed the change is not really that big. >>> >>> On the other hand, I 'll have to start figuring out if FreeBSD really >>> does do 64bit timestamps... >> >> From what I can see here: >> http://fxr.watson.org/fxr/ident?im=10;i=__time_t, >> >> FreeBSD time_t is already 64 bit long. >> So it supports 64 bit timestamps already. > > Yup, I look a bit further into it. After your mail I started diging into > my memory and realised that 64bit times are quite some time on FreeBSD. > And wrote even a small test to be sure... > > #include <stdio.h> > #include <time.h> > #include <sys/stat.h> > #include <sys/timespec.h> > > struct stat s; > struct itimerspec t; > > int main () { > printf( "Size of __time_t: %lu bytes\n", sizeof(__time_t)); > printf( "Size of st_atim: %lu bytes\n", sizeof(s.st_atim)); > printf( "Size of timespec: %lu bytes\n", sizeof(t.it_interval)); > > } > [~/Src/c-code] wjw > print_time_t_size > Size of __time_t: 8 bytes > Size of st_atim: 16 bytes > Size of timespec: 16 bytes > > So FreeBSD is good. > >> >>> So I would welcome a more elaborate description of the problem (it seems >>> that Linux is going 64bit in its inodes??) and what is being fixed and >>> how. So that in the porting case not only the code will be the source to >>> figure out what is going on. >> >> Yes, all inode timestamps are going to be switched over to use timespec64 i.e., >> 64 bit timestamps. >> We will switch over just the vfs layer: struct inode, struct attr and >> struct kstat. >> End filesystems such as ceph will still use timespec at this time. >> So whenever they access vfs timestamps, they will go through conversion apis: >> timespec64_to_timespec() / timespec_to_timespec64(). >> >> My reference tree is https://github.com/deepa-hub/vfs/tree/y2038 . >> >> This gives you an overall idea of the changes. >> Only a few names have to be updated according to review discussions. >> But, you can see what the end filesystems will look like after the vfs >> switch-over. >> >> The problem with ceph is really that the timestamps sent over wire use the same >> encode/ decode function calls whether the timestamp is related to files or not. >> I'm not sure what the plan for wire protocol is here. As Sage >> suggested on the thread: >> http://www.spinics.net/lists/ceph-devel/msg28960.html , you could >> interpret time values as u32. > > I've seen the discussion here but for the time being chose to ignore > that, since whatever would come from it >> >> After this, porting to BSD should be straight forward. >> Then, you can substitute nanotime() for ktime_get_real_ts64()?: >> http://fxr.watson.org/fxr/source/kern/kern_tc.c?im=3#L387,938 > > So that is more or less why I suggest to make into a (small) class. > Otherwise there are going to be wraped into ifdef's. > You could even put it just in a *.h file, and than a good compiler would > do the optimisation, and clean-out one call set, and inline. I'm not sure what you mean by class here. Could you elaborate? ktime apis are not available in the userspace and the changes here are only relevant to linux kernel. -Deepa -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23-6-2016 02:17, Deepa Dinamani wrote: >>> Adding Arnd, as he is the overall y2038 project lead. >>> >>>>> CURRENT_TIME is not y2038 safe. >>>>> The macro will be deleted and all the references to it >>>>> will be replaced by ktime_get_* apis. >>>>> >>>>> struct timespec is also not y2038 safe. >>>>> Retain timespec for timestamp representation here as ceph >>>>> uses it internally everywhere. >>>>> These references will be changed to use struct timespec64 >>>>> in a separate patch. >>>> Hi Deepa, >>>> >>>> Although I applaud 64-bit timestamps, >>>> the ktime_get_* api seems to be a Linux only API. >>>> So IF ever things need to be the least bit portable, I suggest wrapping >>>> these routines into a class of itself. So that porting confines itself >>>> to only that class. (plus/minus hairy semantic details) >>> >>> ktime_get_* apis just return 64 bit timestamps. >>> They are the equivalent of CURRENT_TIME. >>> Did you have a wrapper for CURRENT_TIME? >>> >>> Are these concerns about all of ceph kernel code or just that the libceph >>> is expected to be portable? >>> >>>> For what is now committed the change is not really that big. >>>> >>>> On the other hand, I 'll have to start figuring out if FreeBSD really >>>> does do 64bit timestamps... >>> >>> From what I can see here: >>> http://fxr.watson.org/fxr/ident?im=10;i=__time_t, >>> >>> FreeBSD time_t is already 64 bit long. >>> So it supports 64 bit timestamps already. >> >> Yup, I look a bit further into it. After your mail I started diging into >> my memory and realised that 64bit times are quite some time on FreeBSD. >> And wrote even a small test to be sure... >> >> #include <stdio.h> >> #include <time.h> >> #include <sys/stat.h> >> #include <sys/timespec.h> >> >> struct stat s; >> struct itimerspec t; >> >> int main () { >> printf( "Size of __time_t: %lu bytes\n", sizeof(__time_t)); >> printf( "Size of st_atim: %lu bytes\n", sizeof(s.st_atim)); >> printf( "Size of timespec: %lu bytes\n", sizeof(t.it_interval)); >> >> } >> [~/Src/c-code] wjw > print_time_t_size >> Size of __time_t: 8 bytes >> Size of st_atim: 16 bytes >> Size of timespec: 16 bytes >> >> So FreeBSD is good. >> >>> >>>> So I would welcome a more elaborate description of the problem (it seems >>>> that Linux is going 64bit in its inodes??) and what is being fixed and >>>> how. So that in the porting case not only the code will be the source to >>>> figure out what is going on. >>> >>> Yes, all inode timestamps are going to be switched over to use timespec64 i.e., >>> 64 bit timestamps. >>> We will switch over just the vfs layer: struct inode, struct attr and >>> struct kstat. >>> End filesystems such as ceph will still use timespec at this time. >>> So whenever they access vfs timestamps, they will go through conversion apis: >>> timespec64_to_timespec() / timespec_to_timespec64(). >>> >>> My reference tree is https://github.com/deepa-hub/vfs/tree/y2038 . >>> >>> This gives you an overall idea of the changes. >>> Only a few names have to be updated according to review discussions. >>> But, you can see what the end filesystems will look like after the vfs >>> switch-over. >>> >>> The problem with ceph is really that the timestamps sent over wire use the same >>> encode/ decode function calls whether the timestamp is related to files or not. >>> I'm not sure what the plan for wire protocol is here. As Sage >>> suggested on the thread: >>> http://www.spinics.net/lists/ceph-devel/msg28960.html , you could >>> interpret time values as u32. >> >> I've seen the discussion here but for the time being chose to ignore >> that, since whatever would come from it >>> >>> After this, porting to BSD should be straight forward. >>> Then, you can substitute nanotime() for ktime_get_real_ts64()?: >>> http://fxr.watson.org/fxr/source/kern/kern_tc.c?im=3#L387,938 >> >> So that is more or less why I suggest to make into a (small) class. >> Otherwise there are going to be wraped into ifdef's. >> You could even put it just in a *.h file, and than a good compiler would >> do the optimisation, and clean-out one call set, and inline. > > I'm not sure what you mean by class here. > Could you elaborate? > > ktime apis are not available in the userspace and the changes here are > only relevant to linux kernel. 'mmm, I was certain that I saw the ktime api calls in Ceph code. ktime_get_real_ts() is used in a few of the Ceph-modules to replace CURRENT_TIME. Perhaps that is not really part of the ktime API?? eg.: - osd_req->r_mtime = CURRENT_TIME; + ktime_get_real_ts(&osd_req->r_mtime); Having this wrapped in a separate class or routine would make porting a nobrainer. Other OSes just need to augment this one routine. --WjW -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c index a550289..1825eed 100644 --- a/net/ceph/messenger.c +++ b/net/ceph/messenger.c @@ -1366,8 +1366,9 @@ static void prepare_write_keepalive(struct ceph_connection *con) dout("prepare_write_keepalive %p\n", con); con_out_kvec_reset(con); if (con->peer_features & CEPH_FEATURE_MSGR_KEEPALIVE2) { - struct timespec now = CURRENT_TIME; + struct timespec now; + ktime_get_real_ts(&now); con_out_kvec_add(con, sizeof(tag_keepalive2), &tag_keepalive2); ceph_encode_timespec(&con->out_temp_keepalive2, &now); con_out_kvec_add(con, sizeof(con->out_temp_keepalive2), @@ -3149,8 +3150,9 @@ bool ceph_con_keepalive_expired(struct ceph_connection *con, { if (interval > 0 && (con->peer_features & CEPH_FEATURE_MSGR_KEEPALIVE2)) { - struct timespec now = CURRENT_TIME; + struct timespec now; struct timespec ts; + ktime_get_real_ts(&now); jiffies_to_timespec(interval, &ts); ts = timespec_add(con->last_keepalive_ack, ts); return timespec_compare(&now, &ts) >= 0; diff --git a/net/ceph/osd_client.c b/net/ceph/osd_client.c index 8946959..44eb2d0 100644 --- a/net/ceph/osd_client.c +++ b/net/ceph/osd_client.c @@ -3567,7 +3567,7 @@ ceph_osdc_watch(struct ceph_osd_client *osdc, ceph_oid_copy(&lreq->t.base_oid, oid); ceph_oloc_copy(&lreq->t.base_oloc, oloc); lreq->t.flags = CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK; - lreq->mtime = CURRENT_TIME; + ktime_get_real_ts(&lreq->mtime); lreq->reg_req = alloc_linger_request(lreq); if (!lreq->reg_req) { @@ -3625,7 +3625,7 @@ int ceph_osdc_unwatch(struct ceph_osd_client *osdc, ceph_oid_copy(&req->r_base_oid, &lreq->t.base_oid); ceph_oloc_copy(&req->r_base_oloc, &lreq->t.base_oloc); req->r_flags = CEPH_OSD_FLAG_WRITE | CEPH_OSD_FLAG_ONDISK; - req->r_mtime = CURRENT_TIME; + ktime_get_real_ts(&req->r_mtime); osd_req_op_watch_init(req, 0, lreq->linger_id, CEPH_OSD_WATCH_OP_UNWATCH);
CURRENT_TIME is not y2038 safe. The macro will be deleted and all the references to it will be replaced by ktime_get_* apis. struct timespec is also not y2038 safe. Retain timespec for timestamp representation here as ceph uses it internally everywhere. These references will be changed to use struct timespec64 in a separate patch. Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> Cc: "Yan, Zheng" <zyan@redhat.com> Cc: Sage Weil <sage@redhat.com> Cc: Ilya Dryomov <idryomov@gmail.com> Cc: ceph-devel@vger.kernel.org --- net/ceph/messenger.c | 6 ++++-- net/ceph/osd_client.c | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-)