| Message ID | 1383981551.8994.27.camel@chiang |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 8 November 2013 23:19, David Turner <novalis@novalis.org> wrote: > > On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote: > > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > > > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > > >> Still unnecessary type cast here (but that's a cosmetic issue). > > > ... > > >> Otherwise the patch looks good. You can add: > > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > > > Thanks. A version with this correction and the reviewed-by follows. > > > > Thanks for working on this. It was (seriously) in my list of things to > > get done, but I figured I still had a few years to work on it... > > > > My (unfinished) version of this patch had a nice comment at ext4_encode_time() > > that explained the encoding that is being used very clearly: > > > > /* > > * We need is an encoding that preserves the times for extra epoch "00": > > * > > * extra msb of adjust for signed > > * epoch 32-bit 32-bit tv_sec to > > * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > > * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > > * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > > * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > > * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > > * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > > * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > > * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > > * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > > */ > > > > It seems that your version of the patch seems to use a different encoding. Not > > that this is a problem, per-se, since my patch wasn’t in use anywhere, but it > > would be nice to have a similarly clear explanation of what the mapping is so > > that it can be clearly understood. > > I have included a patch with an explanation (the patch is against > tytso/dev -- I hope that's the correct place). > > > My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, > > which changed the on-disk format for times beyond 2038, but those were already > > broken, and presumably not in use by anyone yet. > > They were actually correct according to my patch's encoding (that is, my > patch used the existing encoding). The existing encoding was written > correctly, but read wrongly. As you say, this should not matter, since > nobody should be writing these timestamps, but I assumed that the > existing encoding had happened for a reason, and wanted to make the > minimal change. > > If you believe it is important, I would be happy to change it. The problem with the existing encoding is that pre-1970 dates are encoded with extra bits 1,1 in 64-bit kernels with ext4, but on 32-bit kernels and inodes that were originally written as ext3 the extra bits will be 0,0. Currently, both are decoded as pre-1970 dates. With your patch, only the 1,1 format used by 64-bit ext4 will decode as a pre-1970 date. Dates previously written by ext3 or a 32-bit kernel will no longer be decoded as expected. Also the patch does not update ext4_encode_extra_time to use this format for pre-1970 dates in 32-bit mode. Possible solutions were discussed here, but no decision was made: http://thread.gmane.org/gmane.comp.file-systems.ext4/26087/focus=26406 > > > > However, it seemed to me this > > was easier to produce the correct results. Have you tested your patch with > > a variety of timestamps to verify its correctness? > > I tested by using touch -d to create one file in each year between 1902 > and 2446. Then I unmounted and remounted the FS, and did ls -l and > manually verified that each file's date matched its name. > > > It looks to me like you > > have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead > > of the (IMHO) natural 0x0 bits. The critical time ranges are listed > > above. > > I think the idea of this is that it is the bottom 34 bits of the 64-bit > signed time. However, it occurs to me that this relies on a two's > complement machine. Even though the C standard does not guarantee this, > I believe the kernel requires it, so that's probably OK. - Mark -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2013-11-09 at 15:51 -0800, Mark Harris wrote: > > The problem with the existing encoding is that pre-1970 dates are > encoded with extra bits 1,1 in 64-bit kernels with ext4, but on 32-bit > kernels and inodes that were originally written as ext3 the extra bits > will be 0,0. Currently, both are decoded as pre-1970 dates. > > With your patch, only the 1,1 format used by 64-bit ext4 will decode > as a pre-1970 date. Dates previously written by ext3 or a 32-bit > kernel will no longer be decoded as expected. Also the patch does > not update ext4_encode_extra_time to use this format for pre-1970 > dates in 32-bit mode. You're right -- I missed the complexity here. > Possible solutions were discussed here, but no decision was made: > http://thread.gmane.org/gmane.comp.file-systems.ext4/26087/focus=26406 To summarize, the previous discussion offered four possible solutions, of which two were thought good: b. Use Andreas's encoding, which is incompatible with pre-1970 files written on 64-bit systems. c. Use an encoding which is compatible with all pre-1970 files, but incompatible with 64-bit post-2038 files, and which encodes a smaller range of time and is more complicated. ------- I don't care about currently-existing post-2038 files, because I believe that nobody has a valid reason to have such files. However, I do believe that pre-1970 files are probably important to someone. Despite this, I prefer option (b), because I think the simplicity is valuable, and because I hate to give up date ranges (even ones that I think we'll "never" need). Option (b) is not actually lossy, because we could correct pre-1970 files with e2fsck; under Andreas's encoding, their dates would be in the far future (and thus cannot be legitimate). Would a patch that does (b) be accepted? I would accompany it with a patch to e2fsck (which I assume would also go to the ext4 developers mailing list?). -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > b. Use Andreas's encoding, which is incompatible with pre-1970 files > written on 64-bit systems. > > I don't care about currently-existing post-2038 files, because I believe > that nobody has a valid reason to have such files. However, I do > believe that pre-1970 files are probably important to someone. > > Despite this, I prefer option (b), because I think the simplicity is > valuable, and because I hate to give up date ranges (even ones that I > think we'll "never" need). Option (b) is not actually lossy, because we > could correct pre-1970 files with e2fsck; under Andreas's encoding, > their dates would be in the far future (and thus cannot be legitimate). > > Would a patch that does (b) be accepted? I would accompany it with a > patch to e2fsck (which I assume would also go to the ext4 developers > mailing list?). I agree, I think this is the best way to go. I'm going to drop your earlier patch, and wait for an updated patch from you. It may miss this merge window, but as Andreas has pointed out, we still have a few years to get this right. :-) Thanks!! - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Nov 11, 2013, at 5:30 PM, Theodore Ts'o <tytso@mit.edu> wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: >> b. Use Andreas's encoding, which is incompatible with pre-1970 files >> written on 64-bit systems. >> >> I don't care about currently-existing post-2038 files, because I believe >> that nobody has a valid reason to have such files. However, I do >> believe that pre-1970 files are probably important to someone. >> >> Despite this, I prefer option (b), because I think the simplicity is >> valuable, and because I hate to give up date ranges (even ones that I >> think we'll "never" need). Option (b) is not actually lossy, because we >> could correct pre-1970 files with e2fsck; under Andreas's encoding, >> their dates would be in the far future (and thus cannot be legitimate). >> >> Would a patch that does (b) be accepted? I would accompany it with a >> patch to e2fsck (which I assume would also go to the ext4 developers >> mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Since this change would immediately break files encoded with pre-1970 dates on ext4 filesystems, should there be a transition period where both pre-1970 dates (with extra epoch bits == 0x3 in the current encoding) and post-2378 (with extra epoch bits == 0x3 in the new encoding) are decoded as being pre-1970? That could be conditional until some release in the future (e.g. >= Linux 4.20, at least 5 years away) to give folks a chance to run the new e2fsck to fix up those files. Are there really any ext4 filesystems that have files with valid dates so old? I don’t want to break anyone’s data, but if this extra complexity is completely pointless then I’m also fine with this minor risk of breakage. Cheers, Andreas
On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > written on 64-bit systems. > > > > I don't care about currently-existing post-2038 files, because I believe > > that nobody has a valid reason to have such files. However, I do > > believe that pre-1970 files are probably important to someone. > > > > Despite this, I prefer option (b), because I think the simplicity is > > valuable, and because I hate to give up date ranges (even ones that I > > think we'll "never" need). Option (b) is not actually lossy, because we > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > their dates would be in the far future (and thus cannot be legitimate). > > > > Would a patch that does (b) be accepted? I would accompany it with a > > patch to e2fsck (which I assume would also go to the ext4 developers > > mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Just to be clear... we're going with Andreas' fix, wherein time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; becomes: time->tv_sec += (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; "or" becomes "plus"? So I can update fuse2fs. Also, can someone proofread [1] and make sure it's correct? --D [1] https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#Inode_Timestamps > > Thanks!! > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-11-12 at 15:03 -0800, Darrick J. Wong wrote: > On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > > written on 64-bit systems. > > > > > > I don't care about currently-existing post-2038 files, because I believe > > > that nobody has a valid reason to have such files. However, I do > > > believe that pre-1970 files are probably important to someone. > > > > > > Despite this, I prefer option (b), because I think the simplicity is > > > valuable, and because I hate to give up date ranges (even ones that I > > > think we'll "never" need). Option (b) is not actually lossy, because we > > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > > their dates would be in the far future (and thus cannot be legitimate). > > > > > > Would a patch that does (b) be accepted? I would accompany it with a > > > patch to e2fsck (which I assume would also go to the ext4 developers > > > mailing list?). > > > > I agree, I think this is the best way to go. I'm going to drop your > > earlier patch, and wait for an updated patch from you. It may miss > > this merge window, but as Andreas has pointed out, we still have a few > > years to get this right. :-) > > Just to be clear... we're going with Andreas' fix, wherein > > time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; > > becomes: > > time->tv_sec += (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; > > "or" becomes "plus"? So I can update fuse2fs. Yes, but with a kernel-version-dependent change, which is something like this: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4,20,0) time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; #else u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; if (extra_bits == 3) extra_bits = 0; time->tv_sec += extra_bits << 32; #endif > Also, can someone proofread [1] and make sure it's correct? It's not quite right. I've requested an account so that I can correct it. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > written on 64-bit systems. > > > > I don't care about currently-existing post-2038 files, because I believe > > that nobody has a valid reason to have such files. However, I do > > believe that pre-1970 files are probably important to someone. > > > > Despite this, I prefer option (b), because I think the simplicity is > > valuable, and because I hate to give up date ranges (even ones that I > > think we'll "never" need). Option (b) is not actually lossy, because we > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > their dates would be in the far future (and thus cannot be legitimate). > > > > Would a patch that does (b) be accepted? I would accompany it with a > > patch to e2fsck (which I assume would also go to the ext4 developers > > mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Just out of curiosity, did this (updated patch) ever happen? --D > > Thanks!! > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-01-21 at 22:22 -0800, Darrick J. Wong wrote: > On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > > written on 64-bit systems. > > > > > > I don't care about currently-existing post-2038 files, because I believe > > > that nobody has a valid reason to have such files. However, I do > > > believe that pre-1970 files are probably important to someone. > > > > > > Despite this, I prefer option (b), because I think the simplicity is > > > valuable, and because I hate to give up date ranges (even ones that I > > > think we'll "never" need). Option (b) is not actually lossy, because we > > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > > their dates would be in the far future (and thus cannot be legitimate). > > > > > > Would a patch that does (b) be accepted? I would accompany it with a > > > patch to e2fsck (which I assume would also go to the ext4 developers > > > mailing list?). > > > > I agree, I think this is the best way to go. I'm going to drop your > > earlier patch, and wait for an updated patch from you. It may miss > > this merge window, but as Andreas has pointed out, we still have a few > > years to get this right. :-) > > Just out of curiosity, did this (updated patch) ever happen? I think I sent a usable patch that Ted merged part of into e2fscktools; the kernel portion was dropped for some reason. While I was waiting to hear back on the kernel portion, I started looking into the dtime stuff, but then I got distracted by a new job. Assuming that I won't have time to deal with dtime (since it seems to be much more complicated), is the right way forward for me to rebase the non-dtime portion of my patch against the latest kernel, and resend it? If so, will it get merged? (Assume here that I do the same with the e2fsck stuff) -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Feb 10, 2014, at 10:12 PM, David Turner <novalis@novalis.org> wrote: > On Tue, 2014-01-21 at 22:22 -0800, Darrick J. Wong wrote: >> On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: >>> On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: >>>> b. Use Andreas's encoding, which is incompatible with pre-1970 files >>>> written on 64-bit systems. >>>> >>>> I don't care about currently-existing post-2038 files, because I believe >>>> that nobody has a valid reason to have such files. However, I do >>>> believe that pre-1970 files are probably important to someone. >>>> >>>> Despite this, I prefer option (b), because I think the simplicity is >>>> valuable, and because I hate to give up date ranges (even ones that I >>>> think we'll "never" need). Option (b) is not actually lossy, because we could correct pre-1970 files with e2fsck; under Andreas's encoding, >>>> their dates would be in the far future (and thus cannot be legitimate). >>>> >>>> Would a patch that does (b) be accepted? I would accompany it with a >>>> patch to e2fsck (which I assume would also go to the ext4 developers >>>> mailing list?). >>> >>> I agree, I think this is the best way to go. I'm going to drop your >>> earlier patch, and wait for an updated patch from you. It may miss >>> this merge window, but as Andreas has pointed out, we still have a few >>> years to get this right. :-) >> >> Just out of curiosity, did this (updated patch) ever happen? > > I think I sent a usable patch that Ted merged part of into e2fscktools; > the kernel portion was dropped for some reason. > > While I was waiting to hear back on the kernel portion, I started > looking into the dtime stuff, but then I got distracted by a new job. > > Assuming that I won't have time to deal with dtime (since it seems to be > much more complicated), is the right way forward for me to rebase the > non-dtime portion of my patch against the latest kernel, and resend it? > If so, will it get merged? (Assume here that I do the same with the > e2fsck stuff) Yes, please submit an updated patch for the kernel. Ted will hopefully merge it this time. I don't know that we really care about dtime so much, since it is mostly just treated as "zero == not deleted" and "non-zero == deleted" by e2fsck, and maybe useful for manual debugging. We might consider that it uses a sliding window in the future, since I'm sure we won't care about files deleted more than 70 years ago, and if we did the fact that they appear to be files deleted 70 years in the future won't matter so much. In any case, that can be looked at in a separate patch. Cheers, Andreas
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 121da383..ab69f14 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -713,6 +713,24 @@ struct move_extent { sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ +/* + * We use the bottom 34 bits of the signed 64-bit time value, with + * the top two of these bits in the bottom of extra. This leads + * to a slightly odd encoding, which works like this: + * + * extra msb of + * epoch 32-bit + * bits time decoded 64-bit tv_sec valid time range + * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 + * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 + * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 + * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 + + * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 + */ static inline __le32 ext4_encode_extra_time(struct timespec *time) {