On Friday 19 February 2016 12:42:02 Ilya Dryomov wrote:
> On Fri, Feb 19, 2016 at 11:00 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Thursday 18 February 2016 18:22:55 Ilya Dryomov wrote:
> >> On Thu, Feb 18, 2016 at 5:20 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >> > On Thursday 18 February 2016 17:02:25 Ilya Dryomov wrote:
> >> >>
> >> >> If we are going to touch this function at all, let's drop all casts and
> >> >> just add a comment along the lines of "the lack of cast/sign-extension
> >> >> here is intentional, this function has a counterpart on the server side
> >> >> which also lacks cast/sign-extension, etc". To someone who is not up
> >> >> on these subtleties, the __u64 cast is going to be more confusing than
> >> >> explicit...
> >> >
> >> > We certainly a comment, I was just suggesting to add the cast as well.
> >> >
> >> > Again, it's not the lack of the cast on the server that is important
> >> > here, it's whether it gets interpreted as signed or unsigned at the
> >> > point where the timestamp is actually used for comparison or
> >> > printing. Let me suggest a comment here:
> >>
> >> I'd argue it's important to have encode/decode code look the same.
> >>
> >> I mentioned before that at least in some places it's interpreted as
> >> signed and that the interpretation is not totally up to the client.
> >> The printing (at least some of it) is right there in the same file
> >> (src/include/utime.h) and you can see ... + 1900.
> >
> > I see this code:
> >
> > class utime_t {
> > struct {
> > __u32 tv_sec, tv_nsec;
> > } tv;
> >
> > time_t sec() const { return tv.tv_sec; }
> >
> > ostream& gmtime(ostream& out) const {
> > out.setf(std::ios::right);
> > char oldfill = out.fill();
> > out.fill('0');
> > if (sec() < ((time_t)(60*60*24*365*10))) {
> > // raw seconds. this looks like a relative time.
> > out << (long)sec() << "." << std::setw(6) << usec();
> > } else {
> > // localtime. this looks like an absolute time.
> > // aim for http://en.wikipedia.org/wiki/ISO_8601
> > struct tm bdt;
> > time_t tt = sec();
> > gmtime_r(&tt, &bdt);
> > out << std::setw(4) << (bdt.tm_year+1900) // 2007 -> '07'
> > << '-' << std::setw(2) << (bdt.tm_mon+1)
> > << '-' << std::setw(2) << bdt.tm_mday
> > << ' '
> > << std::setw(2) << bdt.tm_hour
> > << ':' << std::setw(2) << bdt.tm_min
> > << ':' << std::setw(2) << bdt.tm_sec;
> > out << "." << std::setw(6) << usec();
> > out << "Z";
> > }
> > out.fill(oldfill);
> > out.unsetf(std::ios::right);
> > return out;
> > }
> >
> > which interprets the time roughly in the way I explained:
> >
> > * On 64-bit architectures, the time is interpreted as an unsigned
> > number in the range from 1980-2106, while any times between 1970
> > and 1980 are interpreted as a relative time and printed as
> > a positive seconds number.
> >
> > * On 32-bit architectures, times in the range 1980-2038 are printed
> > as a date like 64-bit does, times from 1970-1980 are also printed
> > as a positive seconds number, and anything with the top bit set
> > is printed as a negative seconds number.
> >
> > It this the intended behavior? I guess we could change the kernel
> > to reject any timestamps before 1980 and after 2038 in
> > ceph_decode_timespec and just set the Linux timestamp to (time_t)0
> > (Jan 1 1970) in that case, and document that there is no valid
> > interpretation for those.
>
> At least conceptually, this isn't a bad idea, as only 1970(1980)..2038
> works on both architectures; everything else is likely to break
> something in ceph either on 32-bit or on both, I think. But, I'm not
> sure if rejecting by setting to 0 is going to work all around and we
> can't change just the kernel client.
Setting to 0 is a bit fishy, but I couldn't think of anything better
either. Right now, we just overflow 32-bit timestamps all the time
and get unpredictable results. I think we will end up implementing
some form of truncation, so times earlier than the start of the
file system specific epoch (1902, 1970, 1980, ... depending on the
implementation) get set to the earliest supported time, while
times after the last supported timestamp (2037, 2038, 2106, ...)
get set to the maximum.
The problem here is that we don't even know whether an underflow
or an overflow happened.
> I want the kernel client to be bug compatible with servers and other
> clients in ceph.git.
>
> >
> > a) Remain consistent with current 64-bit servers and clients,
> > interpreting the number as an unsigned 32-bit integer where
> > possible, and remain broken on existing 32-bit machines
> > until they start using 64-bit time_t.
> >
> > b) Change 64-bit clients to behave the same way that that 32-bit
> > clients and servers do today, and actually use the times as
> > a signed number that you claimed they already did.
> >
> > c) Interpret all times before 1980 or after 2038 as buggy servers
> > and only accept times that are always handled consistently.
> > Also fix the server to reject those times coming from broken
> > clients.
> >
> > d) Leave 64-bit clients unchanged (using unsigned timestamps) and
> > document that 32-bit clients should keep interpreting them as
> > signed even when we fix VFS, to preserve the existing behavior.
> >
> > I think a), b) and c) are all reasonable, and I can send a patch
> > as soon as you know what you want. If you want d), please leave
> > me out of that and do it yourself.
>
> My preference would be a), possibly with some comments along the lines
> of c), to make the ..2038 range more explicit, as I doubt we've ever
> assumed ..2106 on 64-bit. This is all really old code and goes back to
> the dawn of ceph and beyond the kernel client, so let's wait for Sage
> to chime in.
Ok, thanks!
I think the same problem appeared in a lot of places when code was
originally written with the assumption that time_t is 32-bit,
and then it kept working in practice when ported to 64-bit systems
for the first time but the underlying assumptions changed.
/*
* ceph timestamp handling is traditionally inconsistent between
* 32-bit clients and servers using them as a signed 32-bit time_t
* ranging from 1902 to 2038, and 64-bit clients and servers
* interpreting the same numbers as unsigned 32-bit integers
* with negative numbers turning into dates between 2038 and 2106.
*
* Most clients and servers these days are 64-bit, so let's pretend
* that interpreting the times as "__u32" is intended, and use
* that on 32-bit systems as well once using a 64-bit time_t.
*/
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
