| Message ID | 20210909065543.164329-1-cccheng@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exfat: use local UTC offset when EXFAT_TZ_VALID isn't set | expand |
2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng <cccheng@synology.com>: > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat > specification [1]. If this bit isn't set, timestamps should be treated > as having the same UTC offset as the current local time. > > This patch uses the existing mount option 'time_offset' as fat does. If > time_offset isn't set, local UTC offset in sys_tz will be used as the > default value. > > Link: [1] > https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#74102-offsetvalid-field > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> Please read this discussion: https://patchwork.kernel.org/project/linux-fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ Thanks!
Hello, Namjae, I found an important difference between the code we first wrote and the code that has changed since our initial patch review. This difference seems to cause compatibility issues when reading saved timestamps without timezone. (In our initial patch review, there were concerns about possible compatibility issues.) I think the code that reads timestamps without timezone should go back to the concept we wrote in the first place like reported patch. It could be an answer of another timestamp issue. Could you please let me know what you think? Thanks. > -----Original Message----- > From: Namjae Jeon [mailto:linkinjeon@kernel.org] > Sent: Friday, September 10, 2021 10:01 AM > To: Chung-Chiang Cheng <cccheng@synology.com> > Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org; linux- > kernel@vger.kernel.org; shepjeng@gmail.com > Subject: Re: [PATCH] exfat: use local UTC offset when EXFAT_TZ_VALID isn't > set > > 2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng <cccheng@synology.com>: > > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat > > specification [1]. If this bit isn't set, timestamps should be treated > > as having the same UTC offset as the current local time. > > > > This patch uses the existing mount option 'time_offset' as fat does. > > If time_offset isn't set, local UTC offset in sys_tz will be used as > > the default value. > > > > Link: [1] > > https://protect2.fireeye.com/v1/url?k=cba4edf5-943fd4c8-cba566ba-0cc47 > > a31309a-e70aa065be678729&q=1&e=225feff2-841f-404c-9a2e-c12064b232d0&u= > > https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows%2Fwin32%2Ffileio%2F > > exfat-specification%2374102-offsetvalid-field > > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > Please read this discussion: > https://patchwork.kernel.org/project/linux- > fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ > > Thanks!
2021-10-01 22:19 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: > Hello, Namjae, Hi Sungjong, > > I found an important difference between the code we first wrote and the code > that has changed since our initial patch review. This difference seems to > cause compatibility issues when reading saved timestamps without timezone. > (In our initial patch review, there were concerns about possible > compatibility issues.) > I think the code that reads timestamps without timezone should go back to > the concept we wrote in the first place like reported patch. Are you talking about using sys_tz? > It could be an answer of another timestamp issue. What is another timestamp issue ? > > Could you please let me know what you think? > > Thanks. >> -----Original Message----- >> From: Namjae Jeon [mailto:linkinjeon@kernel.org] >> Sent: Friday, September 10, 2021 10:01 AM >> To: Chung-Chiang Cheng <cccheng@synology.com> >> Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org; linux- >> kernel@vger.kernel.org; shepjeng@gmail.com >> Subject: Re: [PATCH] exfat: use local UTC offset when EXFAT_TZ_VALID >> isn't >> set >> >> 2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng <cccheng@synology.com>: >> > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat >> > specification [1]. If this bit isn't set, timestamps should be treated >> > as having the same UTC offset as the current local time. >> > >> > This patch uses the existing mount option 'time_offset' as fat does. >> > If time_offset isn't set, local UTC offset in sys_tz will be used as >> > the default value. >> > >> > Link: [1] >> > https://protect2.fireeye.com/v1/url?k=cba4edf5-943fd4c8-cba566ba-0cc47 >> > a31309a-e70aa065be678729&q=1&e=225feff2-841f-404c-9a2e-c12064b232d0&u= >> > https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows%2Fwin32%2Ffileio%2F >> > exfat-specification%2374102-offsetvalid-field >> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> >> Please read this discussion: >> https://patchwork.kernel.org/project/linux- >> fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ >> >> Thanks! > >
> 2021-10-01 22:19 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: > > Hello, Namjae, > Hi Sungjong, > > > > I found an important difference between the code we first wrote and > > the code that has changed since our initial patch review. This > > difference seems to cause compatibility issues when reading saved > timestamps without timezone. > > (In our initial patch review, there were concerns about possible > > compatibility issues.) I think the code that reads timestamps without > > timezone should go back to the concept we wrote in the first place > > like reported patch. > Are you talking about using sys_tz? Yes, exactly, a part like below. +static inline int exfat_tz_offset(struct exfat_sb_info *sbi) { + return (sbi->options.tz_set ? -sbi->options.time_offset : + sys_tz.tz_minuteswest) * SECS_PER_MIN; } + > > > It could be an answer of another timestamp issue. > What is another timestamp issue ? What I'm saying is "timestamp incompatibilities in exfat-fs" from Reiner <reinerstallknecht@gmail.com> I think it might be the same issue with this. > > > > > Could you please let me know what you think? > > > > Thanks. > >> -----Original Message----- > >> From: Namjae Jeon [mailto:linkinjeon@kernel.org] > >> Sent: Friday, September 10, 2021 10:01 AM > >> To: Chung-Chiang Cheng <cccheng@synology.com> > >> Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org; linux- > >> kernel@vger.kernel.org; shepjeng@gmail.com > >> Subject: Re: [PATCH] exfat: use local UTC offset when EXFAT_TZ_VALID > >> isn't set > >> > >> 2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng <cccheng@synology.com>: > >> > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat > >> > specification [1]. If this bit isn't set, timestamps should be > >> > treated as having the same UTC offset as the current local time. > >> > > >> > This patch uses the existing mount option 'time_offset' as fat does. > >> > If time_offset isn't set, local UTC offset in sys_tz will be used > >> > as the default value. > >> > > >> > Link: [1] > >> > https://protect2.fireeye.com/v1/url?k=cba4edf5-943fd4c8-cba566ba-0c > >> > c47 > >> > a31309a-e70aa065be678729&q=1&e=225feff2-841f-404c-9a2e-c12064b232d0 > >> > &u= > >> > https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows%2Fwin32%2Ffileio > >> > %2F exfat-specification%2374102-offsetvalid-field > >> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > >> Please read this discussion: > >> https://patchwork.kernel.org/project/linux- > >> fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ > >> > >> Thanks! > > > >
2021-10-05 13:05 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: >> 2021-10-01 22:19 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: >> > Hello, Namjae, >> Hi Sungjong, >> > >> > I found an important difference between the code we first wrote and >> > the code that has changed since our initial patch review. This >> > difference seems to cause compatibility issues when reading saved >> timestamps without timezone. >> > (In our initial patch review, there were concerns about possible >> > compatibility issues.) I think the code that reads timestamps without >> > timezone should go back to the concept we wrote in the first place >> > like reported patch. >> Are you talking about using sys_tz? > Yes, exactly, a part like below. Have you read discussion about this before ? Let me know what I am missing something. > > +static inline int exfat_tz_offset(struct exfat_sb_info *sbi) { > + return (sbi->options.tz_set ? -sbi->options.time_offset : > + sys_tz.tz_minuteswest) * SECS_PER_MIN; } > + > >> >> > It could be an answer of another timestamp issue. >> What is another timestamp issue ? > > What I'm saying is "timestamp incompatibilities in exfat-fs" from Reiner > <reinerstallknecht@gmail.com> > I think it might be the same issue with this. Have you checked fuse-exfat patch he shared ? It was exfat timezone support. I am not sure how it is related to sys_tz... Thanks! > >> >> > >> > Could you please let me know what you think? >> > >> > Thanks. >> >> -----Original Message----- >> >> From: Namjae Jeon [mailto:linkinjeon@kernel.org] >> >> Sent: Friday, September 10, 2021 10:01 AM >> >> To: Chung-Chiang Cheng <cccheng@synology.com> >> >> Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org; linux- >> >> kernel@vger.kernel.org; shepjeng@gmail.com >> >> Subject: Re: [PATCH] exfat: use local UTC offset when EXFAT_TZ_VALID >> >> isn't set >> >> >> >> 2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng <cccheng@synology.com>: >> >> > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat >> >> > specification [1]. If this bit isn't set, timestamps should be >> >> > treated as having the same UTC offset as the current local time. >> >> > >> >> > This patch uses the existing mount option 'time_offset' as fat does. >> >> > If time_offset isn't set, local UTC offset in sys_tz will be used >> >> > as the default value. >> >> > >> >> > Link: [1] >> >> > https://protect2.fireeye.com/v1/url?k=cba4edf5-943fd4c8-cba566ba-0c >> >> > c47 >> >> > a31309a-e70aa065be678729&q=1&e=225feff2-841f-404c-9a2e-c12064b232d0 >> >> > &u= >> >> > https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows%2Fwin32%2Ffileio >> >> > %2F exfat-specification%2374102-offsetvalid-field >> >> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> >> >> Please read this discussion: >> >> https://patchwork.kernel.org/project/linux- >> >> fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ >> >> >> >> Thanks! >> > >> > > >
> 2021-10-05 13:05 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: > >> 2021-10-01 22:19 GMT+09:00, Sungjong Seo <sj1557.seo@samsung.com>: > >> > Hello, Namjae, > >> Hi Sungjong, > >> > > >> > I found an important difference between the code we first wrote and > >> > the code that has changed since our initial patch review. This > >> > difference seems to cause compatibility issues when reading saved > >> timestamps without timezone. > >> > (In our initial patch review, there were concerns about possible > >> > compatibility issues.) I think the code that reads timestamps > >> > without timezone should go back to the concept we wrote in the > >> > first place like reported patch. > >> Are you talking about using sys_tz? > > Yes, exactly, a part like below. > Have you read discussion about this before ? Of course. That's why I wrote the expression "go back". As you know, there was the discussion of local time conversion method for reading timestamps that have no OffsetValid bit. that is, whether to use sys_tz or not was the main topic. And I understand that the current exFAT implementation doesn't use sys_tz and depends entirely on time_offset.(UTC if time_offset is not set) > Let me know what I am missing something. No, there is nothing you are missing. I thought compatibility issues regarding sys_tz could be serious because I misunderstood that other compatibility issues were reported. :( I am just concerned that most system daemons mount exfat-fs without time_offset option, which could be reported as a compatibility issue. However, to discuss this more, it would be nice to see the trend of how many issues regarding sys_tz will be reported. > > > > > +static inline int exfat_tz_offset(struct exfat_sb_info *sbi) { > > + return (sbi->options.tz_set ? -sbi->options.time_offset : > > + sys_tz.tz_minuteswest) * SECS_PER_MIN; } > > + > > > >> > >> > It could be an answer of another timestamp issue. > >> What is another timestamp issue ? > > > > What I'm saying is "timestamp incompatibilities in exfat-fs" from > > Reiner <reinerstallknecht@gmail.com> I think it might be the same > > issue with this. > Have you checked fuse-exfat patch he shared ? It was exfat timezone > support. > I am not sure how it is related to sys_tz... As I mentioned above, I misunderstood. :) I mistakenly thought that the code that handles timestamp without OffsetValid bit was added as well. Thanks! > > Thanks! > > > >> > >> > > >> > Could you please let me know what you think? > >> > > >> > Thanks. > >> >> -----Original Message----- > >> >> From: Namjae Jeon [mailto:linkinjeon@kernel.org] > >> >> Sent: Friday, September 10, 2021 10:01 AM > >> >> To: Chung-Chiang Cheng <cccheng@synology.com> > >> >> Cc: sj1557.seo@samsung.com; linux-fsdevel@vger.kernel.org; linux- > >> >> kernel@vger.kernel.org; shepjeng@gmail.com > >> >> Subject: Re: [PATCH] exfat: use local UTC offset when > >> >> EXFAT_TZ_VALID isn't set > >> >> > >> >> 2021-09-09 15:55 GMT+09:00, Chung-Chiang Cheng > <cccheng@synology.com>: > >> >> > EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat > >> >> > specification [1]. If this bit isn't set, timestamps should be > >> >> > treated as having the same UTC offset as the current local time. > >> >> > > >> >> > This patch uses the existing mount option 'time_offset' as fat > does. > >> >> > If time_offset isn't set, local UTC offset in sys_tz will be > >> >> > used as the default value. > >> >> > > >> >> > Link: [1] > >> >> > https://protect2.fireeye.com/v1/url?k=cba4edf5-943fd4c8-cba566ba > >> >> > -0c > >> >> > c47 > >> >> > a31309a-e70aa065be678729&q=1&e=225feff2-841f-404c-9a2e-c12064b23 > >> >> > 2d0 > >> >> > &u= > >> >> > https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fwindows%2Fwin32%2Ffil > >> >> > eio %2F exfat-specification%2374102-offsetvalid-field > >> >> > Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> > >> >> Please read this discussion: > >> >> > >> >> https://protect2.fireeye.com/v1/url?k=9bbcab7c-c4279381-9bbd2033-0 > >> >> 00babff317b-88ee581b44536876&q=1&e=b0d8a44b-4821-44a8-860a-4d3116c > >> >> b634d&u=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux- > >> >> fsdevel/patch/20200115082447.19520-10-namjae.jeon@samsung.com/ > >> >> > >> >> Thanks! > >> > > >> > > > > >
diff --git a/fs/exfat/exfat_fs.h b/fs/exfat/exfat_fs.h index 1d6da61157c9..b57fdd94ad01 100644 --- a/fs/exfat/exfat_fs.h +++ b/fs/exfat/exfat_fs.h @@ -204,6 +204,7 @@ struct exfat_mount_options { /* on error: continue, panic, remount-ro */ enum exfat_error_mode errors; unsigned utf8:1, /* Use of UTF-8 character set */ + tz_set:1, /* Filesystem timestamps' offset set */ discard:1; /* Issue discard requests on deletions */ int time_offset; /* Offset of timestamps from UTC (in minutes) */ }; diff --git a/fs/exfat/misc.c b/fs/exfat/misc.c index d34e6193258d..349e3fe64ea2 100644 --- a/fs/exfat/misc.c +++ b/fs/exfat/misc.c @@ -73,6 +73,12 @@ static void exfat_adjust_tz(struct timespec64 *ts, u8 tz_off) ts->tv_sec += TIMEZONE_SEC(0x80 - tz_off); } +static inline int exfat_tz_offset(struct exfat_sb_info *sbi) +{ + return (sbi->options.tz_set ? -sbi->options.time_offset : + sys_tz.tz_minuteswest) * SECS_PER_MIN; +} + /* 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, u8 tz, __le16 time, __le16 date, u8 time_cs) @@ -96,7 +102,7 @@ void exfat_get_entry_time(struct exfat_sb_info *sbi, struct timespec64 *ts, exfat_adjust_tz(ts, tz & ~EXFAT_TZ_VALID); else /* Convert from local time to UTC using time_offset. */ - ts->tv_sec -= sbi->options.time_offset * SECS_PER_MIN; + ts->tv_sec += exfat_tz_offset(sbi); } /* Convert linear UNIX date to a EXFAT time/date pair. */ diff --git a/fs/exfat/super.c b/fs/exfat/super.c index 5539ffc20d16..caeea375e4fa 100644 --- a/fs/exfat/super.c +++ b/fs/exfat/super.c @@ -173,7 +173,7 @@ static int exfat_show_options(struct seq_file *m, struct dentry *root) seq_puts(m, ",errors=remount-ro"); if (opts->discard) seq_puts(m, ",discard"); - if (opts->time_offset) + if (opts->tz_set) seq_printf(m, ",time_offset=%d", opts->time_offset); return 0; } @@ -303,6 +303,7 @@ static int exfat_parse_param(struct fs_context *fc, struct fs_parameter *param) */ if (result.int_32 < -24 * 60 || result.int_32 > 24 * 60) return -EINVAL; + opts->tz_set = 1; opts->time_offset = result.int_32; break; case Opt_utf8:
EXFAT_TZ_VALID is corresponding to OffsetValid field in exfat specification [1]. If this bit isn't set, timestamps should be treated as having the same UTC offset as the current local time. This patch uses the existing mount option 'time_offset' as fat does. If time_offset isn't set, local UTC offset in sys_tz will be used as the default value. Link: [1] https://docs.microsoft.com/en-us/windows/win32/fileio/exfat-specification#74102-offsetvalid-field Signed-off-by: Chung-Chiang Cheng <cccheng@synology.com> --- fs/exfat/exfat_fs.h | 1 + fs/exfat/misc.c | 8 +++++++- fs/exfat/super.c | 3 ++- 3 files changed, 10 insertions(+), 2 deletions(-)