On 8 November 2013 23:19, David Turner <novalis@xxxxxxxxxxx> wrote:
>
> On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote:
> > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@xxxxxxxxxxx> 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@xxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
