Re: [PATCH v10 09/14] exfat: add misc operations

[Pali RohÃr]

On Wednesday 15 January 2020 22:30:59 Namjae Jeon wrote:
> 2020-01-15 19:10 GMT+09:00, Arnd Bergmann <arnd@xxxxxxxx>:
> > On Wed, Jan 15, 2020 at 9:28 AM Namjae Jeon <namjae.jeon@xxxxxxxxxxx>
> > wrote:
> >
> >> +#define SECS_PER_MIN    (60)
> >> +#define TIMEZONE_SEC(x)        ((x) * 15 * SECS_PER_MIN)
> >> +
> >> +static void exfat_adjust_tz(struct timespec64 *ts, u8 tz_off)
> >> +{
> >> +       if (tz_off <= 0x3F)
> >> +               ts->tv_sec -= TIMEZONE_SEC(tz_off);
> >> +       else /* 0x40 <= (tz_off & 0x7F) <=0x7F */
> >> +               ts->tv_sec += TIMEZONE_SEC(0x80 - tz_off);
> >> +}
> >> +
> >> +static inline int exfat_tz_offset(struct exfat_sb_info *sbi)
> >> +{
> >> +       if (sbi->options.time_offset)
> >> +               return sbi->options.time_offset;
> >> +       return sys_tz.tz_minuteswest;
> >> +}
> >> +
> >> +/* Convert a EXFAT time/date pair to a UNIX date (seconds since 1 1 70).
> >> */
> >> +void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64
> >> *ts,
> >> +               __le16 time, __le16 date, u8 tz)
> >> +{
> >> +       u16 t = le16_to_cpu(time);
> >> +       u16 d = le16_to_cpu(date);
> >> +
> >> +       ts->tv_sec = mktime64(1980 + (d >> 9), d >> 5 & 0x000F, d &
> >> 0x001F,
> >> +                             t >> 11, (t >> 5) & 0x003F, (t & 0x001F) <<
> >> 1);
> >> +       ts->tv_nsec = 0;
> >
> > This part looks good to me now.
> Thanks.
> >
> >> +       if (tz & EXFAT_TZ_VALID)
> >> +               /* Treat as UTC time, but need to adjust timezone to UTC0
> >> */
> >> +               exfat_adjust_tz(ts, tz & ~EXFAT_TZ_VALID);
> >> +       else
> >> +               /* Treat as local time */
> >> +               ts->tv_sec -= exfat_tz_offset(sbi) * SECS_PER_MIN;
> >> +}
> >
> > Whereas this seems rather complex, when it deals with three different
> > cases:
> >
> > - timezone stored in inode
> > - timezone offset passed as mount option
> > - local time from sys_tz.tz_minuteswest
> >
> > Does the exfat specification require to use some notion of 'local time'
> > here
> > as the fallback? The problem with sys_tz.tz_minuteswest is that it is
> > not too well-defined,
> It is not described in the specification. I don't know exactly what
> the problem is because sys_tz.tz_minuteswest seems to work fine to me.
> It can be random garbage value ?
> > so if there is a choice, falling back to UTC would
> > be nicer.
> Okay.

Arnd, what is the default value of sys_tz.tz_minuteswest? What is the
benefit of not using it?

I though that timezone mount option is just an old hack when userspace
does not correctly set kernel's timezone and that this timezone mount
option should be in most cases avoided.

So also another question, what is benefit of having fs specific timezone
mount option? As it is fs specific it means that it would be used so
much.

> >
> >> +/* Convert linear UNIX date to a EXFAT time/date pair. */
> >> +void exfat_set_entry_time(struct exfat_sb_info *sbi, struct timespec64
> >> *ts,
> >> +               __le16 *time, __le16 *date, u8 *tz)
> >> +{
> >> +       struct tm tm;
> >> +       u16 t, d;
> >> +
> >> +       /* clamp to the range valid in the exfat on-disk representation.
> >> */
> >> +       time64_to_tm(clamp_t(time64_t, ts->tv_sec,
> >> EXFAT_MIN_TIMESTAMP_SECS,
> >> +               EXFAT_MAX_TIMESTAMP_SECS), -exfat_tz_offset(sbi) *
> >> SECS_PER_MIN,
> >> +               &tm);
> >
> > I think you can drop the clamping here, as thes_time_min/s_time_max fields
> > should take care of that.
> Okay.
> >
> > For writing out timestamps, it may be best to always encode them as UTC
> > and set set timezone-valid bit for that. That way, the min/max values
> > are known at compile time regardless of which time zone the machine
> > thinks it is in.
> Okay, I will check it.
> Thanks for your review!
> >
> >       Arnd
> >

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx