Akira Fujita wrote: > Hi, Andreas and Mark, > Thank you for looking at this issue. > > (2011/06/27 18:04), Andreas Dilger wrote: >> On 2011-06-24, at 11:12 PM, Mark Harris wrote: >>> On Fri, Jun 24, 2011 at 15:46, Andreas Dilger wrote: >>>> The problem with this encoding is that it requires existing 32-bit >>>> timestamps before 1970 to have encoded "11" in the extra epoch bits, >>>> which is not the case. Current pre-1970 timestamps would be encoded >>>> with "00" there, which would (according to your table) bump them past >>>> 2038 incorrectly. >>> >>> I was under the impression that the encoding code stored bits >>> 33& 32 of tv_sec there, which would be 1,1 for a negative value >>> like -1. Certainly the decoding would be simpler if the extra >>> value was only non-zero for large timestamps. >> >> One problem with a symmetrical encoding is that it wastes half of the >> dynamic range for values that nobody will ever use. Even values before >> 1970 seem so unlikely that I question whether we should support them >> at all. >> >>> On closer inspection of ext4_encode_extra_time, it looks like for >>> tv_sec = -1, a 64-bit kernel will store 1,1 in the extra bits and >>> a 32-bit kernel will store 0,0 in the extra bits. That is a problem >>> if both of these need to be decoded as -1 on a 64-bit system. >> >> That is definitely a problem. >> >>>> What we need is an encoding that preserves the times for extra epoch "00": >>>> >>>> 2 msb of adjustment needed to convert >>>> extra 32-bit sign-extended 32-bit tv_sec >>>> bits time decoded 64-bit tv_sec to decoded 64-bit tv_sec >>>> 0 0 1 -0x80000000..-1 0 >>>> 0 0 0 0x000000000..0x07fffffff 0 >>>> 0 1 1 0x080000000..0x0ffffffff 0x100000000 >>>> 0 1 0 0x100000000..0x17fffffff 0x100000000 >>>> 1 0 1 0x180000000..0x1ffffffff 0x200000000 >>>> 1 0 0 0x200000000..0x27fffffff 0x200000000 >>>> 1 1 1 0x280000000..0x2ffffffff 0x300000000 >>>> 1 1 0 0x300000000..0x37fffffff 0x300000000 >>>> >>>> So, looking at the above desired encoding, it looks like the error in >>>> the existing code is that it is doing a boolean operation on decode >>>> instead of a mathematical one, and it was incorrectly trying to extend >>>> the time to (1ULL<<34). The below should fix this: >>>> >>>> static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) >>>> { >>>> if (unlikely(sizeof(time->tv_sec)> 4&& >>>> (extra& cpu_to_le32(EXT4_EPOCH_MASK))) >>>> time->tv_sec += (u64)(le32_to_cpu(extra)& EXT4_EPOCH_MASK)<< 32; >>>> >>>> time->tv_nsec = (le32_to_cpu(extra)& EXT4_NSEC_MASK)>> EXT4_EPOCH_BITS; >>>> } >>> >>> That is not compatible with the existing ext4_encode_extra_time. >>> For example, 2038-01-31 (0x80101500) is encoded with extra bits >>> equal to bits 33& 32, i.e. 0,0. But this code would decode it >>> as 1901-12-25 (i.e. it would leave the sign-extended 32-bit value >>> unchanged). >> >> Part of the problem is that the encoding/decoding of timestamps beyond >> 2038 is already broken today, so I don't think anyone has been using >> them so far. This gives us some leeway for fixing this problem I think. >> >>> Possible solutions: >>> >>> (a) Define the current 64-bit encoding as the correct encoding since >>> the 2 extra bits are not even decoded on 32-bit kernels, so its >>> encoding doesn't matter much. However, if anyone with existing >>> pre-1970 timestamps written using a 32-bit kernel wants to use >>> their ext4 filesystem with a 64-bit kernel, the pre-1970 >>> timestamps would be wrong unless they re-write them with a >>> fixed kernel. >>> >>> Change ext4_decode_extra_time "if" body to something like: >>> time->tv_sec += ((__u32)time->tv_sec + >>> ((__u64)le32_to_cpu(extra)<< 32) + >>> 0x80000000LL)& 0x300000000LL; >>> >>> Change ext4_encode_extra_time ": 0" to something like: >>> time->tv_sec< 0 ? EXT4_EPOCH_MASK : 0 >> >> The real-world problem isn't with 32-bit systems, where it doesn't >> really matter at all how time is encoded, nor with files on 64-bit systems >> with timestamps 26 years in the future, but rather 256-byte inodes that >> were previously written with ext3 that will break if they are mounted >> with ext4 on a 64-bit system. >> >>> (b) Change the encoding of the extra bits to be those in your new >>> table. This is compatible with the 32-bit kernel encoding >>> (which does not decode these bits) but incompatible with the >>> 64-bit kernel encoding. Existing pre-1970 timestamps written >>> with a 64-bit kernel would be decoded as dates far in the future. >>> >>> Requires your change to ext4_decode_extra_time. >>> Also requires a change to ext4_encode_extra_time, changing >>> (time->tv_sec>> 32) to something like: >>> ((time->tv_sec - (signed int)time->tv_sec)>> 32) >> >> I think this is a reasonable solution, but I dislike that it breaks >> pre-1970 timestamps on 64-bit systems. > > I agree with this solution. > I guess that no one has pre-1970 timestamps on ext4, actually. > > Mark, are you working on this right now? > If you have a patch to fix this issue, please send it to the list. I think that all of the code changes needed to implement this solution (b) are in ext4_decode_extra_time and ext4_encode_extra_time and are included above, if you want to submit them in patch format as a new version of your patch. The issue with this solution is that it breaks existing 1901..1969 timestamps written with a 64-bit kernel to ext4 with 256-byte inodes. (It breaks existing year 2038+ timestamps as well, but those are already broken.) It sounds like Andreas favors either (b) or (c) but would like to hear from Ted. - Mark > > Regards, > Akira Fujita > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html