Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: "Pali Rohár" <pali.rohar@gmail.com>
To: Namjae Jeon <linkinjeon@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Namjae Jeon <namjae.jeon@samsung.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	gregkh <gregkh@linuxfoundation.org>,
	Valdis Kletnieks <valdis.kletnieks@vt.edu>,
	Christoph Hellwig <hch@lst.de>,
	sj1557.seo@samsung.com
Subject: Re: [PATCH v10 09/14] exfat: add misc operations
Date: Wed, 15 Jan 2020 14:38:38 +0100
Message-ID: <20200115133838.q33p5riihsinp6c4@pali> (raw)
In-Reply-To: <CAKYAXd9_qmanQCcrdpScFWvPXuZvk4jhv7Gc=t_vRL9zqWNSjA@mail.gmail.com>

On Wednesday 15 January 2020 22:30:59 Namjae Jeon wrote:
> 2020-01-15 19:10 GMT+09:00, Arnd Bergmann <arnd@arndb.de>:
> > On Wed, Jan 15, 2020 at 9:28 AM Namjae Jeon <namjae.jeon@samsung.com>
> > 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@gmail.com

  reply index

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20200115082818epcas1p4892a99345626188afd111ee263132458@epcas1p4.samsung.com>
2020-01-15  8:24 ` [PATCH v10 00/14] add the latest exfat driver Namjae Jeon
     [not found]   ` <CGME20200115082819epcas1p3303009557f8bf670b47991530f380ef7@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 01/14] exfat: add in-memory and on-disk structures and headers Namjae Jeon
     [not found]   ` <CGME20200115082819epcas1p3825d5bed592a8378ad26254d07d59b18@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 02/14] exfat: add super block operations Namjae Jeon
     [not found]   ` <CGME20200115082820epcas1p34ebebebaf610fd61c4e9882fca8ddbd5@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 03/14] exfat: add inode operations Namjae Jeon
2020-01-15 10:25       ` Arnd Bergmann
2020-01-15 13:39         ` Namjae Jeon
2020-01-17  9:25       ` Pali Rohár
2020-01-17 11:17       ` Pali Rohár
     [not found]   ` <CGME20200115082821epcas1p3db1f70cf53185c40934c3a754c65e648@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 04/14] exfat: add directory operations Namjae Jeon
2020-01-17 11:13       ` Pali Rohár
     [not found]   ` <CGME20200115082821epcas1p4d76d8668dfac70ae3e3889d4ccb6c3ee@epcas1p4.samsung.com>
2020-01-15  8:24     ` [PATCH v10 05/14] exfat: add file operations Namjae Jeon
2020-01-15  9:56       ` Arnd Bergmann
2020-01-15 13:07         ` Namjae Jeon
2020-01-17  9:18       ` Pali Rohár
2020-01-17 12:12         ` Namjae Jeon
     [not found]   ` <CGME20200115082822epcas1p37aca37dad0aa9159d7eddde01edfeec2@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 06/14] exfat: add fat entry operations Namjae Jeon
     [not found]   ` <CGME20200115082822epcas1p384d080724d8bb0a8cd4c3e3015a0895c@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 07/14] exfat: add bitmap operations Namjae Jeon
     [not found]   ` <CGME20200115082823epcas1p27ec5ff6d384f148d826529f573173b04@epcas1p2.samsung.com>
2020-01-15  8:24     ` [PATCH v10 08/14] exfat: add exfat cache Namjae Jeon
     [not found]   ` <CGME20200115082824epcas1p4eb45d088c2f88149acb94563c4a9b276@epcas1p4.samsung.com>
2020-01-15  8:24     ` [PATCH v10 09/14] exfat: add misc operations Namjae Jeon
2020-01-15 10:10       ` Arnd Bergmann
2020-01-15 13:30         ` Namjae Jeon
2020-01-15 13:38           ` Pali Rohár [this message]
2020-01-15 13:50             ` Arnd Bergmann
2020-01-15 14:24               ` Pali Rohár
2020-01-15 15:03                 ` Arnd Bergmann
2020-01-15 15:39                   ` Pali Rohár
2020-01-15 15:52                     ` Arnd Bergmann
2020-01-16 10:19                       ` Pali Rohár
2020-01-16 10:23                         ` Christoph Hellwig
2020-01-16 16:00                           ` Pali Rohár
2020-01-17  2:59                       ` Namjae Jeon
2020-01-17 10:13                         ` Arnd Bergmann
2020-01-17 12:13                           ` Namjae Jeon
     [not found]   ` <CGME20200115082824epcas1p23a0fa255a5833c6dcd2479b384f37740@epcas1p2.samsung.com>
2020-01-15  8:24     ` [PATCH v10 10/14] exfat: add nls operations Namjae Jeon
     [not found]   ` <CGME20200115082825epcas1p1f22ddca6dbf5d70e65d3b0e3c25c3a59@epcas1p1.samsung.com>
2020-01-15  8:24     ` [PATCH v10 11/14] exfat: add Kconfig and Makefile Namjae Jeon
2020-01-15  9:39       ` Pali Rohár
2020-01-17  4:22         ` Namjae Jeon
2020-01-17  9:12           ` Pali Rohár
2020-01-17 11:59             ` Namjae Jeon
     [not found]   ` <CGME20200115082825epcas1p4a0d63d53d8737a84fe8867bc1e63811e@epcas1p4.samsung.com>
2020-01-15  8:24     ` [PATCH v10 12/14] exfat: add exfat in fs/Kconfig and fs/Makefile Namjae Jeon
     [not found]   ` <CGME20200115082826epcas1p117191a39ecccaef5a9b5cbe77b05c5a5@epcas1p1.samsung.com>
2020-01-15  8:24     ` [PATCH v10 13/14] MAINTAINERS: add exfat filesystem Namjae Jeon
     [not found]   ` <CGME20200115082826epcas1p3475ce2b4d03234dc96ced428be582eb3@epcas1p3.samsung.com>
2020-01-15  8:24     ` [PATCH v10 14/14] staging: exfat: make staging/exfat and fs/exfat mutually exclusive Namjae Jeon
2020-01-15  8:56       ` Greg KH
2020-01-15 13:06         ` Namjae Jeon
2020-01-15  9:47   ` [PATCH v10 00/14] add the latest exfat driver Pali Rohár
2020-01-15 13:05     ` Namjae Jeon
2020-01-16 10:51     ` Christoph Hellwig
2020-01-16 11:32       ` Pali Rohár
2020-01-16 13:01       ` Greg KH

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200115133838.q33p5riihsinp6c4@pali \
    --to=pali.rohar@gmail.com \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@lst.de \
    --cc=linkinjeon@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=namjae.jeon@samsung.com \
    --cc=sj1557.seo@samsung.com \
    --cc=valdis.kletnieks@vt.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git