On Sat, 2013-12-07 at 19:53 -0500, Theodore Ts'o wrote: > On Sat, Dec 07, 2013 at 03:02:40PM -0500, David Turner wrote: > > > > However, as Andreas notes, "we want to verify .. that "debugfs -R > > 'stat testfile'" decodes the times correctly." Unfortunately, it > > does not, and it is not trivial to fix. debugfs uses an internal > > function time_to_string(__u32), which calls gmtime or localtime. > > These functions are defined on time_t, which is 32-bits on 32-bit > > Linux systems. In addition, because time_to_string takes > > an unsigned int, it returns bad results for pre-1970 dates. > > We can and should change time_to_string to take an unsigned 64-bit > type; it's an internal interface to debugfs. Shouldn't this be a signed 64-bit type, since we have to support times before 1970? > The question of what to do if the system time_t is only 32-bit. > > What I'd probably suggest is to have a mode (set either by an > environment variable, or a debugfs command) which causes > time_to_string to use an internal version of a 64-bit capable gmtime > function. This will allow for better regression testing, and it in a > way that will be have stable results regardless of whether a > particular platform, or a future version of glibc has a 64-bit time_t > or not. Presently, the TZ environment variable selects between gmtime and localtime. How about the following: We supply a 64-bit version of gmtime (but not localtime). If time_t is 32 bits, and the date being printed is after 2038, and TZ is not GMT, then we return an error message instead of calling localtime. Then, in any of the tests that depend on debugfs date printing we can simply set TZ=GMT, so that they will continue to work regardless of the size of time_t. > > Also, while I was making this change, I happened to notice that > > there is no dtime_extra field into which to put the extra bits. > > This means that there is still a year 2038 problem with tools > > that deal with deleted files (including the corner case that the > > system is shut down cleanly at the exact second that the bottom > > 32 bits of the current time are zero, leading fsck to believe > > that there was an unclean shutdown). > > I'm not sure we care, since nothing is actually using the dtime. This > was originally intended to make it easy to recover from accident file > deletions, using debugfs's lsdel command. However, ever since ext3, > in order to make sure the file system is consistent if it crashes > during an unlink, we end up truncating the file before we unlink it, > so i_blocks[] is completely cleared for deleted inodes. This makes it > essentially impossible for lsdel to work. I just did a quick grep, and it looks like fs/ext4/ialloc.c:recently_deleted is using it. That's not relevant to e2fsprogs, but we should correct it anyway, because it will probably break in 2038. Also fs/ext4/inode.c:ext4_do_update_inode -- I'm not sure quite what's going on there, but it seems like it should be fixed as well. I'll volunteer to make these changes to the kernel and to e2fsprogs, but I think my current set of patches can be safely merged before I do that. > > I want to make an additional change to correct this. Since I > > assume it is impossible to add an additional field, I propose to > > use ctime_extra to store the extra bits of dtime on deletion. > > This is a bit hackish, since it does destroy a bit of data, but > > it is significantly better than dtime being totally broken after > > 2038. What do people think? > > Given that we always set ctime when delete a file (which makes sense, > since we are decrementing i_links_count), I think changing debugfs > (which is the only thing that even looks at dtime, BTW) makes a lot of > sense. I'm not really very familiar with the ext4 internals. Are you saying that it's safe to just read ctime_extra (without explicitly writing it when we write dtime)? Or just that it is safe to overwrite it when we write dtime? -- 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
