On Thursday 18 February 2016 01:35:25 Deepa Dinamani wrote:
> On Thu, Feb 18, 2016 at 1:17 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Wednesday 17 February 2016 22:02:11 Deepa Dinamani wrote:
> >>
> >> I checked the server side code.
> >> So the server side also reads whatever is sent over wire as u32 and
> >> then assigns it to a time_t.
> >
> > I think the more important question is whether the server actually
> > does anything with the timestamp other than pass it back to the
> > client. If not, the interpretation it totally up to the client
> > and the type on the server has no meaning (as long as it can store
> > at least as many bits as the wire protocol, and passes all bits
> > back the same way).
>
> Timestamps do get interpreted and written to on the server according to the
> files in the mds directory.
Ok. I see they are stored internally as a __u32/__u32 pair in "class utime_t",
I just couldn't figure out whether this is used on the server at all, or
stored in a timespec or time_t.
> >> Do you have any documentation about the wire protocol?
> >>
> >> To me, the wire protocol seems to have been designed to support only
> >> positive time values(u32).
> >
> > I think assigning to a time_t usually just means that this wasn't
> > completely thought through when it got implemented (which we already
> > know from the fact that it started out using a 32-bit number from
> > the time). It's totally possible that this code originated on a 32-bit
> > machine and was meant to encode a signed number and that the behavior
> > changed without anyone noticing when it got ported to a 64-bit
> > architecture for the first time.
> >
> >> This means ceph can represent times from 1970 - 2106 as long as both
> >> server and client are 64 bit machines.
> >> Or, client is 64 bit and server does not operate on time values.
> >> And, on 32 bit machines implementation will be broken in 2038.
> >
> >> I will change back the above code to not use any casts(as they are a
> >> no-op) as on the server side and note the above
> >> things in the file and changelog:
> >>
> >> static inline void ceph_decode_timespec(struct timespec *ts,
> >> const struct ceph_timespec *tv)
> >> {
> >> - ts->tv_sec = (__kernel_time_t)le32_to_cpu(tv->tv_sec);
> >> - ts->tv_nsec = (long)le32_to_cpu(tv->tv_nsec);
> >> + ts->tv_sec = le32_to_cpu(tv->tv_sec);
> >> + ts->tv_nsec = le32_to_cpu(tv->tv_nsec);
> >> }
> >
> > Ok. I really like to have an explicit range-extension as well, but
> > as you say, your patch above does not change behavior.
>
> You mean change the functions and data types on both sides?
> Not sure if this is meant for me or the Ceph team.
I meant it should be part of your patch.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
